[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast(FirstArg))
+FirstArg = CE->getSubExprAsWritten();
 

rsmith wrote:
> rjmccall wrote:
> > yronglin wrote:
> > > rjmccall wrote:
> > > > rsmith wrote:
> > > > > This looks very suspicious to me: this will remove a cast expression 
> > > > > that was written in the source code from the AST. That loses source 
> > > > > fidelity, can give the wrong answer if the cast changed the value 
> > > > > (such as a C++ derived-to-base conversion to a non-primary base 
> > > > > class), and in any case this is only done once where there could be 
> > > > > multiple explicit casts written on an argument to the builtin, so if 
> > > > > it's necessary, then it's not being done fully.
> > > > > 
> > > > > Can this be removed?
> > > > Somehow I missed this in my review.  Yes, this line should be 
> > > > unnecessary, and as you say, it is definitely wrong.
> > > CodeGen need real `user-written-type` to generate 
> > > `__ubsan_handle_alignment_assumption ` 's arg, but not `const void *`
> > Because we're using custom type-checking here, there is no conversion to 
> > `const void *` in the first place; that conversion was an artifact of the 
> > earlier implementation.
> > 
> > Also, if the previous code in CodeGen looked like this, then it was buggy: 
> > it is incorrect in the case that someone wrote `(const void*) foo` as the 
> > argument, because it would inappropriately look through the explicit, 
> > user-provided cast.
> The old implementation could get the pointer value wrong, not only the 
> alignment. For example:
> ```
> struct A { virtual ~A(); };
> 
> void *f(A *a) {
>   // Incorrectly returns `a` rather than the address of the complete object.
>   return __builtin_assume_aligned(dynamic_cast(a), 8);
> }
> ```
> The new implementation gets the pointer value wrong in more cases, such as:
> ```
> struct A { int n; };
> struct B { int n; };
> struct C : A, B {};
> 
> void *f(C *c) {
>   // Incorrectly returns `c` rather than the address of the B base class.
>   return __builtin_assume_aligned((B*)c, 8);
> }
> ```
Thanks for your tips, and is this an error in clang, 
https://godbolt.org/z/91qxq73Mb, if lose return 0 in main, ubsan will emit an 
error, but it will pass when there has a return statement. and I have submit an 
issue https://github.com/llvm/llvm-project/issues/62478


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

I've a new patch D133583  , please can you 
guys take a look


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Thanks very much for your comments @rjmccall @rsmith , I've take a look at 
D45383 , I believe that user code isn't 
allowed to declare  __builtin_*, but seems `intrin0.inl.h` is a system header 
on windows, should we keep compatibility(like `__va_start` in D45383 
) with it? or breaking it, but I'd like some 
further guidance. can you help me make that decision? I think you guys are 
experts in this area and have more experience than me, I'll see if I can 
accomplish soon! (maybe I should submit a new RFC patch).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast(FirstArg))
+FirstArg = CE->getSubExprAsWritten();
 

