jyknight added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:4605
+ TryEmitAsCallSiteAttribute(const llvm::AttributeList &Attrs) {
+ llvm::AttributeList NewAttrs = Attrs;
+ if (AA)
----------------
durin42 wrote:
> 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?
Agree, it does sound like there's not going to be much code-sharing, since
emitAlignmentAssumption is already separate. If you find it's clearer to make
them be separate, that sounds reasonable.
Yes, there should be a test-case for sanitized allocalign, just like for
sanitized assume_align.
This sanitizer is all in the frontend, see
clang/test/CodeGen/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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
