[X86] -fpatchable-function-entry=N,0: place patch label after ENDBR{32,64}

[MaskRay]

Summary

Similar to D73680 (AArch64 BTI).

A local linkage function whose address is not taken does not need ENDBR32/ENDBR64. Placing the patch label after ENDBR32/ENDBR64 has the advantage that code does not need to differentiate whether the function has an initial ENDBR.

Also, add 32-bit tests and test that .cfi_startproc is at the function
entry. The line information has a general implementation and is tested
by AArch64/patchable-function-entry-empty.mir

[merge_guards_bot]

Unit tests: fail. 62356 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[MaskRay]

Improve tests

[merge_guards_bot]

Unit tests: fail. 62356 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[nickdesaulniers]

llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll

67

I think @f1i is missing the function attribute "branch-target-enforcement"?

[MaskRay]

Delete "branch-target-enforcement" added by mistake

[MaskRay]

llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll

67

x86 uses a module flag "cf-protection-branch", instead of a function attribute.

Deleted the function attribute "branch-target-enforcement" attached to f0.

[merge_guards_bot]

Unit tests: pass. 62371 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[MaskRay]

@nickdesaulniers :)

[nickdesaulniers]

Thanks for the patch. This looks more consistent with aarch64 now. + some x86 folks in cases there's more to endbr{32|64} than meets the eye.

llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll

69

Do you want to add a CHECK-NEXT that the .cfi_startproc is placed after this CHECK?

[MaskRay]

Add CHECK-NEXT: .cfi_startproc

[merge_guards_bot]

Unit tests: fail. 62298 tests passed, 121 failed and 845 were skipped.

