jyknight 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]] ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119271/new/ https://reviews.llvm.org/D119271 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits