Prior to this patch, we only check alloc_alignment claims in the caller.
I'm about to add some extra logic that threads alloc_alignment
information down into LLVM, which would then have a chance to defeat the
sanitizer checks by performing optimizations. To avoid that, we also
check alignment claims in the called function prior to return.
Details
- Reviewers
jyknight lebedev.ri
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Please can you pose the next patch itself?
I suspect that propagation should simply not be done when sanitizer is enabled.
I'm working on the next patch. Sadly we can't omit allocaligns into the IR layer when sanitizers are enabled because nothing will (when I'm done cleaning up MemoryBuiltins.cpp) have a way to know anything about the alignment of returned pointers.
This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.
clang/lib/CodeGen/CGCall.cpp | ||
---|---|---|
3872 | It may be nice to also verify the alignment required by an AssumeAlignAttr while you're in here already. | |
3879 | 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). | |
clang/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp | ||
30 | 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. |
It may be nice to also verify the alignment required by an AssumeAlignAttr while you're in here already.