jyknight added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:4605
+ TryEmitAsCallSiteAttribute(const llvm::AttributeList &Attrs) {
+ llvm::AttributeList NewAttrs = Attrs;
+ if (AA)
----------------
We do need to fallback to an assume for
"if(CGF.SanOpts.has(SanitizerKind::Alignment)" however -- and NOT emit an
allocalign (nor an align) attribute in that case.
This is necessary so that clang can emit explicit misalignment checks FIRST
(and abort with a message if they fail), before letting LLVM assume the
specified alignment. In order for the ubsan alignment check to work properly,
we need to not trigger misalignment-UB before the check executes!
See
clang/test/CodeGen/catch-alignment-assumption-attribute-assume_aligned-on-function.cpp
-- note how in the sanitized mode, it doesn't have an the "align 128" on the
call result, but instead emits a bunch of code to test alignment, and branch to
an ubsan abort on failure -- followed by the llvm.assume only on the successful
branch. Although the test-case is only testing assume_aligned, the same
behavior should be applicable to allocalign.
(Which shows that we clearly need a sanitized allocalign test-case, too)
================
Comment at: clang/test/CodeGen/alloc-align-attr.c:14
// CHECK-NEXT: [[CASTED_ALIGN:%.*]] = zext i32 [[TMP0]] to i64
// CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(i32* [[CALL]], i64
[[CASTED_ALIGN]]) ]
// CHECK-NEXT: [[TMP1:%.*]] = load i32, i32* [[CALL]], align 4
----------------
durin42 wrote:
> xbolva00 wrote:
> > now this llvm.assume is redudant and should be removed?
> I don't think so? Right now allocalign doesn't mean a whole lot (it really
> just answers some questions that used to be inferred from function name by
> MemoryBuiltins.cpp) so I'd rather leave the llvm.assume given that I doubt
> it's hurting anyone?
I think we should get rid of the assume -- it's redundant. If we're emitting an
allocalign attribute, I think we shouldn't bother emitting an align attribute,
nor an assume.
The prior code either adds an "align" to the return value, or an assume
instruction afterwards. Both of those are redundant with allocalign: the former
will already be done automatically on the llvm side when the LLVM side sees a
constant argument for an allocalign parameter, and the latter doesn't provide
any more information than the allocalign itself does.
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
