[PATCH] D144082: [llvm][AArch64] Fix BTI after returns_twice when call has no attributes

[David Spickett via Phabricator via llvm-commits]

DavidSpickett created this revision.
Herald added subscribers: hiraditya, kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Previously we were looking for the returns twice attribute by manually
getting the function attributes from the call. This meant that we only
found attributes on the call itself, not what it was calling.

So if you had:
%call1 = call i32 @setjmp(ptr noundef null)

We would not BTI protect that even though setjmp clearly needs it.

Clang happens to produce:
%call = call i32 @setjmp(ptr noundef null) #0 ; returns_twice

So all valid calls were protected. This is not guaranteed,
the frontend may choose not to put attributes on the call.

It is undefined behaviour to call setjmp indirectly
(https://pubs.opengroup.org/onlinepubs/9699919799/functions/setjmp.html)
but as I misused the APIs here I think it's worth fixing up regardless.

Added comments to the test file where the IR differs from what
clang would output.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144082

Files:
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
  llvm/test/CodeGen/AArch64/setjmp-bti.ll


Index: llvm/test/CodeGen/AArch64/setjmp-bti.ll
===================================================================
--- llvm/test/CodeGen/AArch64/setjmp-bti.ll
+++ llvm/test/CodeGen/AArch64/setjmp-bti.ll
@@ -18,8 +18,9 @@
 ;
 ; void bbb(void) {
 ;   setjmp(0);
+;   setjmp(0); // With the attributes removed.
 ;   int (*fnptr)(ptr) = setjmp;
-;   fnptr(0);
+;   fnptr(0); // With attributes added.
 ;   notsetjmp();
 ; }
 
@@ -27,6 +28,8 @@
 ; BTI-LABEL: bbb:
 ; BTI:       bl setjmp
 ; BTI-NEXT:  hint #36
+; BTI:       bl setjmp
+; BTI-NEXT:  hint #36
 ; BTI:       blr x{{[0-9]+}}
 ; BTI-NEXT:  hint #36
 ; BTI:       bl notsetjmp
@@ -35,6 +38,8 @@
 ; BTISLS-LABEL: bbb:
 ; BTISLS:       bl setjmp
 ; BTISLS-NEXT:  hint #36
+; BTISLS:       bl setjmp
+; BTISLS-NEXT:  hint #36
 ; BTISLS:       bl __llvm_slsblr_thunk_x{{[0-9]+}}
 ; BTISLS-NEXT:  hint #36
 ; BTISLS:       bl notsetjmp
@@ -43,16 +48,22 @@
 ; NOBTI-LABEL: bbb:
 ; NOBTI:     bl setjmp
 ; NOBTI-NOT: hint #36
+; NOBTI:     bl setjmp
+; NOBTI-NOT: hint #36
 ; NOBTI:     blr x{{[0-9]+}}
 ; NOBTI-NOT: hint #36
 ; NOBTI:     bl notsetjmp
 ; NOBTI-NOT: hint #36
 entry:
   %fnptr = alloca ptr, align 8
+  ; The frontend may apply attributes to the call, but it doesn't have to. We
+  ; should be looking at the call base, which looks past that to the called function.
   %call = call i32 @setjmp(ptr noundef null) #0
+  %call1 = call i32 @setjmp(ptr noundef null)
   store ptr @setjmp, ptr %fnptr, align 8
   %0 = load ptr, ptr %fnptr, align 8
-  %call1 = call i32 %0(ptr noundef null) #0
+  ; Clang does not attach the attribute here but if it did, it should work.
+  %call2 = call i32 %0(ptr noundef null) #0
   call void @notsetjmp()
   ret void
 }
Index: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
===================================================================
--- llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
+++ llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp
@@ -1266,8 +1266,7 @@
     Opc = AArch64::BLR_RVMARKER;
   // A call to a returns twice function like setjmp must be followed by a bti
   // instruction.
-  else if (Info.CB &&
-           Info.CB->getAttributes().hasFnAttr(Attribute::ReturnsTwice) &&
+  else if (Info.CB && Info.CB->hasFnAttr(Attribute::ReturnsTwice) &&
            !Subtarget.noBTIAtReturnTwice() &&
            MF.getInfo<AArch64FunctionInfo>()->branchTargetEnforcement())
     Opc = AArch64::BLR_BTI;
Index: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
===================================================================
--- llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -7173,7 +7173,7 @@
   bool IsSibCall = false;
   bool GuardWithBTI = false;
 
-  if (CLI.CB && CLI.CB->getAttributes().hasFnAttr(Attribute::ReturnsTwice) &&
+  if (CLI.CB && CLI.CB->hasFnAttr(Attribute::ReturnsTwice) &&
       !Subtarget->noBTIAtReturnTwice()) {
     GuardWithBTI = FuncInfo->branchTargetEnforcement();
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144082.497592.patch
Type: text/x-patch
Size: 2987 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230215/407179ea/attachment.bin>