failed: Clang.CXX/modules-ts/basic/basic_link/p3.cppm
failed: Clang.CXX/modules-ts/codegen-basics.cppm
failed: Clang.CodeGen/arm-aapcs-vfp.c
failed: Clang.CodeGen/attr-cpuspecific.c
failed: Clang.CodeGen/attr-target-mv-func-ptrs.c
failed: Clang.CodeGen/attr-target-mv-va-args.c
failed: Clang.CodeGenCUDA/address-spaces.cu
failed: Clang.CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
failed: Clang.CodeGenCUDA/amdgpu-kernel-attrs.cu
failed: Clang.CodeGenCUDA/amdgpu-visibility.cu
failed: Clang.CodeGenCUDA/convergent.cu
failed: Clang.CodeGenCUDA/cuda-builtin-vars.cu
failed: Clang.CodeGenCUDA/device-stub.cu
failed: Clang.CodeGenCUDA/device-var-init.cu
failed: Clang.CodeGenCUDA/device-vtable.cu
failed: Clang.CodeGenCUDA/function-overload.cu
failed: Clang.CodeGenCUDA/kernel-amdgcn.cu
failed: Clang.CodeGenCUDA/kernel-args.cu
failed: Clang.CodeGenCUDA/library-builtin.cu
failed: Clang.CodeGenCUDA/link-device-bitcode.cu
failed: Clang.CodeGenCUDA/nothrow.cu
failed: Clang.CodeGenCUDA/propagate-metadata.cu
failed: Clang.CodeGenCUDA/ptx-kernels.cu
failed: Clang.CodeGenCUDA/types.cu
failed: Clang.CodeGenCUDA/usual-deallocators.cu
failed: Clang.CodeGenCXX/attr-exclude_from_explicit_instantiation.dont_assume_extern_instantiation.cpp
failed: Clang.CodeGenCXX/attr-target-mv-diff-ns.cpp
failed: Clang.CodeGenCXX/attr-target-mv-member-funcs.cpp
failed: Clang.CodeGenCXX/attr-target-mv-out-of-line-defs.cpp
failed: Clang.CodeGenCXX/attr-target-mv-overloads.cpp
failed: Clang.CodeGenCXX/attr.cpp
failed: Clang.CodeGenCXX/builtin-source-location.cpp
failed: Clang.CodeGenCXX/conditional-temporaries.cpp
failed: Clang.CodeGenCXX/const-init-cxx2a.cpp
failed: Clang.CodeGenCXX/ctor-dtor-alias.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist-pr12086.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist-startend.cpp
failed: Clang.CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp
failed: Clang.CodeGenCXX/cxx11-extern-constexpr.cpp
failed: Clang.CodeGenCXX/cxx11-initializer-aggregate.cpp
failed: Clang.CodeGenCXX/cxx11-thread-local-reference.cpp
failed: Clang.CodeGenCXX/cxx11-thread-local-visibility.cpp
failed: Clang.CodeGenCXX/cxx1y-variable-template-linkage.cpp
failed: Clang.CodeGenCXX/cxx1z-inline-variables.cpp
failed: Clang.CodeGenCXX/explicit-instantiation.cpp
failed: Clang.CodeGenCXX/inheriting-constructor.cpp
failed: Clang.CodeGenCXX/member-function-pointers.cpp
failed: Clang.CodeGenCXX/microsoft-uuidof.cpp
failed: Clang.CodeGenCXX/no-odr-use.cpp
failed: Clang.CodeGenCXX/pragma-visibility.cpp
failed: Clang.CodeGenCXX/regcall.cpp
failed: Clang.CodeGenCXX/static-data-member.cpp
failed: Clang.CodeGenCXX/static-init.cpp
failed: Clang.CodeGenCXX/static-member-variable-explicit-specialization.cpp
failed: Clang.CodeGenCXX/visibility-inlines-hidden-staticvar.cpp
failed: Clang.CodeGenCXX/vtable-linkage.cpp
failed: Clang.CodeGenCXX/x86_64-arguments.cpp
failed: Clang.CodeGenCoroutines/coro-await-resume-eh.cpp
failed: Clang.CodeGenCoroutines/coro-await.cpp
failed: Clang.CodeGenCoroutines/coro-cleanup.cpp
failed: Clang.CodeGenCoroutines/coro-gro-nrvo.cpp
failed: Clang.CodeGenCoroutines/coro-params.cpp
failed: Clang.CodeGenCoroutines/coro-ret-void.cpp
failed: Clang.CodeGenObjC/assign.m
failed: Clang.CodeGenObjC/constant-strings.m
failed: Clang.CodeGenObjC/gnu-exceptions.m
failed: Clang.CodeGenObjC/gnustep2-proto.m
failed: Clang.CodeGenObjC/objfw.m
failed: Clang.CodeGenObjC/property.m
failed: Clang.CodeGenObjC/stret_lookup.m
failed: Clang.CodeGenObjCXX/designated-initializers.mm
failed: Clang.CodeGenObjCXX/objfw-exceptions.mm
failed: Clang.CodeGenOpenCL/addr-space-struct-arg.cl
failed: Clang.CodeGenOpenCL/address-spaces-conversions.cl
failed: Clang.CodeGenOpenCL/amdgcn-automatic-variable.cl
failed: Clang.CodeGenOpenCL/amdgcn-large-globals.cl
failed: Clang.CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
failed: Clang.CodeGenOpenCL/amdgpu-attrs.cl
failed: Clang.CodeGenOpenCL/amdgpu-call-kernel.cl
failed: Clang.CodeGenOpenCL/amdgpu-calling-conv.cl
failed: Clang.CodeGenOpenCL/amdgpu-enqueue-kernel.cl
failed: Clang.CodeGenOpenCL/amdgpu-nullptr.cl
failed: Clang.CodeGenOpenCL/as_type.cl
failed: Clang.CodeGenOpenCL/bool_cast.cl
failed: Clang.CodeGenOpenCL/cl20-device-side-enqueue.cl
failed: Clang.CodeGenOpenCL/constant-addr-space-globals.cl
failed: Clang.CodeGenOpenCL/convergent.cl
failed: Clang.CodeGenOpenCL/extension-begin.cl
failed: Clang.CodeGenOpenCL/kernel-arg-info.cl
failed: Clang.CodeGenOpenCL/kernels-have-spir-cc-by-default.cl
failed: Clang.CodeGenOpenCL/partial_initializer.cl
failed: Clang.CodeGenOpenCL/pipe_types.cl
failed: Clang.CodeGenOpenCL/ptx-calls.cl
failed: Clang.CodeGenOpenCL/ptx-kernels.cl
failed: Clang.CodeGenOpenCL/sampler.cl
failed: Clang.CodeGenOpenCL/shifts.cl
failed: Clang.CodeGenOpenCL/spir-calling-conv.cl
failed: Clang.CodeGenOpenCL/vectorLoadStore.cl
failed: Clang.CodeGenOpenCL/visibility.cl
failed: Clang.CodeGenOpenCL/vla.cl
failed: Clang.CodeGenOpenCLCXX/address-space-deduction.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-derived-base.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-operators.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-references.cl
failed: Clang.CodeGenOpenCLCXX/addrspace-with-class.cl
failed: Clang.CodeGenOpenCLCXX/constexpr.cl
failed: Clang.Modules/codegen-flags.test
failed: Clang.Modules/codegen-opt.test
failed: Clang.Modules/codegen.test
failed: Clang.Modules/initializers.cpp
failed: Clang.Modules/templates-2.mm
failed: Clang.Modules/templates.mm
failed: Clang.OpenMP/allocate_codegen.cpp
failed: Clang.OpenMP/declare_variant_mixed_codegen.c
failed: Clang.OpenMP/parallel_codegen.cpp
failed: Clang.OpenMP/parallel_master_codegen.cpp
failed: Clang.OpenMP/simd_metadata.c
failed: Clang.OpenMP/threadprivate_codegen.cpp
failed: Clang.PCH/codegen.cpp
failed: Clang.Parser/pragma-visibility2.c
failed: Clang.Profile/cxx-abc-deleting-dtor.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[MaskRay]

