[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#788379, @filcab wrote:

> In https://reviews.llvm.org/D34299#788152, @vsk wrote:
>
> > 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.
>
>
> It's still an `llvm::Constant`. Just like in StaticData, in line 2966.
>  Basically, I don't see why we need to add the store/load and an additional indirection, since the pointer is constant, and we can just emit the static data as before.


My earlier response made the assumption that you wanted a copy of the source location passed to the runtime handler, by-value. I see now that you're wondering why the source locations aren't part of the static data structure. That's because the location of the return statement isn't known at compile time, i.e it's not static data. The location depends on which return statement is executed.

> We're already doing `Data->Loc.acquire();` for the current version (and all the other checks).

The other checks do not allow the source location within a static data object to change.

>>> 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?
> 
> It would be a sanity check and help with code reading/keeping in mind the lifetime of the information. I'm ok with that happening only on `!NDEBUG` builds.
> 
> Reading the code, I don't know if a `ReturnLocation` might end up being used for more than one checks. If it's supposed to be a different one per check, etc.
>  If it's only supposed to be used once, I'd rather set it to `nullptr` right after use (at least when `!NDEBUG`), and `assert(!ReturnLocation)` before setting. It's not a big deal, but would help with making sense of the flow of information when debugging.

Sure, I'll reset it to Address::invalid().


https://reviews.llvm.org/D34299