[CodeGen] -fsanitize={function,kcfi}: ensure align 4 if +strict-align

[MaskRay]

Summary

Fix https://github.com/llvm/llvm-project/issues/63579

% cat a.c
void foo() {}
% clang --target=arm-none-eabi -mthumb -mno-unaligned-access -fsanitize=kcfi a.c -S -o - | grep p2align
        .p2align        1
% clang --target=armv6m-none-eabi -fsanitize=function a.c -S -o - | grep p2align
        .p2align        1

With -mno-unaligned-access (possibly implicit), we should ensure that
-fsanitize={function,kcfi} instrumented functions are aligned by at least 4, so
that loading the type hash before the function label will not cause a misaligned
access, even if the backend doesn't set setMinFunctionAlignment to 4 or greater.

With this patch, the generated assembly for the examples above will contain .p2align 2.

If -falign-functions= is specified, take the maxiumum.

If __attribute__((aligned(2))) is specified, arbitrarily let the function
attribute win.

Since SanOpts is per-function, move the alignment setting code from
CodeGenModule::SetLLVMFunctionAttributesForDefinition to CodeGenFunction.
This move requires some attention.

Note: CodeGenModule::SetLLVMFunctionAttributesForDefinition is called by many
thunk codegen code with a dummy GlobalDecl/FunctionDecl.
However, in one call site, MicrosoftCXXABI::EmitVirtualMemPtrThunk has a
SetLLVMFunctionAttributesForDefinition use case that requires the
"Some C++ ABIs require 2-byte alignment for member functions" code. So
keep this part in CodeGenModule.

[simon_tatham]

The details of this approach look good to me, but is this the best place to solve it? Doing it in clang means that every language front end that wants to use either of these sanitizers is responsible for doing this same work: tagging every IR function with align 4 if it also has !kcfi_type or !func_sanitize, and perhaps also checking the target-features to decide whether to do that.

I'd imagined the problem being solved at a lower level, when converting the IR into actual function prologues, so that all front ends generating IR would benefit from the fix.

[efriedma]

I also think it makes sense to fix the alignment when we lower the metadata, not in the frontend, unless I'm missing something.

It's not clear to me how "strict-align" is relevant; if sanitizer lowering is generating "align 4" loads, the relevant pointers need to be appropriately aligned regardless of the cost of unaligned loads. Misaligned loads are undefined behavior in LLVM IR on all targets. (32-bit ARM in particular has cases where 32-bit unaligned loads are supported, but certain load instruction variations enforce alignment.)

[MaskRay]

In D154043#4459446, @simon_tatham wrote:

The details of this approach look good to me, but is this the best place to solve it? Doing it in clang means that every language front end that wants to use either of these sanitizers is responsible for doing this same work: tagging every IR function with align 4 if it also has !kcfi_type or !func_sanitize, and perhaps also checking the target-features to decide whether to do that.

I'd imagined the problem being solved at a lower level, when converting the IR into actual function prologues, so that all front ends generating IR would benefit from the fix.

In D154043#4460665, @efriedma wrote:

I also think it makes sense to fix the alignment when we lower the metadata, not in the frontend, unless I'm missing something.

It's not clear to me how "strict-align" is relevant; if sanitizer lowering is generating "align 4" loads, the relevant pointers need to be appropriately aligned regardless of the cost of unaligned loads. Misaligned loads are undefined behavior in LLVM IR on all targets. (32-bit ARM in particular has cases where 32-bit unaligned loads are supported, but certain load instruction variations enforce alignment.)

OK. See D154125 for the MachineFunction.cpp approach. If we go that direction, I'll abandon this patch.

[MaskRay]

Obsoleted by D154125

clang/lib/CodeGen/CodeGenFunction.cpp

#undef SANITIZER
   // Ignore null checks in coroutine functions since the coroutines passes
   // are not aware of how to move the extra UBSan instructions across the split
   // coroutine boundaries.
   if (D && SanOpts.has(SanitizerKind::Null))
     if (FD && FD->getBody() &&
         FD->getBody()->getStmtClass() == Stmt::CoroutineBodyStmtClass)
       SanOpts.Mask &= ~SanitizerKind::Null;
 