rjmccall wrote:
> yronglin wrote:
> > rjmccall wrote:
> > > rsmith wrote:
> > > > This looks very suspicious to me: this will remove a cast expression 
> > > > that was written in the source code from the AST. That loses source 
> > > > fidelity, can give the wrong answer if the cast changed the value (such 
> > > > as a C++ derived-to-base conversion to a non-primary base class), and 
> > > > in any case this is only done once where there could be multiple 
> > > > explicit casts written on an argument to the builtin, so if it's 
> > > > necessary, then it's not being done fully.
> > > > 
> > > > Can this be removed?
> > > Somehow I missed this in my review.  Yes, this line should be 
> > > unnecessary, and as you say, it is definitely wrong.
> > CodeGen need real `user-written-type` to generate 
> > `__ubsan_handle_alignment_assumption ` 's arg, but not `const void *`
> Because we're using custom type-checking here, there is no conversion to 
> `const void *` in the first place; that conversion was an artifact of the 
> earlier implementation.
> 
> Also, if the previous code in CodeGen looked like this, then it was buggy: it 
> is incorrect in the case that someone wrote `(const void*) foo` as the 
> argument, because it would inappropriately look through the explicit, 
> user-provided cast.
The old implementation could get the pointer value wrong, not only the 
alignment. For example:
```
struct A { virtual ~A(); };

void *f(A *a) {
  // Incorrectly returns `a` rather than the address of the complete object.
  return __builtin_assume_aligned(dynamic_cast(a), 8);
}
```
The new implementation gets the pointer value wrong in more cases, such as:
```
struct A { int n; };
struct B { int n; };
struct C : A, B {};

void *f(C *c) {
  // Incorrectly returns `c` rather than the address of the B base class.
  return __builtin_assume_aligned((B*)c, 8);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Please look into the history of that test case to see why we're specifically 
testing for the ability to redeclare this builtin.

We could certainly allow this builtin to be redeclared using the `const void*` 
type — that doesn't really reflect the generic nature of the builtin, but it 
would work for compatibility with other compilers in the vast preponderance of 
code — but if we don't really need to do it, let's not go down that road.




Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast(FirstArg))
+FirstArg = CE->getSubExprAsWritten();
 

yronglin wrote:
> rjmccall wrote:
> > rsmith wrote:
> > > This looks very suspicious to me: this will remove a cast expression that 
> > > was written in the source code from the AST. That loses source fidelity, 
> > > can give the wrong answer if the cast changed the value (such as a C++ 
> > > derived-to-base conversion to a non-primary base class), and in any case 
> > > this is only done once where there could be multiple explicit casts 
> > > written on an argument to the builtin, so if it's necessary, then it's 
> > > not being done fully.
> > > 
> > > Can this be removed?
> > Somehow I missed this in my review.  Yes, this line should be unnecessary, 
> > and as you say, it is definitely wrong.
> CodeGen need real `user-written-type` to generate 
> `__ubsan_handle_alignment_assumption ` 's arg, but not `const void *`
Because we're using custom type-checking here, there is no conversion to `const 
void *` in the first place; that conversion was an artifact of the earlier 
implementation.

Also, if the previous code in CodeGen looked like this, then it was buggy: it 
is incorrect in the case that someone wrote `(const void*) foo` as the 
argument, because it would inappropriately look through the explicit, 
user-provided cast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Seems that a builtin can't be redeclared which has custom type checking
https://github.com/llvm/llvm-project/blob/ec8f2905a33ba970543c8edb4141c47f30d325f7/clang/lib/Basic/Builtins.cpp#L210-L214


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Reproduce windows broken test:

ignore.cpp

  #include 
  
  void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
  void foo() {
int a;
(void) __builtin_assume_aligned(, 4);
  }



  FunctionDecl 0x14a80e480 <./ignore.cpp:3:1, col:7> col:7 
__builtin_assume_aligned 'void *(const void *, size_t, ...) noexcept'
  |-ParmVarDecl 0x14a80df80  col:44 'const void *'
  `-ParmVarDecl 0x14a80e050  col:52 'size_t':'unsigned long'
  FunctionDecl 0x14a80e210 <./ignore.cpp:3:7> col:7 implicit 
