[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]]
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119271