Closed Bug 1308996 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

Attachments

(2 files, 2 obsolete files)

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().
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #8799485 - Flags: review?(bbouvier)
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!
BTW that patch is missing an amendment to the help message for the ARM flag; companion patch coming up.
(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).
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.
Attachment #8799678 - Flags: review?(bbouvier)
Comment on attachment 8799485 [details] [diff] [review]
bug1308996-conditional-simulated-segfault-fixups.patch

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

Sweet, thanks!
Attachment #8799485 - Flags: review?(bbouvier) → review+
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)
Attachment #8799678 - Flags: review?(bbouvier) → review+
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.)
Attachment #8801695 - Flags: feedback?(bbouvier)
Can we make crash-on-unaligned opt-in so that tests work unmodified unless they set the appropriate hwcap flag?
(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.
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?
(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 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.).
Attachment #8801695 - Flags: feedback?(bbouvier)
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+.
Attachment #8799485 - Attachment is obsolete: true
Attachment #8799678 - Attachment is obsolete: true
Attachment #8803877 - Flags: review+
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c928343953fe
Backed out changeset 8cb2020d0325 for arm failures
Flags: needinfo?(lhansen)
https://hg.mozilla.org/mozilla-central/rev/c5affc0f009e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.