1678316 - Cross compiled to 32-bit, testJitABICalls fails a static assertion

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Using a mozconfig like the below

# Flags set for targeting x86.
export CROSS_COMPILE=1

CC="gcc -m32 -mfpmath=sse -msse -msse2"
CXX="g++ -m32 -mfpmath=sse -msse -msse2"
AR=ar
ac_add_options --target=i686-pc-linux

ac_add_options --enable-application=js

ac_add_options --disable-optimize
ac_add_options --enable-debug

ac_add_options --enable-tests

testJitABICalls.cpp fails to compile because of a failed static assertion:

// Not all 32bits architecture align uint64_t type on 8 bytes, so check the
// validity of the stored content based on the alignment of the architecture.
static_assert(alignof(uint64_t) != 8 ||
              std::is_same_v<ArgsOffsets_t<0, uint8_t, uint64_t, bool>,
                             std::integer_sequence<uint64_t, 0, 8, 16>>);

Tested on central (36ef6c97da5b).

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

This is strange, I did not had any issue natively (using an x86 toolchain on x64) compiling on x86 when I tested it in the past.
I will double check in 2 weeks.

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

In the mean time, you can remove this line, this should not impact the test behavior.
Otherwise, you might --disable-tests when compiling, which skip the compilation of jsapi-tests.

Comment 4

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1661256

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

Testing with a native x86 compiler and adding -m32 -mfpmath=sse -msse -msse2 was not enough to reproduce this issue locally.
Can you investigate what this issue is?

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Compiler version:

$  /usr/bin/g++ --versiong++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0

Exact compilation command:

/usr/bin/g++ -m32 -mfpmath=sse -msse -msse2 -std=gnu++17 -o Unified_cpp_js_src_jsapi-tests10.o -c  -I/home/matthew/unified/obj-debug-shell-i686-pc-linux-gnu/dist/system_wrappers -include /home/matthew/unified/config/gcc_hidden.h -fstack-protector-strong -DDEBUG=1 -DMOZ_HAS_MOZGLUE -DJS_CACHEIR_SPEW -DJS_STRUCTURED_SPEW -DEXPORT_JS_API -Dtopsrcdir=/home/matthew/unified/js/src -I/home/matthew/unified/js/src/jsapi-tests -I/home/matthew/unified/obj-debug-shell-i686-pc-linux-gnu/js/src/jsapi-tests -I/home/matthew/unified/obj-debug-shell-i686-pc-linux-gnu/js/src -I/home/matthew/unified/js/src -I/home/matthew/unified/obj-debug-shell-i686-pc-linux-gnu/dist/include -DMOZILLA_CLIENT -include /home/matthew/unified/obj-debug-shell-i686-pc-linux-gnu/js/src/js-confdefs.h -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=coverage-mismatch -Wno-error=free-nonheap-object -Wformat -Wformat-security -Wformat-overflow=2 -Wno-psabi -fno-sized-deallocation -fno-aligned-new -fPIC -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -fno-omit-frame-pointer -funwind-tables -fno-strict-aliasing -Werror=format -Wno-shadow -Wno-attributes  -MD -MP -MF .deps/Unified_cpp_js_src_jsapi-tests10.o.pp  -fdiagnostics-color  Unified_cpp_js_src_jsapi-tests10.cpp

In that static assert, both clauses are false:

• alignof(uint64_t) == 8
• ArgsOffsets_t<0, uint8_t, uint64_t, bool> is std::integer_sequence<long long unsigned int, 0, 8, 12> (dumped using below technique), not the expected std::integer_sequence<uint64_t, 0, 8, 16>> Key difference being in the last entry, 12 != 16

Recovered the actual value for the assert using

template <int s>
struct Wow;
Wow<ArgsOffsets_t<0, uint8_t, uint64_t, bool>> alg;

which provides the lovely compile error:

/home/matthew/unified/js/src/jsapi-tests/testJitABIcalls.cpp:251:45: error: type/value mismatch at argument 1 in template parameter list for ‘template<int s> struct Wow’
 Wow<ArgsOffsets_t<0, uint8_t, uint64_t, bool>> alg;
                                             ^~
/home/matthew/unified/js/src/jsapi-tests/testJitABIcalls.cpp:251:45: note:   expected a constant of type ‘int’, got ‘ArgsOffsets_t<0, unsigned char, long long unsigned int, bool> {aka std::integer_sequence<long long unsigned int, 0, 8, 12>}’

including all the info we need. Thanks yet again to this SO answer

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Exact mozconfig:

# Flags set for targeting x86.
export CROSS_COMPILE=1

CC="gcc -m32 -mfpmath=sse -msse -msse2"
CXX="g++ -m32 -mfpmath=sse -msse -msse2"
AR=ar
ac_add_options --target=i686-pc-linux

ac_add_options --enable-application=js

ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --enable-ccache=ccache
ac_add_options --enable-tests
ac_add_options --enable-build-backends=CompileDB,RecursiveMake

# Dump opt builds into another dir.
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-debug-shell-@CONFIG_GUESS@

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

So, my interpretation of this static assert is that we expect four more bytes of padding for that bool than actually happens.

Comment 9

[Nicolas B. Pierron [:nbp] {away}]

Thanks for the error message!

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #6)

• ArgsOffsets_t<0, uint8_t, uint64_t, bool> is std::integer_sequence<long long unsigned int, 0, 8, 12> (dumped using below technique), not the expected std::integer_sequence<uint64_t, 0, 8, 16>> Key difference being in the last entry, 12 != 16

There is likely a bug in the implementation of ArgsOffsets_t, as an uint64_t should reserve 8 bytes, and the gap between the offset 12 and 8 only leave 4 bytes for storing the uint64_t value. The problem likely lies in the discrepancy of computation:
https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/js/src/jsapi-tests/testJitABIcalls.cpp#240-241

Assuming sizeof(uint64_t) == 8, then we have the following Size + PadBytes(Size, alignof(Arg)) == 8 and Size + PadSize<Arg>(Size) = 4, where Size == 4 (= 0 + PadSize<bool>(0) where BackPadBytes<bool>() == 3).
Thus, PadBytes(4, alignof(uint64_t)) == 4 and PadBytes(4, ActualAlignOf<uint64_t>()) == 0.

This suggests that we have:

static_assert(alignof(uint64_t) == 8);

template <typename T>
constexpr size_t ActualAlignOf() {
  struct Wrapper {
    T _unused;
  };
  return alignof(Wrapper);
}
static_assert(ActualAlignOf<uint64_t>() == 4); // != alignof(uint64_t)

If so, this sounds like this would also be a compiler issue as a structure should inherit the highest alignment from its fields.

Can you paste the last assertion in the test case and check if it fails?
Can you try with a newer version of gcc?

Comment 10

[Nicolas B. Pierron [:nbp] {away}]

Attachment: Bug 1678316 - testJitABICalls: Always use the alignment of the pointee-type when computing offsets.

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Moving your little test case into a stand-alone file, compiles with /usr/bin/g++ -m32 -mfpmath=sse -msse -msse2 -std=gnu++17 -c t.cpp, so your asserts are on the money.

Experimenting with your patch, it also seems to fix it :)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e02533b47a67
testJitABICalls: Always use the alignment of the pointee-type when computing offsets. r=mgaudet

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e02533b47a67