1308996 - wasm: segfault handler emulation in ARM simulator makes it impossible to test whether correct code is generated for unaligned accesses

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

The point here is that if the CPU does not handle unaligned accesses then we need to be able to test that code generation is correct, but we can't do that if the simulator papers over the problem by emulating the signal handling that might result from an unaligned access.

In my opinion the code that calls wasm::IsPCInWasmCode to emulate this signal handler should be removed, but it could be that it is better for it to be enabled by a command line flag, eg, we could add an attribute to the ARMHWCAP string.  We already pass "align" to cause unaligned accesses to fault.  Perhaps adding an additional attribute "fixup" would be the right thing, that is, "fixup" by itself would have no effect (unaligned accesses are allowed) but "align,fixup" would be the current behavior and "align" by itself would cause a MOZ_CRASH().

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1308996-conditional-simulated-segfault-fixups.patch

Comment 2

[Luke Wagner [:luke]]

So, iiuc, the point is that we could write tests that do unaligned access with sub-natural alignment and not-crashing means we successfully emitted the unaligned codegen.  Sounds great!

Comment 3

[Lars T Hansen [:lth]]

BTW that patch is missing an amendment to the help message for the ARM flag; companion patch coming up.

Comment 4

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #2)
> So, iiuc, the point is that we could write tests that do unaligned access
> with sub-natural alignment and not-crashing means we successfully emitted
> the unaligned codegen.  Sounds great!

Indeed, with this flag in place I see some ARM wasm crashes where there shouldn't be any (in Ion-compiled code).

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1308996-armsim-flags-help-and-echo.patch

This patch adds simulator help text for the new flag, and a facility for echoing the flags on stdout (to aid use and debugging).

When I now run the wasm tests on the ARM simulator with the following flags, ie, without the new fixup flag:

  ARMHWCAP=armv7,vfp,vfpv3,vfpv4,neon,idiva,hardfp

I see crashes in these tests:

  jit-test/tests/wasm/memory.js
  jit-test/tests/wasm/spec/float_memory.wast.js
  jit-test/tests/wasm/spec/memory_redundancy.wast.js

These will likely have to be investigated before I can land the patches here, but it seems likely that they are existing bugs that were not seen earlier because of the unaligned-access masking, if they are not tests that are meant to deliberately test that masking and the signal handling.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8799485 [details] [diff] [review]
bug1308996-conditional-simulated-segfault-fixups.patch

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

Sweet, thanks!

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8799678 [details] [diff] [review]
bug1308996-armsim-flags-help-and-echo.patch

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

Thanks!

::: js/src/jit/arm/Architecture-arm.cpp
@@ +131,5 @@
> +PrintARMHwCapFlags(uint32_t flags)
> +{
> +    static const struct {
> +        uint32_t flag;
> +        const char *name;

nit: const char* name

@@ +145,5 @@
> +        {HWCAP_ARMv7, "armv7"},
> +        {HWCAP_ALIGNMENT_FAULT, "align"},
> +#ifdef JS_SIMULATOR_ARM
> +        {HWCAP_FIXUP_FAULT, "fixup"},
> +        {HWCAP_USE_HARDFP_ABI, "hardfp"},

Random suggestion: could we have this list in Architecture-arm.h instead, and use it also for parsing? This way we don't have to sync both lists together whenever one of those changes?

@@ +149,5 @@
> +        {HWCAP_USE_HARDFP_ABI, "hardfp"},
> +#endif
> +    };
> +
> +    printf("ARMHWCAP=");

Could you should either use fprintf(stderr, ...) or call flush(stdout) at the end of this function?

@@ +150,5 @@
> +#endif
> +    };
> +
> +    printf("ARMHWCAP=");
> +    bool separate=false;

nit: spaces before/after =

