one drive-by nit and lgtm -- thanks!

Thanks for this!

Just a few more nits and LGTM. We probably want the thoughts of someone with ownership in warnings to be sure. +rtrieu might be good?

friendly ping :)

Thanks for this! I think this warning looks valuable.

Thanks for this!

Thanks for the comment!

Thank you all for your support!

reverted in https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea until we can figure out how to address the issues outlined above. thanks!

thanks for working on this!

thanks!

thanks for this!

thanks!

looks like all comments here are addressed, so i'll land this.

If we want _full_ analysis, clang-analyzer-unix.Malloc is what originally flagged the free(function_pointer) case that I referenced above; its complaint was:

LGTM, though please wait for a review from someone with more expertise with clang's warnings (maybe Aaron or Richard?) to land.

thanks for this! i like the idea of this warning; seems like a cheap way to catch ugly bugs early. +rtrieu and aaron.ballman, as this is a new diagnostic

(still lgtm. :) )

but when prinitng we were still seeing an unoptimized access
as MustAlias due to the flag not being reset

lgtm with a few tiny nits. thanks for working on this!

Concept and implementation LGTM. Please wait for comment from +alexfh before landing, since I think they have more ownership over clang-tidy in general than I do :)

Eesh -- sorry for taking forever. :)

thanks for this!

Sorry for the latency :)

Thanks for the patch :)

lgtm with one nit. thanks!

sorry for the latency -- a bit busy now, but I hope to get to this by EOD Wednesday :)

oof. thanks for this!

Thanks for the report! Looking now.

thanks!

Nothing in the real world :)

LGTM. If no one else comments within a day or so, feel free to land.

Thanks for this!

IIUC, the additional set would be used for the non-alloca cases, as we have to ensure that there are overwrites along all paths to the exit.

For a more direct comparison, I offer https://godbolt.org/z/fqAhUC . The lack of optimization in the later case is because we're forced to mark the call to __builtin_memcpy in the inline memcpy as nobuiltin. If we instead rename things, this issue doesn't happen: https://godbolt.org/z/FKNTWo.

I think there are 2 cases to distinguish:

1. For accesses to non-alloca objects, this matches what the intrinsic achieves I think. We can only eliminate stores to objects that are visible after the function returns, if they are overwritten along all paths to the exit. So if we have determined a set of blocks that overwrite A (and there are no reads in between), we could check if all paths to the exit from A must go through one of the overwriting blocks. I think that matches your suggestion.

Thanks for working on this!

(I really need to circle back and finish the LocationSize migration...)

Please wait for a stamp from another reviewer here, too.

LGTM modulo a few cosmetic nits. Similar to the previous review, please wait for approval from someone with more ownership here before landing.

Thanks so much for working on this!

Ack -- I'll do that in the future.

thanks for this!

I see -- that seems like a much broader change, but also probably worthwhile. I have a few ideas about how to tackle that; let me see what I can come up with locally.

Friendly ping Gerolf (or anyone else who has opinions on standardish benchmarks) :)

I have concerns on so much plumbing changes to make the small heuristic change.

ping :)

Thanks for the review!

Oof, yeah, these look similar, but it's not clear to me if there's a good way to unify them as a part of this patch (or lift this out to there). Lemme poke it a bit and see :)

Updated to plumb through the "are we doing ThinLTO || prelink LTO" bit in a way that makes sense to me. Suggestions for potentially better approaches is welcome :)

Yup - I also don't know of any location caching the cost. I wonder if this was a nerver-realized goal, and whether it is abandoned. May be worth checking with the author of the comment?

Looks like this was done in https://reviews.llvm.org/D65280; closing

On the first bullet, I'm trying to find where InlineCost values are cached. Are we doing that?

Until recently, both CrOS and Android disabled FORTIFY wholesale when any of asan/msan/tsan were active. Bionic recently got support for FORTIFY playing nicely with sanitizers, but that support boils down to "turn off all the FORTIFY runtime checks, but leave the ones that generate compiler diags on."