Arm: Don't define a label twice with two setjmps in a function.

[MatzeB]

Summary

Simply do what the comment in ARMAsmPrinter says and use directional labels.

[MatzeB]

ping

[john.brawn]

Simply do what the comment in ARMAsmPrinter says and use directional labels.

Which comment would that be? I had a look in both ARMAsmPrinter.h and ARMAsmPrinter.cpp and could find no mention of directional labels.

[MatzeB]

ping

[rengolin]

I'm a bit lost, too.

The review description is seriously lacking, and the change seems odd.

I'm no expert in SJLJ, so I'd rather someone with more knowledge in it to review, but for that, we'd need a better description and maybe some comments on why your substitution is valid, what it fixes, etc.

cheers,
--renato

[MatzeB]

First to avoid confusion this is not about SJLJ exception handling but just about the implementation of setjmp (which is part of SJLJ exception handling but can also be used independently with __builtin_setjmp() for example).

The code in question implements setjmp, to do it has to: Store the current program counter into the provided jump buffer. The tricky part here is that controlflow can either start at the beginning of the sequence, or we can come from a longjmp call which executed a jump to the stored program counter. Now depending on whether we executed the setjmp first or came from a longjmp invocation we want to return different values. This is why the code is generated in a way that first sets r0 to 0 and then jumps over an instruction that sets r0 to 1, while the PC that is stored earlier is adjust in a way that the PC of the 1-setting instruction is stored. So far for what the code there is supposed to be doing[1]. The comment in the code I referenced explains how the generated instructions should look like:

// Two incoming args: GPR:$src, GPR:$val
 // mov $val, pc
 // adds $val, #7
 // str $val, [$src, #4]
 // movs r0, #0
 // b 1f
 // movs r0, #1
 // 1:

The problem here is that the label for the jump is created by this:

raw_svector_ostream(Name) << DL->getPrivateGlobalPrefix() << "SJLJEH"

which is unique per function, but as soon as someone uses __builtin_setjmp() twice in a function we will have a duplicate label. My patch simply fixes that by using local/directional lables. Local labels are documented here:
https://sourceware.org/binutils/docs-2.25/as/Symbol-Names.html#Symbol-Names
The comment already used the syntax for local labels so that seemed like a natural fix.

[1] Now that I explained that in detail I realize that the existing code is even more broken in that it returns 1 in the longjmp case but should be returning the value loaded from the jmpbuf by the longjmp instruction (the longjmp instruction always loads that value into r7) if longjmp would simply load the value into r0 we could also implement all of this easily without a jump... But my intention here was to fix a simply bug that makes the assembler abort. Fixing the whole setjmp implementation properly is for another patch.

[MatzeB]

ping

[rengolin]

Hi Matthias,

Sorry, this fell out of my radar. Now it's a lot clearer what do you mean by "the comment" and why are you using the directional labels. It looks ok to me, but I'm adding some EH and Darwin folks, as this affects them more than anything.

I just have one question: can you create your temporary labels with some prefix? Like "SJLJEH".1 .2 .3 etc? That'd help when reading the resulting assembly.

cheers,
--renato

[MatzeB]

ping

[rengolin]

Hi Matthias,

I guess people don't feel strongly about this one. I think it's good.

Can you tag the labels with some "SJLJ" text? Just so we know what are they?

Otherwise, LGTM. Thanks!

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

bool isThumb = TT.getArch() == Triple::thumb ||
                    TT.getSubArch() == Triple::ARMSubArch_v6m;
     return isThumb ? ARM::DW_ISA_ARM_thumb : ARM::DW_ISA_ARM_arm;
   }
 
 private:
   MCOperand GetSymbolRef(const MachineOperand &MO, const MCSymbol *Symbol);
   MCSymbol *GetARMJTIPICJumpTableLabel(unsigned uid) const;
 
-  MCSymbol *GetARMSJLJEHLabel() const;
-
   MCSymbol *GetARMGVSymbol(const GlobalValue *GV, unsigned char TargetFlags);
 
 public:
   /// EmitMachineConstantPoolValue - Print a machine constantpool value to
   /// the .s file.
   void EmitMachineConstantPoolValue(MachineConstantPoolValue *MCPV) override;
 };
 } // end namespace llvm
 
 #endif

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

 GetARMJTIPICJumpTableLabel(unsigned uid) const {
   const DataLayout &DL = getDataLayout();
   SmallString<60> Name;
   raw_svector_ostream(Name) << DL.getPrivateGlobalPrefix() << "JTI"
                             << getFunctionNumber() << '_' << uid;
   return OutContext.getOrCreateSymbol(Name);
 }
 