Looks good?

llvm/lib/Target/X86/X86MCInstLower.cpp

void X86AsmPrinter::EmitInstruction(const MachineInstr *MI) {
   }
 
   case X86::CATCHRET: {
     // Lower these as normal, but add some comments.
     OutStreamer->AddComment("CATCHRET");
     break;
   }
 
+  case X86::ENDBR32:
+  case X86::ENDBR64: {
+    // CurrentPatchableFunctionEntrySym can be CurrentFnBegin only for
+    // -fpatchable-function-entry=N,0. The entry MBB is guaranteed to be
+    // non-empty. If MI is the initial ENDBR, place the
+    // __patchable_function_entries label after ENDBR.
+    if (CurrentPatchableFunctionEntrySym &&
+        CurrentPatchableFunctionEntrySym == CurrentFnBegin &&
+        MI == &MF->front().front()) {
+      MCInst Inst;
+      MCInstLowering.Lower(MI, Inst);
+      EmitAndCountInstruction(Inst);
+      CurrentPatchableFunctionEntrySym = createTempSymbol("patch");
+      OutStreamer->EmitLabel(CurrentPatchableFunctionEntrySym);
+      return;
+    }
+    break;
+  }
+
   case X86::TAILJMPr:
   case X86::TAILJMPm:
   case X86::TAILJMPd:
   case X86::TAILJMPd_CC:
   case X86::TAILJMPr64:
   case X86::TAILJMPm64:
   case X86::TAILJMPd64:
   case X86::TAILJMPd64_CC:

llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll

; CHECK-NOT:  .section __patchable_function_entries
   ret void
 }
 
 ;; -fpatchable-function-entry=1 -mbranch-protection=bti
 ;; For M=0, place the label .Lpatch0 after the initial BTI.
 define void @f1() "patchable-function-entry"="1" "branch-target-enforcement" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
-; CHECK:      // %bb.0:
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: // %bb.0:
 ; CHECK-NEXT:  hint #34
 ; CHECK-NEXT: .Lpatch0:
 ; CHECK-NEXT:  nop
 ; CHECK-NEXT:  ret
 ; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
 ; CHECK-NEXT: .p2align 3
 ; CHECK-NEXT: .xword .Lpatch0
   ret void
 }
 
 ;; -fpatchable-function-entry=2,1 -mbranch-protection=bti
 define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" "branch-target-enforcement" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
 ; CHECK-NEXT: f2_1:
 ; CHECK-NEXT: .Lfunc_begin2:
-; CHECK:      // %bb.0:
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: // %bb.0:
 ; CHECK-NEXT:  hint #34
 ; CHECK-NEXT:  nop
 ; CHECK-NEXT:  ret
 ; CHECK:      .Lfunc_end2:
 ; CHECK-NEXT: .size f2_1, .Lfunc_end2-f2_1
 ; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
 ; CHECK-NEXT: .p2align 3
 ; CHECK-NEXT: .xword .Ltmp0

llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll

-; RUN: llc -mtriple=x86_64 %s -o - | FileCheck --check-prefixes=CHECK %s
+; RUN: llc -mtriple=i686 %s -o - | FileCheck --check-prefixes=CHECK,32 %s
+; RUN: llc -mtriple=x86_64 %s -o - | FileCheck --check-prefixes=CHECK,64 %s
 
 ;; -fpatchable-function-entry=0 -fcf-protection=branch