@@ +151,5 @@
> +    };
> +
> +    printf("ARMHWCAP=");
> +    bool separate=false;
> +    for ( size_t i=0 ; i < sizeof(items)/sizeof(items[0]) ; i++ ) {

nit: for (size_t i = 0; i < sizeof(items) / sizeof(items[0]); i++) {

@@ +154,5 @@
> +    bool separate=false;
> +    for ( size_t i=0 ; i < sizeof(items)/sizeof(items[0]) ; i++ ) {
> +        if (flags & items[i].flag) {
> +            printf("%s%s", separate ? "," : "", items[i].name);
> +            separate=true;

nit: see above

@@ +208,5 @@
>      armHwCapFlags = CanonicalizeARMHwCapFlags(flags);
>      JitSpew(JitSpew_Codegen, "ARM HWCAP: 0x%x\n", armHwCapFlags);
> +
> +    if (getenv("ARMHWCAP_PRINT") != nullptr)
> +        PrintARMHwCapFlags(armHwCapFlags);

Maybe put the call to JitSpew in the PrintARMHwCapFlags too? (here and below)

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1308996-predicate-and-test.patch

This is how it might look in a test case, I'd like some feedback before I adapt all the test cases as necessary.

Currently these test cases need fixing if we're going to land these patches:

jit-test/tests/wasm/bce.js
jit-test/tests/wasm/errors.js
jit-test/tests/wasm/memory.js
jit-test/tests/wasm/spec/address.wast.js
jit-test/tests/wasm/spec/float_memory.wast.js
jit-test/tests/wasm/spec/grow-memory.wast.js
jit-test/tests/wasm/spec/imports.wast.js
jit-test/tests/wasm/spec/memory_redundancy.wast.js

(I'm not very happy about having to modify the imported spec tests, obviously.)

Comment 9

[Luke Wagner [:luke]]

Can we make crash-on-unaligned opt-in so that tests work unmodified unless they set the appropriate hwcap flag?

Comment 10

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #9)

> Can we make crash-on-unaligned opt-in so that tests work unmodified unless
> they set the appropriate hwcap flag?

We can.  That somewhat defeats the purpose of the tests, but it would make it simpler to import spec tests naively, and we can compensate by having more elaborate private tests.

Comment 11

[Luke Wagner [:luke]]

If the problem is just being able to naively import spec tests, we could just have the test harness set the hwcap flag.  If crash-on-unaligned is on by default, doesn't that mean the fuzzer will be able to create crashing testcases w/o having to call the fuzzing-unsafe hwcap function?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #11)
> If crash-on-unaligned is on
> by default, doesn't that mean the fuzzer will be able to create crashing
> testcases w/o having to call the fuzzing-unsafe hwcap function?

That sounds right and it would be easy to trigger; any dynamic unaligned load/store would trigger it.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8801695 [details] [diff] [review]
bug1308996-predicate-and-test.patch

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

So it seems a bit unfortunate that we'd have to modify the test cases for this, especially the imported test cases. In particular, we won't see the number of tests being skipped, because we usually don't look at the testing output when everything goes well on treeherder.

I think the combination of align & ~fixup should be opt in, so the fuzzers don't hit this (too much? or they could blacklist the function making this opt in). Also, this introduces a new shell function, which fuzzers might use and it would then be a pain when we remove it. I really think we should just fix it, through a stub of always generated code that gets patched at runtime, or an emulated byte load, or anything really, instead of papering over the issue. I would like to take a stab at this soon. That being said, I don't know when this will happen, so if this is blocking you, we could take in the patch, for a short amount of time (famous last words et al.).

Comment 14

[Lars T Hansen [:lth]]

Attachment: bug1308996-conditional-simulated-segfault-fixups-v2.patch

Reworks first patch to make fixups the default; disable with "nofixup".  Imports the help text change from the other patch but abandons the rest of that.

Carrying r+.

Comment 15

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb2020d0325cc15def6299cf270ebbdd4c0e31b
Bug 1308996 - make simulated segfault fixups conditional. r=bbouvier

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c928343953fe
Backed out changeset 8cb2020d0325 for arm failures

Comment 17

[Carsten Book [:Tomcat]]

had to backout for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38123617&repo=mozilla-inbound

Comment 18

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1418a8aca75b

Comment 19

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5affc0f009e9e260c3040e737ea333610d0cd31
Bug 1308996 - make simulated segfault fixups conditional.  r=bbouvier

Comment 20

[Phil Ringnalda (:philor)]

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