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

[chill]

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.

[rprichard]

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.)

[chill]

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

[chill]

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.

[rprichard]

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]

lib/Target/ARM/ARMAsmPrinter.cpp

1120 ↗

(On Diff #127240)

To me, it makes sense doing it.

[chill]

Ping?

[compnerd]

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

[chill]

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.

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

if (MI->mayStore()) {
     assert(DstReg == ARM::SP &&
            "Only stack pointer as a destination reg is supported");
 
     SmallVector<unsigned, 4> RegList;
     // Skip src & dst reg, and pred ops.
     unsigned StartOp = 2 + 2;
     // Use all the operands.
     unsigned NumOffset = 0;
+    // Amount of SP adjustment folded into a push.
+    unsigned Pad = 0;
 
     switch (Opc) {
     default:
       MI->print(errs());
       llvm_unreachable("Unsupported opcode for unwinding information");
     case ARM::tPUSH:
       // Special case here: no src & dst reg, but two extra imp ops.
       StartOp = 2; NumOffset = 2;
       LLVM_FALLTHROUGH;
     case ARM::STMDB_UPD:
     case ARM::t2STMDB_UPD:
     case ARM::VSTMDDB_UPD:
       assert(SrcReg == ARM::SP &&
              "Only stack pointer as a source reg is supported");
       for (unsigned i = StartOp, NumOps = MI->getNumOperands() - NumOffset;
            i != NumOps; ++i) {
         const MachineOperand &MO = MI->getOperand(i);
         // Actually, there should never be any impdef stuff here. Skip it
         // temporary to workaround PR11902.
         if (MO.isImplicit())
           continue;
+        // Registers, pushed as a part of folding an SP update into the
+        // push instruction are marked as undef and should not be
+        // restored when unwinding, because the function can modify the
+        // corresponding stack slots.
+        if (MO.isUndef()) {
+          assert(RegList.empty() &&
+                 "Pad registers must come before restored ones");
+          Pad += 4;
+          continue;
+        }
         RegList.push_back(MO.getReg());
       }
       break;
     case ARM::STR_PRE_IMM:
     case ARM::STR_PRE_REG:
     case ARM::t2STR_PRE:
       assert(MI->getOperand(2).getReg() == ARM::SP &&
              "Only stack pointer as a source reg is supported");
       RegList.push_back(SrcReg);
       break;
     }
-    if (MAI->getExceptionHandlingType() == ExceptionHandling::ARM)
+    if (MAI->getExceptionHandlingType() == ExceptionHandling::ARM) {
       ATS.emitRegSave(RegList, Opc == ARM::VSTMDDB_UPD);
+      // Account for the SP adjustment, folded into the push.
+      if (Pad)
+        ATS.emitPad(Pad);
+    }
   } else {
     // Changes of stack / frame pointer.
     if (SrcReg == ARM::SP) {
       int64_t Offset = 0;
       switch (Opc) {
       default:
         MI->print(errs());
         llvm_unreachable("Unsupported opcode for unwinding information");

llvm/trunk/lib/Target/ARM/ARMBaseInstrInfo.cpp

bool llvm::tryFoldSPUpdateIntoPushPop(const ARMSubtarget &Subtarget,
 
   const MCPhysReg *CSRegs = TRI->getCalleeSavedRegs(&MF);
 
   // Now try to find enough space in the reglist to allocate NumBytes.
   for (int CurRegEnc = FirstRegEnc - 1; CurRegEnc >= 0 && RegsNeeded;
        --CurRegEnc) {
     unsigned CurReg = RegClass->getRegister(CurRegEnc);
     if (!IsPop) {
-      // Pushing any register is completely harmless, mark the
-      // register involved as undef since we don't care about it in
-      // the slightest.
+      // Pushing any register is completely harmless, mark the register involved
+      // as undef since we don't care about its value and must not restore it
+      // during stack unwinding.
       RegList.push_back(MachineOperand::CreateReg(CurReg, false, false,
                                                   false, false, true));
       --RegsNeeded;
       continue;
     }
 
     // However, we can only pop an extra register if it's not live. For
     // registers live within the function we might clobber a return value

llvm/trunk/test/CodeGen/ARM/PR35379.ll

+; RUN: llc -mtriple=armv7a-eabi < %s | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=armv6m-eabi < %s | FileCheck %s --check-prefix=CHECK-THM
+
+; Function Attrs: minsize optsize
+declare void @g(i32*) local_unnamed_addr #0
+
+; Function Attrs: minsize optsize
+define void @f() local_unnamed_addr #0 {
+entry:
+  %i = alloca i32, align 4
+  %0 = bitcast i32* %i to i8*
+  store i32 1, i32* %i, align 4
+  call void @g(i32* nonnull %i)
+  ret void
+}
+
+; Check unwind info does not mention the registers used for padding, and
+; the amount of stack adjustment is the same as in the actual
+; instructions.
+
+; CHECK-ARM:      .save {r11, lr}
+; CHECK-ARM-NEXT: .pad #8
+; CHECK-ARM-NEXT: push {r9, r10, r11, lr}
+; CHECK-ARM:      pop {r2, r3, r11, pc}
+
+; CHECK-THM:      .save {r7, lr}
+; CHECK-THM-NEXT: .pad #8
+; CHECK-THM-NEXT: push {r5, r6, r7, lr}
+; CHECK-THM:      pop {r2, r3, r7, pc}
+
+
+define void @f1() local_unnamed_addr #1 {
+entry:
+  %i = alloca i32, align 4
+  %0 = bitcast i32* %i to i8*
+  store i32 1, i32* %i, align 4
+  call void @g(i32* nonnull %i)
+  ret void
+}
+
+; Check that unwind info is the same whether or not using -Os (minsize attr)
+
+; CHECK-ARM:      .save {r11, lr}
+; CHECK-ARM-NEXT: push {r11, lr}
+; CHECK-ARM-NEXT: .pad #8
+
+; CHECK-THM:      .save {r7, lr}
+; CHECK-THM-NEXT: push {r7, lr}
+; CHECK-THM-NEXT: .pad #8
+
+attributes #0 = { minsize optsize }
+attributes #1 = { optsize }