Last Comment Bug 618485 - 64-bit big-endian jsval_layout is not defined, compile fails
: 64-bit big-endian jsval_layout is not defined, compile fails
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: Sun Solaris
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
: 619918 620111 627664 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-10 15:51 PST by Andrew Paprocki
Modified: 2014-04-09 08:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds 64-bit big-endian jsval_layout (706 bytes, patch)
2010-12-10 15:51 PST, Andrew Paprocki
luke: review+
Details | Diff | Splinter Review
Add jsuword word to jsval_layout on 64-bit big-endian (387 bytes, patch)
2011-03-05 03:06 PST, Landry Breuil (:gaston)
luke: review+
Details | Diff | Splinter Review
Remove uintptr_t word to jsval_layout on 64-bit big-endian (827 bytes, patch)
2013-06-03 09:53 PDT, Landry Breuil (:gaston)
luke: review+
Details | Diff | Splinter Review

Description Andrew Paprocki 2010-12-10 15:51:53 PST
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 Luke Wagner [:luke] 2010-12-10 16:26:12 PST
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 2010-12-10 16:49:10 PST
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 3 Luke Wagner [:luke] 2010-12-17 09:43:11 PST
*** Bug 619918 has been marked as a duplicate of this bug. ***
Comment 4 Luke Wagner [:luke] 2010-12-17 09:46:45 PST
Well, I'll land your patch (as soon as the tree reopens) and we'll find out.
Comment 5 Andrew Paprocki 2010-12-17 10:57:37 PST
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] 2010-12-17 13:48:50 PST
(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 2010-12-17 14:00:58 PST
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 8 Luke Wagner [:luke] 2010-12-18 11:39:17 PST
*** Bug 620111 has been marked as a duplicate of this bug. ***
Comment 9 Luke Wagner [:luke] 2011-01-03 11:35:35 PST
Sorry for the delay
http://hg.mozilla.org/tracemonkey/rev/e9c00ec4d920
Comment 10 Chris Leary [:cdleary] (not checking bugmail) 2011-01-04 14:28:46 PST
http://hg.mozilla.org/mozilla-central/rev/e9c00ec4d920
Comment 11 Landry Breuil (:gaston) 2011-02-14 12:40:42 PST
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) 2011-02-14 12:59:36 PST
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) 2011-03-05 03:06:33 PST
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.
Comment 14 Luke Wagner [:luke] 2011-03-07 09:53:36 PST
Arg, forgot to hg qref -u "Laundry Breuil"; sorry about that!

http://hg.mozilla.org/tracemonkey/rev/68203913d04c
Comment 15 Landry Breuil (:gaston) 2011-03-07 14:52:33 PST
Thanks, no worries, re-closing then. Oh, and that's "Landry" :)
Comment 16 Luke Wagner [:luke] 2011-03-07 16:55:04 PST
Oops, slip o' the fingers.  Not my day for proper attribution ;-)
Comment 17 AWAY Tom Schuster [:evilpie] 2011-06-14 13:55:29 PDT
*** Bug 627664 has been marked as a duplicate of this bug. ***
Comment 18 Chris Coulson 2011-06-14 13:57:06 PDT
*** Bug 627664 has been marked as a duplicate of this bug. ***
Comment 19 Sean Stangl [:sstangl] 2013-04-25 16:32:17 PDT
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] 2013-04-25 17:06:51 PDT
Followup to rebase this patch onto reality:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b041f8c5a7
Comment 21 Landry Breuil (:gaston) 2013-04-25 22:33:46 PDT
Thanks sean! It was on my radar but i somehow completely forgot to push it to m-i.
Comment 23 Landry Breuil (:gaston) 2013-05-31 17:59:23 PDT
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 2013-06-01 03:10:20 PDT
I have no local diffs for Value.h, and the only Heap.h changes are from #840242
Comment 25 Landry Breuil (:gaston) 2013-06-01 06:22:55 PDT
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 2013-06-03 00:11:13 PDT
Same on NetBSD - sizeof(jsval_layout::asPtr) == sizeof(jsval_layout::asUIntPtr) == sizeof(jsval_layout) == 8
Comment 27 Martin Husemann 2013-06-03 01:56:36 PDT
(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 2013-06-03 03:07:06 PDT
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) 2013-06-03 08:09:49 PDT
(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 2013-06-03 08:40:15 PDT
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 2013-06-03 08:40:55 PDT
(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) 2013-06-03 09:53:07 PDT
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.
Comment 34 Ed Morley [:emorley] 2013-06-10 02:11:31 PDT
https://hg.mozilla.org/mozilla-central/rev/82fab5c8a6d5

Note You need to log in before you can comment on or make changes to this bug.