[ARM] Fix assembly in `tInt_eh_sjlj_longjmp`

[xtkoba]

Summary

On Linux, it was made possible by https://reviews.llvm.org/D38253 that __builtin_longjmp can jump between ARM mode and Thumb mode. However, it has been revealed that it does not work in some cases. This is because in Thumb mode __builtin_longjmp emits an assembly instruction ldr r11, [[BUFREG]] which is misassembled by the integrated assembler, leading to r11 (the frame pointer register in ARM mode) not being restored correctly.

The proposed change uses the instruction mov r11, r7 in place of ldr r11, [[BUFREG]]. Following ldr r7, [[BUFREG]], it should correctly restore r11.

[mstorsjo]

When uploading diffs, please make the diffs with extra context (e.g. git diff -U999).

llvm/lib/Target/ARM/ARMInstrThumb.td

1548

This change looks good and seems to have been requested (and missed by me) in D38253, but we should do the same for Int_eh_sjlj_longjmp in ARMInstrInfo.td too - can you amend the patch to change that too?

[xtkoba]

llvm/lib/Target/ARM/ARMInstrThumb.td

1548

Now I am preparing for another differential to fix pr50202. The requested change will be included therein.

[xtkoba]

llvm/lib/Target/ARM/ARMInstrThumb.td

1548

The requested change is included in D109129.

[mstorsjo]

llvm/lib/Target/ARM/ARMInstrThumb.td

1548

Now I am preparing for another differential to fix pr50202. The requested change will be included therein.

1548

Now I am preparing for another differential to fix pr50202. The requested change will be included therein.

The requested change is included in D109129.

Thanks!

Are you still planning on a different fix than this one for PR50202, or does this one still stand wrt that? (Does this one need similar changes as in D109129, to use a different GPR register class that doesn't include the FP?)

[xtkoba]

llvm/lib/Target/ARM/ARMInstrThumb.td

1548

Strictly, tInt_eh_sjlj_longjmp should also use a new register class, say tGPRnofp, that excludes r7. What differentiates Thumb case from ARM case is that in the former users can workaround r7 from being used as the scratch register in virtue of the compiler option -fno-omit-frame-pointer, because r7 is the sole potential candidate of frame pointer register in Thumb mode.

I will create another differential if need be, but I am not going to include it here or in D109129. These three should be considered as separate problems.

[kristof.beyls]

I'm afraid that I'm not fully up to speed on the code changes here.
That being said, I hope it's useful to point out that I expect that in the not-too-distant future the recent specification of r11 being the frame pointer for both Arm and Thumb to be implemented in LLVM. See https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst (search for "frame pointer") to see the specification. A frame pointer was only recently added to the ABI specification. Before there was no definition of frame pointer for 32-bit ARM in the ABI specification.

I guess that when the new frame pointer specification gets implemented (both Thumb and Arm use r11 as frame pointer), some of the code in the patch may need to be adapted?

[xtkoba]

Diff context extended

[mstorsjo]

I'd say this looks ok to me.

[mstorsjo]

@xtkoba Do you have commit access for pushing this patch, or do you want me to do it? In the latter case, can you provide your preferred git author line, i.e. Real Name <email@address>?

[brad]

In D109041#3001484, @mstorsjo wrote:

@xtkoba Do you have commit access for pushing this patch, or do you want me to do it? In the latter case, can you provide your preferred git author line, i.e. Real Name <email@address>?

I don't see any commits from him. I just commited something for him and used this for author..

Tee KOBAYASHI <[e-mail]>

llvm/lib/Target/ARM/ARMAsmPrinter.cpp

         // Predicate.
         .addImm(ARMCC::AL)
         .addReg(0));
-      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tLDRi)
+      EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::tMOVr)
         .addReg(ARM::R11)
-        .addReg(SrcReg)
-        .addImm(0)
+        .addReg(ARM::R7)
         // Predicate.
         .addImm(ARMCC::AL)
         .addReg(0));

llvm/lib/Target/ARM/ARMInstrThumb.td

 
 // FIXME: Non-IOS version(s)
 let isBarrier = 1, hasSideEffects = 1, isTerminator = 1, isCodeGenOnly = 1,
-    Defs = [ R7, LR, SP ] in
+    Defs = [ R7, R11, LR, SP ] in
 def tInt_eh_sjlj_longjmp : XI<(outs), (ins tGPR:$src, tGPR:$scratch),
                               AddrModeNone, 0, IndexModeNone,
                               Pseudo, NoItinerary, "", "",

llvm/test/CodeGen/ARM/setjmp_longjmp.ll

 ; RUN: llc -simplifycfg-require-and-preserve-domtree=1 %s -o - | FileCheck %s
 ; RUN: llc -mtriple=armv7-linux -exception-model sjlj -simplifycfg-require-and-preserve-domtree=1 %s -o - | FileCheck %s -check-prefix CHECK-LINUX
+; RUN: llc -mtriple=thumbv7-linux -exception-model sjlj -simplifycfg-require-and-preserve-domtree=1 %s -o - | FileCheck %s -check-prefix CHECK-LINUX-THUMB2
 ; RUN: llc -mtriple=thumbv7-win32 -exception-model sjlj -simplifycfg-require-and-preserve-domtree=1 %s -o - | FileCheck %s -check-prefix CHECK-WIN32
 target triple = "armv7-apple-ios"
 

 ; CHECK-LINUX-NEXT: ldr r11, {{\[}}[[BUFREG]]{{\]}}
 ; CHECK-LINUX-NEXT: bx [[DESTREG]]
 
+; CHECK-LINUX-THUMB2: ldr [[DESTREG:r[0-9]+]], [{{\s*}}[[BUFREG:r[0-9]+]], #8]
+; CHECK-LINUX-THUMB2-NEXT: mov sp, [[DESTREG]]
+; CHECK-LINUX-THUMB2-NEXT: ldr [[DESTREG]], {{\[}}[[BUFREG]], #4]
+; CHECK-LINUX-THUMB2-NEXT: ldr r7, {{\[}}[[BUFREG]]{{\]}}
+; CHECK-LINUX-THUMB2-NEXT: mov r11, r7
+; CHECK-LINUX-THUMB2-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]