-define void @f0() "patchable-function-entry"="0" "branch-target-enforcement" {
+define void @f0() "patchable-function-entry"="0" {
 ; CHECK-LABEL: f0:
 ; CHECK-NEXT: .Lfunc_begin0:
-; CHECK:      # %bb.0:
-; CHECK-NEXT:  endbr64
-; CHECK-NEXT:  retq
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: # %bb.0:
+; 32-NEXT:     endbr32
+; 64-NEXT:     endbr64
+; CHECK-NEXT:  ret
 ; CHECK-NOT:  .section __patchable_function_entries
   ret void
 }
 
 ;; -fpatchable-function-entry=1 -fcf-protection=branch
+;; For M=0, place the label .Lpatch0 after the initial ENDBR.
+;; .cfi_startproc should be placed at the function entry.
 define void @f1() "patchable-function-entry"="1" {
 ; CHECK-LABEL: f1:
 ; CHECK-NEXT: .Lfunc_begin1:
-; CHECK:       endbr64
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: # %bb.0:
+; 32-NEXT:     endbr32
+; 64-NEXT:     endbr64
+; CHECK-NEXT: .Lpatch0:
 ; CHECK-NEXT:  nop
-; CHECK-NEXT:  retq
+; CHECK-NEXT:  ret
 ; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
-; CHECK-NEXT: .p2align 3
-; CHECK-NEXT: .quad .Lfunc_begin1
+; 32-NEXT:    .p2align 2
+; 32-NEXT:    .long .Lpatch0
+; 64-NEXT:    .p2align 3
+; 64-NEXT:    .quad .Lpatch0
   ret void
 }
 
 ;; -fpatchable-function-entry=2,1 -fcf-protection=branch
 define void @f2_1() "patchable-function-entry"="1" "patchable-function-prefix"="1" {
 ; CHECK-LABEL: .type f2_1,@function
 ; CHECK-NEXT: .Ltmp0:
 ; CHECK-NEXT:  nop
 ; CHECK-NEXT: f2_1:
 ; CHECK-NEXT: .Lfunc_begin2:
-; CHECK:      # %bb.0:
-; CHECK-NEXT:  endbr64
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: # %bb.0:
+; 32-NEXT:     endbr32
+; 64-NEXT:     endbr64
 ; CHECK-NEXT:  nop
-; CHECK-NEXT:  retq
+; CHECK-NEXT:  ret
 ; CHECK:      .Lfunc_end2:
 ; CHECK-NEXT: .size f2_1, .Lfunc_end2-f2_1
 ; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
-; CHECK-NEXT: .p2align 3
-; CHECK-NEXT: .quad .Ltmp0
+; 32-NEXT:    .p2align 2
+; 32-NEXT:    .long .Ltmp0
+; 64-NEXT:    .p2align 3
+; 64-NEXT:    .quad .Ltmp0
+  ret void
+}
+
+;; -fpatchable-function-entry=1 -fcf-protection=branch
+;; For M=0, don't create .Lpatch0 if the initial instruction is not ENDBR,
+;; even if other basic blocks may have ENDBR.
+@buf = internal global [5 x i8*] zeroinitializer
+declare i32 @llvm.eh.sjlj.setjmp(i8*)
+
+define internal void @f1i() "patchable-function-entry"="1" {
+; CHECK-LABEL: f1i:
+; CHECK-NEXT: .Lfunc_begin3:
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: # %bb.0:
+; CHECK-NEXT:  nop
+; CHECK-NOT:  .Lpatch0:
+;; Another basic block has ENDBR, but it doesn't affect our decision to not create .Lpatch0
+; CHECK:       endbr
+; CHECK:      .section __patchable_function_entries,"awo",@progbits,f1,unique,0
+; 32-NEXT:    .p2align 2
+; 32-NEXT:    .long .Lfunc_begin3
+; 64-NEXT:    .p2align 3
+; 64-NEXT:    .quad .Lfunc_begin3
+entry:
+  tail call i32 @llvm.eh.sjlj.setjmp(i8* bitcast ([5 x i8*]* @buf to i8*))
   ret void
 }
 
 !llvm.module.flags = !{!0}
 
 !0 = !{i32 4, !"cf-protection-branch", i32 1}