This is an archive of the discontinued LLVM Phabricator instance.

clang: also check alloc_alignment claims in return
Needs ReviewPublic

Authored by durin42 on Mar 14 2022, 11:14 AM.

Details

Summary

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.

Diff Detail

Event Timeline

durin42 created this revision.Mar 14 2022, 11:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 11:14 AM
durin42 requested review of this revision.Mar 14 2022, 11:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 11:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lebedev.ri added a subscriber: lebedev.ri.

Please can you pose the next patch itself?
I suspect that propagation should simply not be done when sanitizer is enabled.

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.

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.

The rest of the stack is now implemented and uploaded.

lebedev.ri resigned from this revision.Jan 12 2023, 5:29 PM

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.

durin42 updated this revision to Diff 529446.Jun 7 2023, 3:05 PM
durin42 updated this revision to Diff 529615.Jun 8 2023, 8:49 AM

FYI: this is ready for review.

jyknight added inline comments.Jun 13 2023, 4:37 PM
clang/lib/CodeGen/CGCall.cpp
3871

It may be nice to also verify the alignment required by an AssumeAlignAttr while you're in here already.

3878

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.