+  if (FD && FD->hasAttr<AlignedAttr>()) {
+    if (unsigned alignment =
+            FD->getMaxAlignment() / getContext().getCharWidth())
+      Fn->setAlignment(llvm::Align(alignment));
+  } else if (FD) {
+    if (getLangOpts().FunctionAlignment)
+      Fn->setAlignment(llvm::Align(1ull << getLangOpts().FunctionAlignment));
+    // -fsanitize=function and -fsanitize=kcfi instrument indirect function
+    // calls to load a type hash before the function label. Ensure the function
+    // is aligned by a least 4 to avoid unaligned access for
+    // -mno-unaligned-access, even if the backend does not increase the
+    // alignment.
+    if (Fn->getAlignment() < 4 && (SanOpts.has(SanitizerKind::Function) ||
+                                   SanOpts.has(SanitizerKind::KCFI))) {
+      llvm::StringMap<bool> FeatureMap;
+      getContext().getFunctionFeatureMap(FeatureMap, GD);
+      if (FeatureMap.lookup("strict-align"))
+        Fn->setAlignment(llvm::Align(4));
+    }
+  }
+
   // Apply xray attributes to the function (as a string, for now)
   bool AlwaysXRayAttr = false;
   if (const auto *XRayAttr = D ? D->getAttr<XRayInstrumentAttr>() : nullptr) {
     if (CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
             XRayInstrKind::FunctionEntry) ||
         CGM.getCodeGenOpts().XRayInstrumentationBundle.has(
             XRayInstrKind::FunctionExit)) {
       if (XRayAttr->alwaysXRayInstrument() && ShouldXRayInstrumentFunction()) {

clang/lib/CodeGen/CodeGenModule.cpp

if (!D->hasAttr<OptimizeNoneAttr>()) {
     if (D->hasAttr<HotAttr>())
       B.addAttribute(llvm::Attribute::Hot);
     if (D->hasAttr<MinSizeAttr>())
       B.addAttribute(llvm::Attribute::MinSize);
   }
 
   F->addFnAttrs(B);
 
-  unsigned alignment = D->getMaxAlignment() / Context.getCharWidth();
-  if (alignment)
-    F->setAlignment(llvm::Align(alignment));
-
-  if (!D->hasAttr<AlignedAttr>())
-    if (LangOpts.FunctionAlignment)
-      F->setAlignment(llvm::Align(1ull << LangOpts.FunctionAlignment));
-
   // Some C++ ABIs require 2-byte alignment for member functions, in order to
   // reserve a bit for differentiating between virtual and non-virtual member
   // functions. If the current target's C++ ABI requires this and this is a
   // member function, set its alignment accordingly.
   if (getTarget().getCXXABI().areMemberFunctionsAligned()) {
     if (F->getAlignment() < 2 && isa<CXXMethodDecl>(D))
       F->setAlignment(llvm::Align(2));
   }

clang/test/CodeGen/kcfi.c

-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -x c++ -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -fpatchable-function-entry-offset=3 -o - %s | FileCheck %s --check-prefixes=CHECK,OFFSET
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -o - %s | FileCheck %s --check-prefixes=CHECK,BUNDLE
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -x c++ -o - %s | FileCheck %s --check-prefixes=CHECK,BUNDLE
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=kcfi -fpatchable-function-entry-offset=3 -o - %s | FileCheck %s --check-prefixes=CHECK,BUNDLE,OFFSET
+// RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -fsanitize=kcfi -target-feature -strict-align -o - %s | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -fsanitize=kcfi -target-feature +strict-align -o - %s | FileCheck %s --check-prefix=STRICTALIGN
 #if !__has_feature(kcfi)
 #error Missing kcfi?
 #endif
 
 /// Must emit __kcfi_typeid symbols for address-taken function declarations
 // CHECK: module asm ".weak __kcfi_typeid_[[F4:[a-zA-Z0-9_]+]]"
 // CHECK: module asm ".set __kcfi_typeid_[[F4]], [[#%d,HASH:]]"
 /// Must not __kcfi_typeid symbols for non-address-taken declarations
 // CHECK-NOT: module asm ".weak __kcfi_typeid_{{f6|_Z2f6v}}"
 typedef int (*fn_t)(void);
 
 // CHECK: define dso_local{{.*}} i32 @{{f1|_Z2f1v}}(){{.*}} !kcfi_type ![[#TYPE:]]
+// STRICTALIGN: define{{.*}} i32 @f1() #[[#]] align 4 !kcfi_type ![[#TYPE:]]
 int f1(void) { return 0; }
 
 // CHECK: define dso_local{{.*}} i32 @{{f2|_Z2f2v}}(){{.*}} !kcfi_type ![[#TYPE2:]]
 unsigned int f2(void) { return 2; }
 
 // CHECK-LABEL: define dso_local{{.*}} i32 @{{__call|_Z6__callPFivE}}(ptr{{.*}} %f)
 int __call(fn_t f) __attribute__((__no_sanitize__("kcfi"))) {
   // CHECK-NOT: call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"
   return f();
 }
 
 // CHECK: define dso_local{{.*}} i32 @{{call|_Z4callPFivE}}(ptr{{.*}} %f){{.*}}
 int call(fn_t f) {
-  // CHECK: call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#HASH]]) ]
+  // BUNDLE: call{{.*}} i32 %{{.}}(){{.*}} [ "kcfi"(i32 [[#HASH]]) ]
   return f();
 }
 
 // CHECK-DAG: define internal{{.*}} i32 @{{f3|_ZL2f3v}}(){{.*}} !kcfi_type ![[#TYPE]]
 static int f3(void) { return 1; }
 
 // CHECK-DAG: declare !kcfi_type ![[#TYPE]]{{.*}} i32 @[[F4]]()
 extern int f4(void);

clang/test/CodeGen/ubsan-function.cpp

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,64
 // RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,64
 // RUN: %clang_cc1 -triple aarch64_be-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,64
 // RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s --check-prefixes=CHECK,ARM,32
 
-// CHECK: define{{.*}} void @_Z3funv() #0 !func_sanitize ![[FUNCSAN:.*]] {
+/// With -munaligned-access, ensure that the alignment is at least 4.
+// RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all -target-feature +strict-align | FileCheck %s --check-prefix=ALIGN4
+/// Smaller -faligned-function= is overridden while larger -faligned-function= wins.
+// RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all -target-feature +strict-align -function-alignment 1 | FileCheck %s --check-prefix=ALIGN4
+// RUN: %clang_cc1 -triple arm-none-eabi -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all -target-feature +strict-align -function-alignment 5 | FileCheck %s --check-prefix=ALIGN32
+
+// CHECK:   define{{.*}} void @_Z3funv() #0 !func_sanitize ![[FUNCSAN:.*]] {
+// ALIGN4:  define{{.*}} void @_Z3funv() #0 align 4 !func_sanitize ![[FUNCSAN:.*]] {
+// ALIGN32: define{{.*}} void @_Z3funv() #0 align 32 !func_sanitize ![[FUNCSAN:.*]] {
 void fun() {}
 
+// CHECK:   define{{.*}} void @_Z8aligned2v() #[[#]] align 2
+// ALIGN4:  define{{.*}} void @_Z8aligned2v() #[[#]] align 2
+// ALIGN32: define{{.*}} void @_Z8aligned2v() #[[#]] align 2
+__attribute__((aligned(2)))
+void aligned2() {}
+
 // CHECK-LABEL: define{{.*}} void @_Z6callerPFvvE(ptr noundef %f)
 // ARM:   ptrtoint ptr {{.*}} to i32, !nosanitize !5
 // ARM:   and i32 {{.*}}, -2, !nosanitize !5
 // ARM:   inttoptr i32 {{.*}} to ptr, !nosanitize !5
 // CHECK: getelementptr <{ i32, i32 }>, ptr {{.*}}, i32 -1, i32 0, !nosanitize
 // CHECK: load i32, ptr {{.*}}, align {{.*}}, !nosanitize
 // CHECK: icmp eq i32 {{.*}}, -1056584962, !nosanitize
 // CHECK: br i1 {{.*}}, label %[[LABEL1:.*]], label %[[LABEL4:.*]], !nosanitize