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

LGTM after this last nit is addressed. Like said, I plan to leave the final stamp to someone with more context. Thanks again!

Thanks for this! I'm always happy to see reduction tooling gain new powers :)

So there are existing test with 0 sized objects that would fail if we do not have the Size != 0 check but I'll add a few more to nail down what we expect as soon as I find the time. It might take a week or two though.

Thanks for this!

(It's interesting to me if gcc doesn't warn about that libcxx code, since the whole point of the gnuc 5.0 check there was "the compiler should check this for us now"...)

Just a few more nits, and this LGTM. Thanks again!

Should we also have a quick test-case in Sema/ verifying that we still get the warnings that Eli mentioned?

Thanks for looking into this!

Thanks for reviewing!

I don't immediately see any issues with this patch, but it's outside of my area of expertise, so I'll defer to the other reviewers :)

Thanks!

Thanks for this!

Does this have any significant impact on -fsyntax-only performance?