User Details
- User Since
- Jul 15 2014, 12:28 PM (410 w, 2 d)
Apr 19 2022
LGTM, thanks!
Mar 30 2022
Do you have commit access, or should I land this for you?
Dec 15 2021
LGTM :)
Nov 12 2021
LGTM % 2 nits. Please feel free to commit after it LGT @aaron.ballman too. :)
Oct 20 2021
LGTM. Thanks again!
Oct 19 2021
Thanks for this! The idea LGTM, and sounds like a solid way for us to do better about diagnosing FORTIFY'ed calls in Clang. I have a handful of mostly nits/questions for you :)
LGTM % nits -- thanks for this! :)
Aug 3 2021
...This entirely dropped off my radar. Will try to land it now; thanks, all!
Jul 29 2021
Welcome! :)
Jul 28 2021
Thanks for the reviews and commentary!
Jul 27 2021
Added steveklabnik to the doc as requested -- thanks!
Jul 26 2021
I'm curious, as X86 is probably one of the more complicated targets, does rust make use of llvm::X86::updateImpliedFeatures and llvm::X86::getFeaturesForCPU from llvm/lib/Support/X86TargetParser.cpp? That's a large chunk of the code clang uses for feature interactions.
Jul 22 2021
Why not add a separate flag instead of polluting the feature string?
Jul 21 2021
Jul 20 2021
please give a day for other reviewers to add any last minute comments, then i think we can land this.
Jul 15 2021
Adding something to the IR for the sole purpose of producing a diagnostic feels really weird. I'm not sure I see why the frontend can't see this attribute and directly warn
Jul 14 2021
thanks for this! mostly just nits from me
Jul 7 2021
Thanks for the quick review! :)
Jun 14 2021
Jun 1 2021
May 13 2021
one drive-by nit and lgtm -- thanks!
Apr 21 2021
Thanks for this!
Apr 19 2021
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 :)
Apr 15 2021
Thanks for this! I think this warning looks valuable.
Apr 9 2021
Thanks for this!
Apr 6 2021
Thanks for the comment!
Mar 26 2021
Mar 25 2021
Thank you all for your support!
Mar 23 2021
Jan 20 2021
reverted in https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea until we can figure out how to address the issues outlined above. thanks!
Jan 7 2021
Jan 6 2021
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.