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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
