Closed Bug 1439885 Opened 2 years ago Closed 2 years ago

error: static assertion failed: RelocationOverlay::magic_ is in the wrong location

Categories

(Core :: JavaScript: GC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: petr.sumbera, Assigned: petr.sumbera)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180206200532

Steps to reproduce:

I have recently started to see following on sparc64 build (Solaris):

/usr/bin/g++ -std=gnu++14 -o CTypes.o -c -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/system_wrappers -include /scratch/firefox/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_WASM_SATURATING_TRUNC_OPS -DENABLE_WASM_SIGNEXTEND_OPS -DENABLE_WASM_THREAD_OPS -DENABLE_WASM_GLOBAL -DJS_CACHEIR_SPEW -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES '-DDLL_PREFIX="lib"' '-DDLL_SUFFIX=".so"' -DMOZ_HAS_MOZGLUE -I/scratch/firefox/js/src -I/scratch/firefox/obj-sparc64-sun-solaris2.11/js/src -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/include -I/scratch/firefox/obj-sparc64-sun-solaris2.11/dist/include/nspr -fPIC -DMOZILLA_CLIENT -include /scratch/firefox/obj-sparc64-sun-solaris2.11/js/src/js-confdefs.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -O -fno-omit-frame-pointer -DFFI_NO_RAW_API -I/usr/lib/sparcv9/libffi-3.2.1/include -Wno-shadow -Werror=format -fno-strict-aliasing  -MD -MP -MF .deps/CTypes.o.pp  -fdiagnostics-color  /scratch/firefox/js/src/ctypes/CTypes.cpp
In file included from /scratch/firefox/js/src/vm/JSObject-inl.h:29:0,
                 from /scratch/firefox/js/src/ctypes/CTypes.cpp:45:
/scratch/firefox/js/src/gc/Marking-inl.h: In member function ‘void js::gc::RelocationOverlay::forwardTo(js::gc::Cell*)’:
/scratch/firefox/js/src/gc/Marking-inl.h:95:5: error: static assertion failed: RelocationOverlay::magic_ is in the wrong location
     static_assert(offsetof(RelocationOverlay, magic_) == offsetof(JSObject, group_) + sizeof(uint32_t),
     ^
/scratch/firefox/js/src/gc/Marking-inl.h:97:5: error: static assertion failed: RelocationOverlay::magic_ is in the wrong location
     static_assert(offsetof(RelocationOverlay, magic_) == offsetof(js::Shape, base_) + sizeof(uint32_t),
     ^
/scratch/firefox/js/src/gc/Marking-inl.h:99:5: error: static assertion failed: RelocationOverlay::magic_ is in the wrong location
     static_assert(offsetof(RelocationOverlay, magic_) == offsetof(JSString, d.u1.length),
     ^
I'm unsure if this is a JavaScript : GC issue or it's a Build Config one.

:jonco do you have any thoughts on this?
Component: Untriaged → JavaScript: GC
Flags: needinfo?(jcoppeard)
Product: Firefox → Core
It looks like these assertions don't take account of endianness.
Flags: needinfo?(jcoppeard) → needinfo?(sphink)
Priority: -- → P3
I tried to get this right for big-endian, but apparently failed. I think this is probably a simple fix: #include "mozilla/EndianUtils.h", then #ifdef MOZ_LITTLE_ENDIAN the static_asserts in https://searchfox.org/mozilla-central/source/js/src/gc/Marking-inl.h#95 and make big-endian variants of them. I *think* I made sure the layout is correct for big-endian (as per the comment at the above link.) I don't have time to do it myself at the moment.
Attached patch Bug1439885.patch (obsolete) — Splinter Review
Attachment #8955120 - Flags: review?(sphink)
Comment on attachment 8955120 [details] [diff] [review]
Bug1439885.patch

Review of attachment 8955120 [details] [diff] [review]:
-----------------------------------------------------------------

Ooh boy. Check me on this, because I'm awful at keeping endianness straight.

I think this patch is wrong, and I set up the 64-bit big-endian RelocationOverlay incorrectly. And I underdocumented. So let me try talking through this:

On little-endian:
  offset 0 is the preserve_ field of RelocationOverlay, the low 32 bits of group_ or base_ pointers, or the flags field of strings.
  offset 4 is the magic_ field of RelocationOverlay, the high 32 bits of group_ or base_, or the length field of strings.

The low bit of flags must be preserved, because it is used to distinguish between strings and objects. For strings, it is the NON_ATOM bit, which will always be set for a string in the nursery (atoms are never in the nursery). For objects, it is the low bit of an aligned pointer, so it will always be clear.

The magic value Relocated is 0xbad0bad1. Its low bit is set. This will not conflict with low bit of the low 32 bits of group_/base_, since they are aligned so the bit will be clear. It is also not a valid string length, as the maximum string length is 2^28-1.

For big-endian:
  offset 0 is the magic_ field of RelocationOverlay, the high 32 bits of group_ or base_ pointers, or the flags field of strings.
  offset 4 is the preserve_ field of RelocationOverlay, the low 32 bits of group_ or base_, or the length field of strings.

This is no good. When magic_ is set to 0xbad0bad1, it is at the same location as the flags field of strings, so when we are distinguishing strings from objects, we might find a relocated object and incorrectly thing it is a string. Assuming big-endian platforms always have high pointer bits clear, it will work ok for group_/base_.

I think we want, for big-endian:
  offset 0 is the preserve_ field of RelocationOverlay, the high 32 bits of group_ or base_ pointers, or the flags field of strings.
  offset 4 is the magic_ field of RelocationOverlay, the low 32 bits of group_ or base_, or the length field of strings.

Now the string flags field overlays preserve_, which is good because we need to preserve it. We will write 0xbad0bad1 into the low 32 bits of group_/base_, which is good because it cannot be the low 32 bits of an aligned pointer. It will also alias the string length, which is ok for the same reasons as little-endian.

So the main bug here is in RelocationOverlay.h; it should not have #if MOZ_LITTLE_ENDIAN, it should just always have preserve_ followed by magic_.

For the assertions, I think for big-endian we want the exact same thing as little-endian: magic_ should alias 32 bits after the start of group_/base_, or the string length. The only difference between little- and big-endian is whether we're aliasing the low or high bits of those pointers, and fortunately our magic value is invalid for either, assuming 64-bit pointers have their high bit clear. (Which is true on our supported platforms, but not on all.)

Ironically, if I hadn't tried to handle big-endianness at all, it would have been fine.
Attachment #8955120 - Flags: review?(sphink) → review-
Attached patch Bug1439885.patchSplinter Review
Attachment #8955120 - Attachment is obsolete: true
Attachment #8957587 - Flags: review?(sphink)
Comment on attachment 8957587 [details] [diff] [review]
Bug1439885.patch

Review of attachment 8957587 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I'm assuming this worked for you.
Attachment #8957587 - Flags: review?(sphink) → review+
Flags: needinfo?(sphink)
Attachment #8957587 - Flags: checkin?
Assignee: nobody → petr.sumbera
The patch works for me as well on sparc64-linux. We still need the patch from 1144632 as well.
Attachment #8957587 - Flags: checkin? → checkin+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/d7b8250f6b17
Fix RelocationOverlay for big-endian machines in JavaScript : GC. r=sfink
https://hg.mozilla.org/mozilla-central/rev/d7b8250f6b17
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.