[PATCH] D119271: CGCall: also emit LLVM `allocalign` attribute when handling AllocAlign

[James Y Knight via Phabricator via cfe-commits]

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