__builtin_assume_aligned 'void *(const void *, unsigned long, ...) noexcept' 
extern
  |-ParmVarDecl 0x14a80e308 <>  'const void *'
  |-ParmVarDecl 0x14a80e370 <>  'unsigned long'
  |-BuiltinAttr 0x14a80e2b0 <> Implicit 402
  |-NoThrowAttr 0x14a80e3e8  Implicit
  `-ConstAttr 0x14a80e410  Implicit

  ./ignore.cpp:3:7: error: cannot redeclare builtin function 
'__builtin_assume_aligned'
  void *__builtin_assume_aligned(const void *, size_t, ...) noexcept;
^
  ./ignore.cpp:3:7: note: '__builtin_assume_aligned' is a builtin with type 
'void *(const void *, unsigned long, ...) noexcept'
  1 error generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast(FirstArg))
+FirstArg = CE->getSubExprAsWritten();
 

rjmccall wrote:
> rsmith wrote:
> > This looks very suspicious to me: this will remove a cast expression that 
> > was written in the source code from the AST. That loses source fidelity, 
> > can give the wrong answer if the cast changed the value (such as a C++ 
> > derived-to-base conversion to a non-primary base class), and in any case 
> > this is only done once where there could be multiple explicit casts written 
> > on an argument to the builtin, so if it's necessary, then it's not being 
> > done fully.
> > 
> > Can this be removed?
> Somehow I missed this in my review.  Yes, this line should be unnecessary, 
> and as you say, it is definitely wrong.
CodeGen need real `user-written-type` to generate 
`__ubsan_handle_alignment_assumption ` 's arg, but not `const void *`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Thanks a lot for your comments @rsmith @rjmccall .

Firstly, as far as I know, turning on the `-fsanitizer=alignment` options when 
calling `__builtin_assume_aligned` in C code, Clang will emit `call void 
@__ubsan_handle_alignment_assumption(...)` in CodeGen,  and CodeGen need 
`user-written-type` to generate correct `TypeDescriptor` (this class in 
compiler-rt/UBSan).

Secondly, before this patch, 
`clang::CodeGen::CodeGenFunction::emitAlignmentAssumption` use 
`CastExpr->getSubExprAsWritten` to get `user-written-type` in CodeGen,  In 
`Diff 457643` , with John's comments, we use custom sema checking. we  just use 
`DefaultFunctionArrayLvalueConversion` to convert 1st arg, but not implicit 
cast 1st arg to `const void *`(We expect pass `user-written-type` to CodeGen).

Unfortunately,  `Diff 457643` broken windows sanitize test, because there have 
a forward declaration `__MACHINE(void * __cdecl __builtin_assume_aligned(const 
void *, size_t, ...) noexcept)`in `intrin0.inl.h`, I think the reason for this 
problem is we use `nct` in `BUILTIN(__builtin_assume_aligned, "v*vC*z.", 
"nct")`, I try to find a solution based on `Diff 457643`, what do you all think 
about?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast(FirstArg))
+FirstArg = CE->getSubExprAsWritten();
 

rsmith wrote:
> This looks very suspicious to me: this will remove a cast expression that was 
> written in the source code from the AST. That loses source fidelity, can give 
> the wrong answer if the cast changed the value (such as a C++ derived-to-base 
> conversion to a non-primary base class), and in any case this is only done 
> once where there could be multiple explicit casts written on an argument to 
> the builtin, so if it's necessary, then it's not being done fully.
> 
> Can this be removed?
Somehow I missed this in my review.  Yes, this line should be unnecessary, and 
as you say, it is definitely wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652
+  Expr *FirstArg = TheCall->getArg(0);
+  if (auto *CE = dyn_cast(FirstArg))
+FirstArg = CE->getSubExprAsWritten();
 

This looks very suspicious to me: this will remove a cast expression that was 
written in the source code from the AST. That loses source fidelity, can give 
the wrong answer if the cast changed the value (such as a C++ derived-to-base 
conversion to a non-primary base class), and in any case this is only done once 
where there could be multiple explicit casts written on an argument to the 
builtin, so if it's necessary, then it's not being done fully.

Can this be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D133202#3774905 , @aaron.ballman 
wrote:

> I've pushed the changes up for you, thanks!
>
> In D133202#3774801 , @yronglin 
> wrote:
>
>> ping~
>
> Something to keep in mind for the future: we typically only "ping" a review 
> after about a week of no activity on it (just because folks end up being 
> busy, taking vacation, want to contemplate the review for a while, etc).

Thanks a lot Aaron, I will remember what you said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I've pushed the changes up for you, thanks!

In D133202#3774801 , @yronglin wrote:

> ping~

Something to keep in mind for the future: we typically only "ping" a review 
after about a week of no activity on it (just because folks end up being busy, 
taking vacation, want to contemplate the review for a while, etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Aaron Ballman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG6ed21fc51523: Avoid __builtin_assume_aligned crash when the 
1st arg is array type (authored by yronglin, committed by aaron.ballman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/catch-alignment-assumption-array.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
  clang/test/Sema/builtin-assume-aligned.c

Index: clang/test/Sema/builtin-assume-aligned.c
===
--- clang/test/Sema/builtin-assume-aligned.c
+++ clang/test/Sema/builtin-assume-aligned.c
@@ -66,6 +66,11 @@
 }
 #endif
 
+int test13(int *a) {
+  a = (int *)__builtin_assume_aligned(a, 2 * 2.0); // expected-error {{argument to '__builtin_assume_aligned' must be a constant integer}}
+  return a[0];
+}
+
 void test_void_assume_aligned(void) __attribute__((assume_aligned(32))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
 int test_int_assume_aligned(void) __attribute__((assume_aligned(16))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
 void *test_ptr_assume_aligned(void) __attribute__((assume_aligned(64))); // no-warning
Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_array_volatiles
+void *ignore_array_volatiles() {
+  volatile int arr[] = {1};
+  return __builtin_assume_aligned(arr, 4);
+}
Index: clang/test/CodeGen/catch-alignment-assumption-array.c
===
--- /dev/null
+++ clang/test/CodeGen/catch-alignment-assumption-array.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:   define{{.*}}
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:%[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:%[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:   %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:   %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:   br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT: call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:   call void 

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin added a comment.

ping~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

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

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

[Lin Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 458403.
yronglin retitled this revision from "[Clang][CodeGen] Avoid 
__builtin_assume_aligned crash when the 1st arg is array type" to "[Clang] 
Avoid __builtin_assume_aligned crash when the 1st arg is array type".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133202

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/catch-alignment-assumption-array.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
  clang/test/Sema/builtin-assume-aligned.c

Index: clang/test/Sema/builtin-assume-aligned.c
===
--- clang/test/Sema/builtin-assume-aligned.c
+++ clang/test/Sema/builtin-assume-aligned.c
@@ -66,6 +66,11 @@
 }
 #endif
 
+int test13(int *a) {
+  a = (int *)__builtin_assume_aligned(a, 2 * 2.0); // expected-error {{argument to '__builtin_assume_aligned' must be a constant integer}}
+  return a[0];
+}
+
 void test_void_assume_aligned(void) __attribute__((assume_aligned(32))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
 int test_int_assume_aligned(void) __attribute__((assume_aligned(16))); // expected-warning {{'assume_aligned' attribute only applies to return values that are pointers}}
 void *test_ptr_assume_aligned(void) __attribute__((assume_aligned(64))); // no-warning
Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_array_volatiles
+void *ignore_array_volatiles() {
+  volatile int arr[] = {1};
+  return __builtin_assume_aligned(arr, 4);
+}
Index: clang/test/CodeGen/catch-alignment-assumption-array.c
===
--- /dev/null
+++ clang/test/CodeGen/catch-alignment-assumption-array.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:   define{{.*}}
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:%[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:%[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:   %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:   %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:   br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT: call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:   call void