[PATCH] D121629: clang: also check alloc_alignment claims in return

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:3684
 
+  const AllocAlignAttr *AA = CurCodeDecl->getAttr<AllocAlignAttr>();
+  if (SanOpts.has(SanitizerKind::Alignment) && AA) {
----------------
It may be nice to also verify the alignment required by an AssumeAlignAttr while you're in here already.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:3879
+      emitAlignmentAssumption(RV, FnRetTy,
+                              // TODO: this first location seems like it should
+                              // be the _return_ statement is, but I'm not quite
----------------
The return location is not available as a static SourceLocation here, because there can be multiple "return"s in the function, and the return-value-checking sanitizer code gets shared for all of them.

See below for how the nonnull check works -- it loads the location from the ReturnLocation pointer, and passes it as a dynamic parameter to the sanitizer handler.

So to get this right, you'll need to plumb that same thing through, which looks like a bit of work.

Firstly -- you'll  need to create a new variant of emitAlignmentAssumption which takes a Value for the source location, and ultimtaely ends up calling EmitCheck with a newly defined SanitizerHandler kind (perhaps call the new handler "return_alignment_assumption"?). The difference in the new handler vs the existing "alignment_assumption" is that it has Loc as a dynamic argument, instead of contained in the static Data struct.

Then, you'll need to actually write the new ubsan error handler function in compiler-rt underlying that. As a hint I'd say compare `__ubsan_handle_nonnull_return_v1`, which takes the SourceLocation of the return as a parameter, and the SourceLocation of the attribute in the "Data" param pointing to constant data block, vs the `__ubsan_handle_alignment_assumption` which takes both SourceLocation in the static data block (and has other dynamic parameters).


================
Comment at: clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp:30
+// CHECK-SANITIZE-NORECOVER:       cont:
+// CHECK-SANITIZE-NORECOVER-NEXT:    call void @llvm.assume(i1 true) [ "align"(ptr [[TMP0]], i64 [[ALIGNMENT]]) ]
+// CHECK-SANITIZE-NORECOVER-NEXT:    ret ptr [[TMP0]]
----------------
This llvm.assume is unnecessary here and we shouldn't emit it. Note that the other calls to emitAlignmentAssumption are primarily emitting this assume, and as a side-effect also emitting a sanitizer check while doing so. Here I think we actually _just_ want the sanitizer check.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121629/new/

https://reviews.llvm.org/D121629