The default bug view has changed. See this FAQ.

64-bit big-endian jsval_layout is not defined, compile fails

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Andrew Paprocki, Unassigned)

Tracking

Trunk
Sun
Solaris
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 496976 [details] [diff] [review]
Adds 64-bit big-endian jsval_layout

jsval.h is missing the jsval_layout for 64-bit big-endian. This fails when we attempt to validate our code in both 32-/64-bit on all big-endian systems.

Comment 1

6 years ago
Comment on attachment 496976 [details] [diff] [review]
Adds 64-bit big-endian jsval_layout

Looks good.  Some unfortunate soul already caught what I hope is all our big/little-endian mistakes in the jits, but, just to be sure, jit tests pass? 

 cd js/src/jit-test
 python jit_tests.py path/to/js --jitflags=,j,m,jm,jmp
Attachment #496976 - Flags: review+
(Reporter)

Comment 2

6 years ago
I would have to complete a 64-bit build and check. I only caught this because my big-endian 32-bit work does 32- *and* 64-bit build checks upon checkin.

Updated

6 years ago
Duplicate of this bug: 619918

Comment 4

6 years ago
Well, I'll land your patch (as soon as the tree reopens) and we'll find out.
(Reporter)

Comment 5

6 years ago
I was able to build a 64-bit shell. Getting the tracejit to build required some other changes.

This is related to bug 577056. Until jsvals are 128-bit, realistically 64-bit Solaris won't work. I was able to get the shell to run in 64-bit by hacking jsgc to use valloc() instead of mmap(), but that was just to prove that it would actually run with my changes.

I do think this patch does have some merit, at least to ensure that the *rest* of the SM tree can continue to be build-tested in 64-bit. So tell me what you want to do.. I can give you code I changed to get 64-bit to build even though it won't run, as I think that is more useful than a tree that won't even build.

Comment 6

6 years ago
(In reply to comment #5)
> I was able to get the shell to run in 64-bit by hacking
> jsgc to use valloc() instead of mmap(), but that was just to prove that it
> would actually run with my changes.

I am probably misunderstanding you, but: if you can use valloc() (which I assume gives you memory with the high 17 bits zero?), then what prevents 64-bit jsvals from working?
(Reporter)

Comment 7

6 years ago
Well, it was just to see if it worked at all. The valloc/free replacement to mmap/munmap would not work in reality for me because in my embedding all memory must come from mmap. Also, this immediately crashes for any stack usage inside the engine, where the address can also be > 48-bit.

Updated

6 years ago
Duplicate of this bug: 620111

Comment 9

6 years ago
Sorry for the delay
http://hg.mozilla.org/tracemonkey/rev/e9c00ec4d920
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/e9c00ec4d920
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
With ffx 4.0b11, it still fails to build on OpenBSD/sparc64 :

c++ -o jsanalyze.o -c  -fvisibility=hidden -DOSTYPE=\"OpenBSD4\" -DOSARCH=OpenBSD -DEXPORT_JS_API -D__STDC_LIMIT_MACROS -DJS_HAS_CTYPES -DD
\".so.22.0\" -Ictypes/libffi/include -I.  -I. -I. -I./../../dist/include -I./../../dist/include/nsprpub  -I/usr/local/include/nspr   -I. -I
no-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-invalid-offset
=return-type -O2 -pipe -Wall -fno-strict-aliasing -pthread -pipe  -DNDEBUG -DTRIMMED -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JI
 ./js-confdefs.h -MD -MF .deps/jsanalyze.pp jsanalyze.cpp
In file included from jsobj.h:64,
                 from jsstr.h:56,
                 from jsatom.h:52,
                 from jscntxt.h:59,
                 from jsanalyze.h:45,
                 from jsanalyze.cpp:40:
jsvalue.h: In member function 'const jsuword* js::Value::payloadWord() const':
jsvalue.h:732: error: 'const union jsval_layout::<anonymous struct>::<anonymous>' has no member named 'word'

rev e9c00ec4d920 doesn't contain 'word' struct member, but i have no idea if blindly adding all the missing members is the way to go.

JSBool         boo;
JSString       *str;
JSObject       *obj;
void           *ptr;
jsuword        word;

I think this bug should be reopened..

The only related change i have is in js/src/configure.in:

@@ -3013,6 +3013,10 @@ arm*-*)
 sparc-*)
     AC_DEFINE(AVMPLUS_SPARC)
     ;;
+sparc64-*)
+    AC_DEFINE(AVMPLUS_SPARC)
+    AC_DEFINE(AVMPLUS_64BIT)
+    ;;
 esac
 
 case "$target" in
looking at http://hg.mozilla.org/mozilla-central/diff/c57549b012a2/js/src/jsval.h, i think it's just a missing 'jsuword word;' member in the long endian/64 bits case.
Created attachment 517107 [details] [diff] [review]
Add jsuword word to jsval_layout on 64-bit big-endian

Here's the patch i apply on OpenBSD to make ffx4b12 build on sparc64.
Attachment #517107 - Flags: review?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #517107 - Flags: review? → review?(luke)

Updated

6 years ago
Attachment #517107 - Flags: review?(luke) → review+

Comment 14

6 years ago
Arg, forgot to hg qref -u "Laundry Breuil"; sorry about that!

http://hg.mozilla.org/tracemonkey/rev/68203913d04c
Thanks, no worries, re-closing then. Oh, and that's "Landry" :)
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 16

6 years ago
Oops, slip o' the fingers.  Not my day for proper attribution ;-)
Duplicate of this bug: 627664

