LGTM, thanks!

Do you have commit access, or should I land this for you?

LGTM :)

LGTM % 2 nits. Please feel free to commit after it LGT @aaron.ballman too. :)

LGTM. Thanks again!

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! :)

...This entirely dropped off my radar. Will try to land it now; thanks, all!

Welcome! :)

Thanks for the reviews and commentary!

Added steveklabnik to the doc as requested -- thanks!

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.

Why not add a separate flag instead of polluting the feature string?

please give a day for other reviewers to add any last minute comments, then i think we can land this.

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

thanks for this! mostly just nits from me

Thanks for the quick review! :)

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.