[PATCH] D119271: clang: emit allocalign to LLVM for alloc_align attributes

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp:55
   // CHECK-NEXT:                        %[[ALIGNMENT_RELOADED:.*]] = load i64, i64* %[[ALIGNMENT_ADDR]], align 8
-  // CHECK-NEXT:                        %[[X_RETURNED:.*]] = call noundef i8** @[[PASSTHROUGH]](i8** noundef %[[X_RELOADED]], i64 noundef %[[ALIGNMENT_RELOADED]])
-  // CHECK-SANITIZE-NEXT:               %[[PTRINT:.*]] = ptrtoint i8** %[[X_RETURNED]] to i64
-  // CHECK-SANITIZE-NEXT:               %[[MASK:.*]] = sub i64 %[[ALIGNMENT_RELOADED]], 1
-  // CHECK-SANITIZE-NEXT:               %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], %[[MASK]]
-  // CHECK-SANITIZE-NEXT:               %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
-  // CHECK-SANITIZE-NEXT:               %[[PTRINT_DUP:.*]] = ptrtoint i8** %[[X_RETURNED]] 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 ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[LINE_100_ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 %[[ALIGNMENT_RELOADED]], i64 0){{.*}}, !nosanitize
-  // CHECK-SANITIZE-RECOVER-NEXT:       call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[LINE_100_ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 %[[ALIGNMENT_RELOADED]], i64 0){{.*}}, !nosanitize
-  // CHECK-SANITIZE-TRAP-NEXT:          call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
-  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
-  // CHECK-SANITIZE:                  [[CONT]]:
-  // CHECK-NEXT:                        call void @llvm.assume(i1 true) [ "align"(i8** %[[X_RETURNED]], i64 %1) ]
+  // CHECK-NEXT:                        %[[X_RETURNED:.*]] = call noundef i8** @[[PASSTHROUGH]](i8** noundef %[[X_RELOADED]], i64 allocalign noundef %[[ALIGNMENT_RELOADED]])
   // CHECK-NEXT:                        ret i8** %[[X_RETURNED]]
----------------
jyknight wrote:
> lebedev.ri wrote:
> > durin42 wrote:
> > > lebedev.ri wrote:
> > > > durin42 wrote:
> > > > > lebedev.ri wrote:
> > > > > > This is a regression, the old behavior was correct.
> > > > > I don't think so: we now (as of D121629) check the alignment of the returned pointer in the callee instead of the caller, and LLVM knows about allocalign implying an alignment at the callsite. So I think it's correctly optimizing away the sanitizer checks here.
> > > > > 
> > > > > You or @jyknight will need to correct me if my understanding is incorrect here, but my understanding was that this change was what motivated D121629 in the first place.
> > > > I'm saying that as the author of said test and it's behavior.
> > > How is this wrong though? Are you concerned because the `allocalign` isn't being asserted upon by the sanitizer in the caller? How this a problem given that the callee code now does the assertions in sanitizer mode?
> > > 
> > > (If I'm not understanding please elaborate: this is my first nontrivial work on LLVM, so I'm going to need more than "I wrote this, and you are wrong" by way of help understanding my error.)
> > The original code was not relying on the happenstance that the called code would have been
> > compiled with sanitizers, and this new code does make such an assumption. This is a regression.
> > As i have said, we simply should not be emitting `allocalign` in such situation,
> > that would defeat the sanitizer check that should be emitted here.
> I suggested this change, which I think reasonable given that it's the same thing we are doing for null sanitization.
> 
> E.g. `clang -fsanitize=null -emit-llvm -O2 -o - -S /tmp/test.cc` on:
> ```
> int &foo(int *x);
> 
> int bar(int*x) {
>   return foo(x);
> }
> ```
> ->
> ```
> define dso_local noundef i32 @_Z3barPi(i32* noundef %x) local_unnamed_addr #0 {
> entry:
>   %call = tail call noundef nonnull align 4 dereferenceable(4) i32* @_Z3fooPi(i32* noundef %x)
>   %0 = load i32, i32* %call, align 4, !tbaa !3
>   ret i32 %0
> }
> ```
> 
> There, too, we validate it in the callee before returning.
> 
> 
That may be your view on the situation, but i'm not sure i agree with it.
I think that is simply a bug. More concretely, now you will have a discrepancy with
e.g. `catch-alignment-assumption-attribute-assume_aligned-on-function.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119271