This is an archive of the discontinued LLVM Phabricator instance.

[-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.
ClosedPublic

Authored by NoQ on Mar 22 2023, 4:30 PM.

Details

Summary

This patch implements a new clang driver flag -fsafe-buffer-usage-suggestions which allows turning the smart suggestion machine on and off (defaults to off). This is valuable for stability reasons, as the machine is being rapidly improved and we don't want accidental breakages to ruin the build for innocent users. It is also arguably useful in general because it enables separation of concerns between project contributors: some users will actively update the code to conform to the programming model, while others simply want to make sure that they aren't regressing it. Finally, there could be other valid reasons to opt out of suggestions entirely on some codebases (while continuing to enforce -Wunsafe-buffer-usage warnings), such as lack of access to hardened libc++ (or even to the C++ standard library in general) on the target platform.

When the flag is disabled, the unsafe buffer usage analysis is reduced to an extremely minimal mode of operation that contains virtually no smarts: not only it doesn't offer automatic fixits, but also textual suggestions such as "change the type of this variable to std::span to preserve bounds information" are not displayed*, and in fact the machine doesn't even try to blame specific variables in the first place, it simply warns on the operations and leaves everything else to the user. So this flag turns off a lot more of our complex machinery than what we already turn off in presence of say -fno-diagnostic-fixit-info.

The flag is discoverable: when it's off, the warnings are accompanied by a note: telling the user that there's a flag they can use. (Do we really need that? Maybe we should somehow throttle it to reduce noise?)

Diff Detail

Event Timeline

NoQ created this revision.Mar 22 2023, 4:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 4:30 PM
NoQ requested review of this revision.Mar 22 2023, 4:30 PM
NoQ updated this revision to Diff 507549.Mar 22 2023, 4:43 PM

Add some nice assertions.

jkorous accepted this revision.Mar 22 2023, 6:59 PM

LGTM.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11792

I like this idea!

Thinking about the questions you raised:

Do we really need that? Maybe we should somehow throttle it to reduce noise?

The only case I can see value in not emitting this note is when the user passes -fno-safe-buffer-usage-suggestions. In that case it can be argued that the user is well aware of the flag since they explicitly turned it off.

That being said I am fine with the current behavior and in either case expect we will tweak this once we get feedback from the users.

This revision is now accepted and ready to land.Mar 22 2023, 6:59 PM
ziqingluo-90 added inline comments.Mar 23 2023, 11:50 AM
clang/lib/Analysis/UnsafeBufferUsage.cpp
1139

nitpick: I'm afraid that we will forgot the difference between EmitSuggestions and EmitFixits later if there is no document about it.

ziqingluo-90 added inline comments.Mar 23 2023, 11:58 AM
clang/lib/Sema/AnalysisBasedWarnings.cpp
2203

nitpick:
I was a bit confused by the name of the variable. My understand of
!SuggestSuggestions here means "we are suggesting suggestions now".
Then what does SuggestSuggestions mean?

t-rasmud requested changes to this revision.Mar 23 2023, 12:42 PM
This revision now requires changes to proceed.Mar 23 2023, 12:42 PM
t-rasmud accepted this revision.Mar 23 2023, 12:42 PM
This revision is now accepted and ready to land.Mar 23 2023, 12:42 PM
t-rasmud added inline comments.Mar 23 2023, 12:43 PM
clang/lib/Sema/AnalysisBasedWarnings.cpp
2159

Was there a reason for naming this variable SuggestSuggestions? Can this be called EmitSuggestions? I think that would make it uniform and easy to follow the code.

NoQ updated this revision to Diff 507903.Mar 23 2023, 3:39 PM

Explain the weird variable name.

Confirm that even though warn_unsafe_buffer_operation and note_unsafe_buffer_operation have a different number of modes, the mismatch doesn't cause problems yet. Add an assert to catch the problem when it starts happening. Add tests to confirm that messages make sense.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11792

The only case I can see value in not emitting this note is when the user passes -fno-safe-buffer-usage-suggestions. In that case it can be argued that the user is well aware of the flag since they explicitly turned it off.

Yeah fair enough. This will need some rework though, because right now -fno-... is purely a Driver flag, so -cc1 invocations aren't even aware of it being passed this way. So it turns from a normal flag to a somewhat quirky flag.

clang/lib/Sema/AnalysisBasedWarnings.cpp
2159

It means "Should I emit a note reminding the user to enable -fsafe-buffer-usage-suggestions if they want suggestions?", so it's quite different from EmitSuggestions (in fact it's the opposite). Added a comment.

2203

Yeah it means "Should I emit a note reminding the user to enable -fsafe-buffer-usage-suggestions if they want suggestions?". Added a comment to the variable.

NoQ updated this revision to Diff 507912.Mar 23 2023, 4:05 PM

Don't recommend enabling suggestions when suggestions are impossible (eg. due to lack of C++20).

NoQ added a comment.EditedMar 28 2023, 3:17 PM

So in a nutshell, this is the intended behavior:

codediagnosticEmitFixits && EmitSuggestions!EmitFixits && EmitSuggestions!EmitFixits && !EmitSuggestions
void foo(int *p) {note: 'p' declared hereyesyesno
int *q;warning: 'q' is unsafe pointer to raw bufferyesyesno
note with fixit: change type of 'q' and 'p' to span to propagate bounds informationyesyes (no fixit)no
q = p;note: bounds information propagates from 'p' to 'q' hereyesyesno
q[5] = 10;note: unsafe raw buffer operation hereyesyesyes (warning)
note: pass -fsafe-buffer-usage-suggestions to receive code hardening suggestionsnonoyes
}

The first mode, "EmitFixits && EmitSuggestions", is the default behavior before this patch, and the behavior under -fsafe-buffer-usage-suggestions after this patch. The third mode, "!EmitFixits && !EmitSuggestions", is the default behavior after the patch.

The second mode, "!EmitFixits && EmitSuggestions" (the special behavior under -fno-diagnostics-fixit-info) was implemented for two purposes:

  • Recover some performance when -fno-diagnostics-fixit-info is passed (don't run the sophisticated fixit machinery if fixits are going to be discarded);
  • As a possible workaround for potential crashes in the fixit machinery.

We initially hoped that this mode will make "!EmitSuggestions" unnecessary, but as the table above demonstrates, a very large portion of the fixit machine's logic is still being invoked for the purposes of building notes, even when fixits aren't attached. And we cannot really disable this logic under that compiler flag, given that -fno-diagnostics-fixit-info already has a well-defined meaning which doesn't allow us to change the shape of the diagnostics in response. So we needed a new, more powerful flag, so here we are. I suspect that explicit support for !EmitFixits is no longer necessary. It is now useful for performance purposes only, and we don't any concrete data to support the claim that it's a valuable optimization, nor do we believe that this flag is ever intentionally passed in practice, so I guess I'll go ahead and remove it.

NoQ updated this revision to Diff 510933.EditedApr 4 2023, 2:47 PM
  • Rebase! (I'll update related revisions soon but not immediately, need to make sense out of them first.)
  • Eliminate the EmitFixits flag as discussed above (so the middle mode is now gone).
NoQ updated this revision to Diff 515110.Apr 19 2023, 3:41 PM

Before I forget: Update tests that didn't fail due to my patch because they were testing absence of things (warnings or fixits) and my patch only made them more absent. They need to keep testing absence of these things even when the flag is passed.

NoQ updated this revision to Diff 523595.May 18 2023, 4:09 PM

Rebase!

NoQ updated this revision to Diff 523601.May 18 2023, 4:43 PM

Rebase *correctly*!

Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 5:20 PM