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

[Augie Fackler via Phabricator via cfe-commits]

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