Closed
Bug 1308996
Opened 9 years ago
Closed 9 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)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
Details
Attachments
(2 files, 2 obsolete files)
7.09 KB,
patch
|
Details | Diff | Splinter Review | |
10.36 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•9 years ago
|
||
![]() |
||
Comment 2•9 years ago
|
||
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!
Assignee | ||
Comment 3•9 years ago
|
||
BTW that patch is missing an amendment to the help message for the ARM flag; companion patch coming up.
Assignee | ||
Comment 4•9 years ago
|
||
(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).
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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)
![]() |
||
Comment 9•9 years ago
|
||
Can we make crash-on-unaligned opt-in so that tests work unmodified unless they set the appropriate hwcap flag?
Assignee | ||
Comment 10•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb2020d0325cc15def6299cf270ebbdd4c0e31b
Bug 1308996 - make simulated segfault fixups conditional. r=bbouvier
Comment 16•9 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c928343953fe
Backed out changeset 8cb2020d0325 for arm failures
Comment 17•9 years ago
|
||
had to backout for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38123617&repo=mozilla-inbound
Flags: needinfo?(lhansen)
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(lhansen)
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5affc0f009e9e260c3040e737ea333610d0cd31
Bug 1308996 - make simulated segfault fixups conditional. r=bbouvier
Comment 20•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•