Updated

6 years ago
Duplicate of this bug: 627664
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1921e38c6c

The patch from Comment 13 was r+'d but never landed. We carried it separately for the ESR17 standalone release, so merging onto mainline before ESR24.
Followup to rebase this patch onto reality:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b041f8c5a7
Thanks sean! It was on my radar but i somehow completely forgot to push it to m-i.
https://hg.mozilla.org/mozilla-central/rev/da1921e38c6c
https://hg.mozilla.org/mozilla-central/rev/67b041f8c5a7
I'm a bit lost right now, since i (well, mostly martin) resumed working on sparc64 support for m-c, and it seems that this struct member (ie uintptr_t word) makes the struct size blow :

./../../dist/include/js/Value.h:335:1: error: size of array 'moz_static_assert11' is negative
./../../dist/include/js/Value.h: In member function 'void JS::Value::staticAssertions()':
./../../dist/include/js/Value.h:1173:9: error: size of array 'moz_static_assert16' is negative

It's not in the struct in the LE case, so is it really needed for the BE case ? Commenting it out make that assert go away for another one :

/home/cvs/landry/m-c/js/src/gc/Heap.h:837:1: error: size of array 'moz_static_assert56' is negative

martin, do you have a patch for js/public/Value.h to fix jsval_layout on sparc64 ? And what about Heap.h ? Could that one be loosely linked to bug 840242

Comment 24

4 years ago
I have no local diffs for Value.h, and the only Heap.h changes are from #840242
What size is uintptr_t on NetBSD/sparc64 ? on OpenBSD, uintptr_t is defined as unsigned long, which sizeof is 8.

Comment 26

4 years ago
Same on NetBSD - sizeof(jsval_layout::asPtr) == sizeof(jsval_layout::asUIntPtr) == sizeof(jsval_layout) == 8

Comment 27

4 years ago
(In reply to Andrew Paprocki from comment #7)
> [] Also, this immediately crashes for any stack
> usage inside the engine, where the address can also be > 48-bit.

This is true, though in practical testing I have not run into it (yet).
If we want to go fancy, we could do something like:

    struct debugView {
	uint64_t           high_bit : 1;
        JSValueTag         tag : 17;
        uint64_t           payload46 : 46;
    };

and propagate the high_bit value on decomposition over the tag bits.

(Just thinking out loud, since this just came up again)

Comment 28

4 years ago
Thinking further about it: the problem would only happen with JS::Value encapsulating pointers to objects on the main thread's stack, due to implementation characteristics of our libpthread, kernel mmap implementation and userland virtual address space layout.

So another option would be to add a runtime assertion to the tagging macros for this case and call the whole issue academic untill it fires.

I don't know if the same is true for Solaris (nor wether they care about a 64bit version of Firefox).

Landry, I guess with OpenBSD it is similar?

We might even ignore it for now, and add the assert only once we suspect to see related lossage.
(In reply to Martin Husemann from comment #26)
> Same on NetBSD - sizeof(jsval_layout::asPtr) ==
> sizeof(jsval_layout::asUIntPtr) == sizeof(jsval_layout) == 8

I was more concerned about the sizeof(uintptr_t) itself - word seems to be the struct member that makes the whole struct size bigger than 8 on OpenBSD it seems.

And it is not in the JS_BITS_PER_WORD == 64 && defined(IS_LITTLE_ENDIAN) jsval_layout, apparently it got removed at some point - so not used ? payload.word is only shown in js/src/ion/IonFrames.cpp

Comment 30

4 years ago
Oh, I see what you mean - check the JSVAL_IS... and JSVAL_TO... accessors later down in Value.h, which are all defined via various union members for 32bit, but via asBits in the 64bit case.

So the 64bit BE case can be simplified to the equivalent of its 64bit LE cousin.

(The branch I worked from did not have your "word" changeset yet, which caused the main confusion)

Comment 31

4 years ago
(In reply to Martin Husemann from comment #28)
> Thinking further about it: the problem would only happen with JS::Value
> encapsulating pointers to objects on the main thread's stack, due to
> implementation characteristics of our libpthread, kernel mmap implementation
> and userland virtual address space layout.
> 
> So another option would be to add a runtime assertion to the tagging macros
> for this case and call the whole issue academic untill it fires.
> 
> I don't know if the same is true for Solaris (nor wether they care about a
> 64bit version of Firefox).
> 
> Landry, I guess with OpenBSD it is similar?

I think so.  We have the VA "hole" between 0x000007ffffffffff and 0xfffff80000000000 just like NetBSD.  I never bothered to add another level to the page tables to support  newer CPUs that have a more VA bits implemented.  The stack for the main thread is allocated up high above the hole and grows down.  Addresses returned by mmap(2) are randomized somewhat aggressively on OpenBSD, will fall below the hole.

> We might even ignore it for now, and add the assert only once we suspect to
> see related lossage.

I think I saw such lossage when I looked at the issue a couple of years ago.  In that case your suggestion from comment #27 should be the way to go.
Created attachment 757462 [details] [diff] [review]
Remove uintptr_t word to jsval_layout on 64-bit big-endian

So let's start by removing this crazy unused word member, effectively reverting da1921e38c6c & 67b041f8c5a7.
Attachment #757462 - Flags: review?(luke)

Updated

4 years ago
Attachment #757462 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/82fab5c8a6d5
https://hg.mozilla.org/mozilla-central/rev/82fab5c8a6d5

Updated

3 years ago
See Also: → bug 994133
You need to log in before you can comment on or make changes to this bug.