Simplify some s390x inline assembly in unwinder#10973
Simplify some s390x inline assembly in unwinder#10973cfallin merged 1 commit intobytecodealliance:mainfrom
Conversation
|
I'm pretty perplexed staring at this one too -- for reference here are Godbolt links for before (working) and after (apparently segfaulting). The clobber-saves come in different order wrt loading values into payload (r6/r7) but the dataflow from arguments to PC/SP (r15)/payload (r6, r7) looks completely equivalent... |
|
cc @uweigand would you be able to help dig in to what's going on here? (we can provide more background context if necessary, but the general gist is that this PR in theory is a refactoring that shouldn't do anything but it causes tests to segfault) |
This was split out of bytecodealliance#10960 after some CI issues. The simplification of s390x is split to bytecodealliance#10973 to show its failure but otherwise these changes aren't expected to cause any issues (yet). Part of enabling this was in bytecodealliance#10974 where ASAN showed some issues as well. prtest:full
|
I was finally able to reproduce this. Turns out the crash only happens when building with |
crates/unwinder/src/arch/s390x.rs
Outdated
| in(reg) payload1, | ||
| in(reg) payload2, | ||
| in(reg) sp, | ||
| in(reg) pc, |
There was a problem hiding this comment.
Here's the problem. The br instruction requires an address register operand (i.e. GPR 1-15, but not GPR 0). In the failing case, the compiler chose to use GPR 0 to hold pc. To fix this, you'll need to use the reg_addr register class instead of reg for pc.
There was a problem hiding this comment.
Nice thanks so much for helping to track this down! Changing to reg_addr looks to fix it indeed!
Extracted from bytecodealliance#10960 with a bugfix. prtest:linux-s390x
|
Ok this should be good to go now, thanks again @uweigand! |
|
ping @pchickey, mind taking a look at this? (I can also reassign if you'd like too) |
cfallin
left a comment
There was a problem hiding this comment.
Happy to give a quick r+ since I wrote the original snippet; thanks!
This was split out of bytecodealliance#10960 after some CI issues. The simplification of s390x is split to bytecodealliance#10973 to show its failure but otherwise these changes aren't expected to cause any issues (yet). Part of enabling this was in bytecodealliance#10974 where ASAN showed some issues as well. prtest:full
Extracted from bytecodealliance#10960 with a bugfix. prtest:linux-s390x
Extracted from #10960 with an extra bugfix
prtest:linux-s390x