durin42 added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:4605
+ TryEmitAsCallSiteAttribute(const llvm::AttributeList &Attrs) {
+ llvm::AttributeList NewAttrs = Attrs;
+ if (AA)
----------------
jyknight wrote:
> 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)
It sounds like (between this comment and the one below) I should just abandon
the code-sharing between AllocAlignAttrEmitter and AssumeAlignedAttrEmitter -
is that right?
Also, should I be making a new test case or adding to an existing one for a
sanitized allocalign test case? Is that an llvm-level thing or a clang-level
thing?
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
