User Details
- User Since
- Jul 15 2014, 12:28 PM (340 w, 5 d)
Wed, Jan 20
reverted in https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea until we can figure out how to address the issues outlined above. thanks!
Thu, Jan 7
Wed, Jan 6
thanks for working on this!
Dec 7 2020
thanks!
Nov 30 2020
thanks for this!
Nov 12 2020
thanks!
Nov 2 2020
Oct 28 2020
looks like all comments here are addressed, so i'll land this.
Oct 26 2020
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.
Oct 22 2020
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
Oct 1 2020
Sep 25 2020
Sep 16 2020
(still lgtm. :) )
Sep 15 2020
but when prinitng we were still seeing an unoptimized access
as MustAlias due to the flag not being reset
Aug 3 2020
lgtm with a few tiny nits. thanks for working on this!
Jul 6 2020
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 :)
Jun 9 2020
Eesh -- sorry for taking forever. :)
May 26 2020
thanks for this!
May 20 2020
Sorry for the latency :)
May 6 2020
Thanks for the patch :)
May 5 2020
lgtm with one nit. thanks!
May 4 2020
sorry for the latency -- a bit busy now, but I hope to get to this by EOD Wednesday :)
Apr 29 2020
oof. thanks for this!
Apr 16 2020
Thanks for the report! Looking now.
Apr 15 2020
thanks!
Apr 14 2020
Nothing in the real world :)
Apr 13 2020
LGTM. If no one else comments within a day or so, feel free to land.
Apr 10 2020
Thanks for this!
Apr 7 2020
Apr 6 2020
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.
Apr 3 2020
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.
Apr 1 2020
I think there are 2 cases to distinguish:
- 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.
Mar 30 2020
Thanks for working on this!
Mar 17 2020
(I really need to circle back and finish the LocationSize migration...)
Mar 10 2020
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!
Mar 9 2020
Ack -- I'll do that in the future.
Mar 5 2020
thanks for this!
Mar 3 2020
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.
Mar 2 2020
Feb 24 2020
Friendly ping Gerolf (or anyone else who has opinions on standardish benchmarks) :)
Feb 14 2020
I have concerns on so much plumbing changes to make the small heuristic change.
Feb 13 2020
ping :)
Feb 8 2020
Thanks for the review!
Feb 6 2020
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 :)
Feb 5 2020
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?
Feb 4 2020
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."
Feb 3 2020
LGTM after this last nit is addressed. Like said, I plan to leave the final stamp to someone with more context. Thanks again!
Feb 2 2020
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.
Jan 30 2020
Thanks for this!
Jan 25 2020
Jan 16 2020
(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"...)
Dec 12 2019
Just a few more nits, and this LGTM. Thanks again!
Dec 11 2019
Should we also have a quick test-case in Sema/ verifying that we still get the warnings that Eli mentioned?
Dec 5 2019
Thanks for looking into this!
Dec 4 2019
Thanks for reviewing!
Nov 22 2019
Oct 15 2019
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 :)
Oct 10 2019
Thanks!
Oct 8 2019
Thanks for this!
Sep 30 2019
Does this have any significant impact on -fsyntax-only performance?