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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay abandoned this revision.
MaskRay added a comment.

Obsoleted by D154125 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154043/new/

https://reviews.llvm.org/D154043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154043/new/

https://reviews.llvm.org/D154043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154043/new/

https://reviews.llvm.org/D154043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154043/new/

https://reviews.llvm.org/D154043

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: efriedma, rjmccall, simon_tatham, samitolvanen.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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
  .p2align1
  % clang --target=armv6m-none-eabi -fsanitize=function a.c -S -o - | grep 
p2align
  .p2align1

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154043

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/kcfi.c
  clang/test/CodeGen/ubsan-function.cpp

Index: clang/test/CodeGen/ubsan-function.cpp
===
--- clang/test/CodeGen/ubsan-function.cpp
+++ clang/test/CodeGen/ubsan-function.cpp
@@ -3,9 +3,23 @@
 // 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
Index: clang/test/CodeGen/kcfi.c
===
--- clang/test/CodeGen/kcfi.c
+++ clang/test/CodeGen/kcfi.c
@@ -1,6 +1,8 @@
-// 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