This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix assembly in `tInt_eh_sjlj_longjmp`
AcceptedPublic

Authored by xtkoba on Aug 31 2021, 10:21 PM.

Details

Reviewers
efriedma
mstorsjo
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.

Diff Detail

Event Timeline

xtkoba created this revision.Aug 31 2021, 10:21 PM
xtkoba requested review of this revision.Aug 31 2021, 10:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 10:21 PM
mstorsjo added a subscriber: 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 added inline comments.Sep 1 2021, 12:58 AM
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 added inline comments.Sep 1 2021, 9:33 PM
llvm/lib/Target/ARM/ARMInstrThumb.td
1548

The requested change is included in D109129.

mstorsjo added inline comments.Sep 1 2021, 11:06 PM
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 added inline comments.Sep 3 2021, 3:34 AM
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.

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 updated this revision to Diff 370714.Sep 4 2021, 1:57 AM

Diff context extended

mstorsjo accepted this revision.Sep 14 2021, 1:51 PM

I'd say this looks ok to me.

This revision is now accepted and ready to land.Sep 14 2021, 1:51 PM

@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 added a subscriber: brad.Dec 3 2021, 8:48 PM

@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 <xtkoba@gmail.com>