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

[Filipe Cabecinhas via Phabricator via cfe-commits]

filcab added a comment.

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.
We're already doing `Data->Loc.acquire();` for the current version (and all the other checks).

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


https://reviews.llvm.org/D34299