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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D34299#788152, @vsk wrote:

> 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.


Ok. However, when the source is not using C++, the check for `-fsanitize=return` won't be emitted. So we will end up with a call to `__ubsan_handle_nullability_return_abort` without a location, which won't report a diagnostic, but the program will crash because compiler-rt will call `abort`. This seems like a regression, since previously in C mode this diagnostic was reported.

> @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