-
-MCSymbol *ARMAsmPrinter::GetARMSJLJEHLabel() const {
-  const DataLayout &DL = getDataLayout();
-  SmallString<60> Name;
-  raw_svector_ostream(Name) << DL.getPrivateGlobalPrefix() << "SJLJEH"
-                            << getFunctionNumber();
-  return OutContext.getOrCreateSymbol(Name);
-}
-
 bool ARMAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNum,
                                     unsigned AsmVariant, const char *ExtraCode,
                                     raw_ostream &O) {
   // Does this asm operand have a single letter operand modifier?
   if (ExtraCode && ExtraCode[0]) {
     if (ExtraCode[1] != 0) return true; // Unknown modifier.
 
     switch (ExtraCode[0]) {
void ARMAsmPrinter::EmitInstruction(const MachineInstr *MI) {
   case ARM::t2Int_eh_sjlj_setjmp:
   case ARM::t2Int_eh_sjlj_setjmp_nofp:
   case ARM::tInt_eh_sjlj_setjmp: {
     // Two incoming args: GPR:$src, GPR:$val
     // mov $val, pc
     // adds $val, #7
     // str $val, [$src, #4]
     // movs r0, #0
-    // b 1f
+    // b LSJLJEH
     // movs r0, #1
-    // 1:
+    // LSJLJEH:
     unsigned SrcReg = MI->getOperand(0).getReg();
     unsigned ValReg = MI->getOperand(1).getReg();
-    MCSymbol *Label = GetARMSJLJEHLabel();
+    MCSymbol *Label = OutContext.createTempSymbol("SJLJEH", false, true);
     OutStreamer->AddComment("eh_setjmp begin");
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tMOVr)
       .addReg(ValReg)
       .addReg(ARM::PC)
       // Predicate.
       .addImm(ARMCC::AL)
       .addReg(0));
 

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

+; RUN: llc %s -o - | FileCheck %s
+target triple = "thumbv7-apple-ios"
+
+declare i32 @llvm.eh.sjlj.setjmp(i8*)
+declare void @llvm.eh.sjlj.longjmp(i8*)
+declare i8* @llvm.frameaddress(i32)
+declare i8* @llvm.stacksave()
+@g = external global i32
+
+; CHECK-LABEL: double_foobar
+;
+; setjmp sequence:
+; CHECK: mov [[PCREG:r[0-9]+]], pc
+; CHECK-NEXT: adds [[PCREG]], [[PCREG]], #7
+; CHECK-NEXT: str [[PCREG]], {{\[}}[[BUFREG:r[0-9]+]], #4]
+; CHECK-NEXT: movs r0, #0
+; CHECK-NEXT: b [[LABEL:L[a-zA-Z0-9]+]]
+; CHECK-NEXT: movs r0, #1
+; CHECK-NEXT: [[LABEL]]:
+;
+; setjmp sequence 2:
+; CHECK: mov [[PCREG:r[0-9]+]], pc
+; CHECK-NEXT: adds [[PCREG]], [[PCREG]], #7
+; CHECK-NEXT: str [[PCREG]], {{\[}}[[BUFREG:r[0-9]+]], #4]
+; CHECK-NEXT: movs r0, #0
+; CHECK-NEXT: b [[LABEL:L[a-zA-Z0-9]+]]
+; CHECK-NEXT: movs r0, #1
+; CHECK-NEXT: [[LABEL]]:
+
+; longjmp sequence:
+; CHECK: ldr [[TEMPREG:r[0-9]+]], [{{\s*}}[[BUFREG:r[0-9]+]], #8]
+; CHECK-NEXT: mov sp, [[TEMPREG]]
+; CHECK-NEXT: ldr [[DESTREG:r[0-9]+]], {{\[}}[[BUFREG]], #4]
+; CHECK-NEXT: ldr r7, {{\[}}[[BUFREG]]{{\]}}
+; CHECK-NEXT: bx [[DESTREG]]
+;
+; longjmp sequence2:
+; CHECK: ldr [[TEMPREG:r[0-9]+]], [{{\s*}}[[BUFREG:r[0-9]+]], #8]
+; CHECK-NEXT: mov sp, [[TEMPREG]]
+; CHECK-NEXT: ldr [[DESTREG:r[0-9]+]], {{\[}}[[BUFREG]], #4]
+; CHECK-NEXT: ldr r7, {{\[}}[[BUFREG]]{{\]}}
+; CHECK-NEXT: bx [[DESTREG]]
+define void @double_foobar() {
+entry:
+  %buf = alloca [5 x i8*], align 4
+  %bufptr = bitcast [5 x i8*]* %buf to i8*
+  %arraydecay = getelementptr inbounds [5 x i8*], [5 x i8*]* %buf, i32 0, i32 0
+
+  %fa = tail call i8* @llvm.frameaddress(i32 0)
+  store i8* %fa, i8** %arraydecay, align 4
+  %ss = tail call i8* @llvm.stacksave()
+  %ssgep = getelementptr [5 x i8*], [5 x i8*]* %buf, i32 0, i32 2
+  store i8* %ss, i8** %ssgep, align 4
+
+  %setjmpres = call i32 @llvm.eh.sjlj.setjmp(i8* %bufptr)
+  %tobool = icmp ne i32 %setjmpres, 0
+  br i1 %tobool, label %if.then, label %if.else
+
+if.then:
+  store volatile i32 1, i32* @g, align 4
+  br label %if.end
+
+if.else:
+  store volatile i32 0, i32* @g, align 4
+  call void @llvm.eh.sjlj.longjmp(i8* %bufptr)
+  unreachable
+
+if.end:
+  %fa2 = tail call i8* @llvm.frameaddress(i32 0)
+  store i8* %fa2, i8** %arraydecay, align 4
+  %ss2 = tail call i8* @llvm.stacksave()
+  store i8* %ss2, i8** %ssgep, align 4
+
+  %setjmpres2 = call i32 @llvm.eh.sjlj.setjmp(i8* %bufptr)
+  %tobool2 = icmp ne i32 %setjmpres2, 0
+  br i1 %tobool2, label %if2.then, label %if2.else
+
+if2.then:
+  store volatile i32 3, i32* @g, align 4
+  br label %if2.end
+
+if2.else:
+  store volatile i32 2, i32* @g, align 4
+  call void @llvm.eh.sjlj.longjmp(i8* %bufptr)
+  unreachable
+
+if2.end:
+  ret void
+}