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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

Attachment: 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

[Luke Wagner [:luke]]

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

Comment 2

[Andrew Paprocki]

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.

Comment 4

[Luke Wagner [:luke]]

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

Comment 5

[Andrew Paprocki]

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

[Luke Wagner [:luke]]

(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?

Comment 7

[Andrew Paprocki]

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.

Comment 9

[Luke Wagner [:luke]]

Sorry for the delay
http://hg.mozilla.org/tracemonkey/rev/e9c00ec4d920

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/e9c00ec4d920

Comment 11

[Landry Breuil (:gaston)]

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

Comment 12

[Landry Breuil (:gaston)]

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.

Comment 13

[Landry Breuil (:gaston)]

Attachment: 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.

Comment 14

[Luke Wagner [:luke]]

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

http://hg.mozilla.org/tracemonkey/rev/68203913d04c

Comment 15

[Landry Breuil (:gaston)]

Thanks, no worries, re-closing then. Oh, and that's "Landry" :)

Comment 16

[Luke Wagner [:luke]]

Oops, slip o' the fingers.  Not my day for proper attribution ;-)

Comment 19

[Sean Stangl [:sstangl]]

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.

Comment 20

[Sean Stangl [:sstangl]]

Followup to rebase this patch onto reality:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b041f8c5a7

Comment 21

[Landry Breuil (:gaston)]

Thanks sean! It was on my radar but i somehow completely forgot to push it to m-i.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da1921e38c6c
https://hg.mozilla.org/mozilla-central/rev/67b041f8c5a7

Comment 23

[Landry Breuil (:gaston)]

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

[Martin Husemann]

I have no local diffs for Value.h, and the only Heap.h changes are from #840242

Comment 25

[Landry Breuil (:gaston)]

What size is uintptr_t on NetBSD/sparc64 ? on OpenBSD, uintptr_t is defined as unsigned long, which sizeof is 8.

Comment 26

[Martin Husemann]

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

Comment 27

[Martin Husemann]

(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

[Martin Husemann]

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.

Comment 29

[Landry Breuil (:gaston)]

(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

[Martin Husemann]

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

[Mark Kettenis]

(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.

Comment 32

[Landry Breuil (:gaston)]

Attachment: 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.

Comment 33

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82fab5c8a6d5

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/82fab5c8a6d5