This is an archive of the discontinued LLVM Phabricator instance.

[-Wunsafe-buffer-usage] Make raw (ungrouped) warnings a bit more verbose.
AcceptedPublic

Authored by NoQ on Mar 23 2023, 5:11 PM.

Details

Summary

So far we didn't pay enough attention to them given that we spent a lot of time on grouped warnings, but D146669 will cause them to be displayed more often. Make them nicer and more understandable.

Diff Detail

Event Timeline

NoQ created this revision.Mar 23 2023, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 5:11 PM
NoQ requested review of this revision.Mar 23 2023, 5:11 PM
NoQ added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11786

The first mode doesn't show up in any tests and it's probably dead code at this point.

malavikasamak added inline comments.Apr 26 2023, 1:16 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11786

What do you think of specifying the variable name in these warnings? It could be helpful when there are more than one DREs in a statement.

NoQ added inline comments.May 15 2023, 5:07 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11786

These warnings are emitted only when we *cannot* identify the variable; either because it's not there at all, or we don't know how to find it. If we could figure out the variable, we'd be emitting warn_unsafe_buffer_variable instead. IIRC, in all of the provided test cases the operand isn't a DRE.

That said, I totally agree that it's a great idea to try a few other things to point out the specific subexpression, even if we don't connect the warning gadget to a variable. Say, we can try to blame MemberExprs or CallExprs the same way we blame DREs, and include their name in the warning. I'll try a few things real quick and see how bad it gets.

Note that these are also the things for which we may eventually start providing constructive solutions. Say, an unsafe operation on CallExpr can be a reason to autofix the callee to return a std::span. So hopefully these warnings will eventually be replaced by better warnings in more and more cases.

ziqingluo-90 accepted this revision.May 18 2023, 3:10 PM

LGTM, Thanks!

This revision is now accepted and ready to land.May 18 2023, 3:10 PM
NoQ added inline comments.May 19 2023, 1:02 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11786

Wait nvm, the whole point of this patch is to warn when variable discovery is manually turned off! I'm an idiot, this absolutely deserves more effort.

NoQ updated this revision to Diff 552560.Aug 22 2023, 6:37 PM

Rebase.

Make warnings a bit more verbose than before. This plays nicely with our attempts to categorize unemitted fixits via -mllvm -debug-only=SafeBuffers, but this time it doesn't make sense to make the extra information debug-only; it makes perfect sense as part of the warning.

TODO: Some new code paths aren't covered by tests yet (captured variables, static member variables).

NoQ added inline comments.Aug 23 2023, 5:30 PM
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
231

I don't think this FIXME was correct. The code we're warning about isn't expanded from a template.

NoQ updated this revision to Diff 558179.Tue, Nov 28, 1:31 PM

Rebase. Add remaining tests.

I think this is ready to land now.