Page MenuHomePhabricator

[ARM] Fix PR35379 - incorrect unwind information when compiling with -Oz
ClosedPublic

Authored by chill on Dec 15 2017, 9:42 AM.

Details

Summary

Compiling with -Oz enable an optimisation, where the compiler tries to fold an update to SP by adding extra registers to existing push/pop instructions. This doesn't interact correctly with ARMAsmPrinter::EmitUnwindingInstruction - the generated unwind information causes the extra registers to be restored when an exception is propagated through the function, but the corresponding stack slots can be already clobbered.

When adding these registers to a push, the register operands are marked as "undef". This patch modifies ARMAsmPrinter::EmitUnwindingInstruction to examine the register operand flags and not mention in the .save the registers, which
have this flag set.

Diff Detail

Repository
rL LLVM

Event Timeline

chill created this revision.Dec 15 2017, 9:42 AM

If I understand correctly, with this fix, we'll output something like this for the original test case (https://godbolt.org/g/rJRr75):

work:
  .save {r11, lr}
  push {r9, r10, r11, lr}
  .setfp r11, sp, #8
  add r11, sp, #8
  ...

The .save directive apparently only accounts for 8 bytes of SP adjustment, but SP was decremented by 16. If I assemble it, then the readelf -u output looks broken:

$ arm-linux-androideabi-gcc -c test.s && readelf -u test.o

Unwind table index '.ARM.exidx' at offset 0x64 contains 1 entries:

0x0 <work>: @0x0
  Compact model index: 1
  0x9b      vsp = r11
  0x41      vsp = vsp - 8
  0x84 0x80 pop {r11, r14}
  0xb0      finish
  0xb0      finish

(r11 points at the saved {r11, r14} registers. The vsp = vsp - 8 adjustment is wrong.)

If the tryFoldSPUpdateIntoPushPop optimization is disabled (e.g. by compiling with -Os instead), there is a separate 8-byte stack adjustment, which is marked with a .pad directive:

.pad #8
sub sp, sp, #8

It looks like EmitUnwindingInstruction also emits the .pad directive. If I add a .pad #8 directive to my assembly file, immediately after the .save directive, then the readelf -u output is fixed. (i.e. The vsp = vsp - 8 line disappears.)

@rprichard , thank you, I'll look at this.

chill updated this revision to Diff 127240.EditedDec 16 2017, 3:50 AM

Update: fix stack adjustment information in unwind info, so it accounts for the extra registers used for padding; now the unwind info should be identical, whether using -Os or not; added a test for this.

I think "Diff 2" fixes the problem I found. The fix seems OK to me.

lib/Target/ARM/ARMAsmPrinter.cpp
1120 ↗(On Diff #127240)

I wonder if we should assert that the registers to skip appear at the front of the instruction. e.g. Add an assert(RegList.empty()); inside the MO.isUndef() block.

test/CodeGen/ARM/PR35379.ll
41 ↗(On Diff #127240)

typo in "... same even whetherw or not ..."

chill updated this revision to Diff 127755.Dec 20 2017, 10:12 AM
chill marked 2 inline comments as done.
chill added inline comments.
lib/Target/ARM/ARMAsmPrinter.cpp
1120 ↗(On Diff #127240)

To me, it makes sense doing it.

chill marked an inline comment as done.Jan 2 2018, 1:45 AM

Ping?

compnerd accepted this revision.Jan 5 2018, 5:37 PM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
test/CodeGen/ARM/PR35379.ll
23 ↗(On Diff #127755)

Can you add a CHECK-NOT here for the save? The check next attempts to do that, but explicitly ensuring it doesn’t happen would be better (and makes it much more obvious).

29 ↗(On Diff #127755)

And here

This revision is now accepted and ready to land.Jan 5 2018, 5:37 PM
This revision was automatically updated to reflect the committed changes.
chill added inline comments.Jan 12 2018, 9:56 AM
test/CodeGen/ARM/PR35379.ll
23 ↗(On Diff #127755)

I'm sorry, I forgot to do this. I'll do it in a follow-up patch.