[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In https://reviews.llvm.org/D34299#787795, @arphaman wrote:

> It looks like if we have a function without the `return` (like the sample below), we will pass in a `0` as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are `nil`. Is this a bug or is this the intended behaviour?
>
>   int *_Nonnull nonnull_retval1(int *p) {
>   }
>


This is the intended behavior (I'll add a test). Users should not see a "null return value" diagnostic here. There is another check, -fsanitize=return, which can catch this issue.

@filcab --

> Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

> I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?


https://reviews.llvm.org/D34299