[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)
----------------
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