[ARM] Restore the right frame pointer register in Int_eh_sjlj_longjmp

[mstorsjo]

Summary

In setupEntryBlockAndCallSites in CodeGen/SjLjEHPrepare.cpp, we fetch and store the actual frame pointer, but on return via the longjmp intrinsic, it always was restored into the r7 variable.

On windows, the frame pointer should be restored into r11 instead of r7.

On Darwin (where sjlj exception handling is used by default), the frame pointer is always r7, both in arm and thumb mode, and likewise, on windows, the frame pointer always is r11.

On linux however, if sjlj exception handling is enabled (which it isn't by default), libcxxabi and the user code can be built in differing modes using different registers as frame pointer. Therefore, when restoring registers on a platform where we don't always use the same register depending on code mode, restore both r7 and r11.

[compnerd]

Um, doesn't that clobber a register which is unexpected?

[mstorsjo]

In D38253#881421, @compnerd wrote:

Um, doesn't that clobber a register which is unexpected?

I would think it doesn't really matter. Since the __builtin_longjmp only restores fp, sp and pc, the landing pad needs to restore any other registers it expects to have any specific value anyway, so I would expect it not to matter if we additionally modify r7/r11 here.

If you disagree, I can at least provide a version of this patch that only restores one single register, based on useR7AsFramePointer(). That should work in all cases except on linux, if cxxabi and the user code are built in differing arm/thumb modes.

[mstorsjo]

FWIW, this was more or less approved by @t.p.northover on IRC:

"The second one is ugly, but that describes SjLj as a whole so I doubt we'd be making the world a worse place." and "There was actually some talk about standardising FP a while ago. Doesn't seem to have gone anywhere yet though."

So unless there's further comments, I'll probably push this later today/tomorrow.

[MatzeB]

In D38253#883234, @mstorsjo wrote:

FWIW, this was more or less approved by @t.p.northover on IRC:

"The second one is ugly, but that describes SjLj as a whole so I doubt we'd be making the world a worse place." and "There was actually some talk about standardising FP a while ago. Doesn't seem to have gone anywhere yet though."

This is NOT how patch aproval works in llvm!! Please revert!

Just glancing at this I can see this changes the asmprinter output but not the modelling of the instruction which now changes r11 in some cases but does not model that fact with machine operands.

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

1207

There’s no need to put those statements on the hot path of this function.

[mstorsjo]

In D38253#884501, @MatzeB wrote:

In D38253#883234, @mstorsjo wrote:

FWIW, this was more or less approved by @t.p.northover on IRC:

"The second one is ugly, but that describes SjLj as a whole so I doubt we'd be making the world a worse place." and "There was actually some talk about standardising FP a while ago. Doesn't seem to have gone anywhere yet though."

This is NOT how patch aproval works in llvm!! Please revert!

Really sorry about this - I entirely missed this comment back then... :-( At this point, though, reverting is a bit late/harsh...

Just glancing at this I can see this changes the asmprinter output but not the modelling of the instruction which now changes r11 in some cases but does not model that fact with machine operands.

A suggested follow-up patch in D109041 adds r11 for tInt_eh_sjlj_longjmp in ARMInstrThumb.td - does that fix this particular concern (if we'd do the same for Int_eh_sjlj_longjmp in ARMInstrInfo.td too)?

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

1207

Good point, I'll send a separate patch to fix that, if they aren't used elsewhere now.

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

 // instructions) auto-generated.
 #include "ARMGenMCPseudoLowering.inc"
 
 void ARMAsmPrinter::EmitInstruction(const MachineInstr *MI) {
   const DataLayout &DL = getDataLayout();
   MCTargetStreamer &TS = *OutStreamer->getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
 
+  const MachineFunction &MF = *MI->getParent()->getParent();
+  const ARMSubtarget &STI = MF.getSubtarget<ARMSubtarget>();
+  unsigned FramePtr = STI.useR7AsFramePointer() ? ARM::R7 : ARM::R11;
+
   // If we just ended a constant pool, mark it as such.
   if (InConstantPool && MI->getOpcode() != ARM::CONSTPOOL_ENTRY) {
     OutStreamer->EmitDataRegion(MCDR_DataRegionEnd);
     InConstantPool = false;
   }
 
   // Emit unwinding stuff for frame-related instructions
   if (Subtarget->isTargetEHABICompatible() &&
case ARM::Int_eh_sjlj_longjmp: {
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::LDRi12)
       .addReg(ScratchReg)
       .addReg(SrcReg)
       .addImm(4)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
 
+    if (STI.isTargetDarwin() || STI.isTargetWindows()) {
+      // These platforms always use the same frame register
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::LDRi12)
+        .addReg(FramePtr)
+        .addReg(SrcReg)
+        .addImm(0)
+        // Predicate.
+        .addImm(ARMCC::AL)
+        .addReg(0));
+    } else {
+      // If the calling code might use either R7 or R11 as
+      // frame pointer register, restore it into both.
       EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::LDRi12)
         .addReg(ARM::R7)
         .addReg(SrcReg)
         .addImm(0)
         // Predicate.
         .addImm(ARMCC::AL)
         .addReg(0));
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::LDRi12)
+        .addReg(ARM::R11)
+        .addReg(SrcReg)
+        .addImm(0)
+        // Predicate.
+        .addImm(ARMCC::AL)
+        .addReg(0));
+    }
 
     assert(Subtarget->hasV4TOps());
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::BX)
       .addReg(ScratchReg)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
     return;
case ARM::tInt_eh_sjlj_longjmp: {
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tLDRi)
       .addReg(ScratchReg)
       .addReg(SrcReg)
       .addImm(1)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
 
+    if (STI.isTargetDarwin() || STI.isTargetWindows()) {
+      // These platforms always use the same frame register
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tLDRi)
+        .addReg(FramePtr)
+        .addReg(SrcReg)
+        .addImm(0)
+        // Predicate.
+        .addImm(ARMCC::AL)
+        .addReg(0));
+    } else {
+      // If the calling code might use either R7 or R11 as
+      // frame pointer register, restore it into both.
       EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tLDRi)
         .addReg(ARM::R7)
         .addReg(SrcReg)
         .addImm(0)
         // Predicate.
         .addImm(ARMCC::AL)
         .addReg(0));
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tLDRi)
+        .addReg(ARM::R11)
+        .addReg(SrcReg)
+        .addImm(0)
+        // Predicate.
+        .addImm(ARMCC::AL)
+        .addReg(0));
+    }
 
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tBX)
       .addReg(ScratchReg)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
     return;
   }

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

 ; RUN: llc %s -o - | FileCheck %s
+; RUN: llc -mtriple=armv7-linux -exception-model sjlj %s -o - | FileCheck %s -check-prefix CHECK-LINUX
+; RUN: llc -mtriple=thumbv7-win32 -exception-model sjlj %s -o - | FileCheck %s -check-prefix CHECK-WIN32
 target triple = "armv7-apple-ios"
 
 declare i32 @llvm.eh.sjlj.setjmp(i8*)
 declare void @llvm.eh.sjlj.longjmp(i8*)
 @g = external global i32
 
 declare void @may_throw()
 declare i32 @__gxx_personality_sj0(...)

 ; CHECK-NEXT: add pc, pc, #0
 ; CHECK-NEXT: mov r0, #1
 ;
 ; longjmp sequence:
 ; CHECK: ldr sp, [{{\s*}}[[BUFREG:r[0-9]+]], #8]
 ; CHECK-NEXT: ldr [[DESTREG:r[0-9]+]], {{\[}}[[BUFREG]], #4]
 ; CHECK-NEXT: ldr r7, {{\[}}[[BUFREG]]{{\]}}
 ; CHECK-NEXT: bx [[DESTREG]]
+
+; CHECK-LINUX: ldr sp, [{{\s*}}[[BUFREG:r[0-9]+]], #8]
+; CHECK-LINUX-NEXT: ldr [[DESTREG:r[0-9]+]], {{\[}}[[BUFREG]], #4]
+; CHECK-LINUX-NEXT: ldr r7, {{\[}}[[BUFREG]]{{\]}}
+; CHECK-LINUX-NEXT: ldr r11, {{\[}}[[BUFREG]]{{\]}}
+; CHECK-LINUX-NEXT: bx [[DESTREG]]
+
+; CHECK-WIN32: ldr.w r11, [{{\s*}}[[BUFREG:r[0-9]+]]]
+; CHECK-WIN32-NEXT: ldr.w sp, {{\[}}[[BUFREG]], #8]
+; CHECK-WIN32-NEXT: ldr.w pc, {{\[}}[[BUFREG]], #4]
 define void @foobar() {
 entry:
   %buf = alloca [5 x i8*], align 4
   %arraydecay = getelementptr inbounds [5 x i8*], [5 x i8*]* %buf, i32 0, i32 0
   %bufptr = bitcast i8** %arraydecay to i8*
   ; Note: This is simplified, in reality you have to store the framepointer +
   ; stackpointer in the buffer as well for this to be legal!
   %setjmpres = call i32 @llvm.eh.sjlj.setjmp(i8* %bufptr)