This is an archive of the discontinued LLVM Phabricator instance.

[-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas
ClosedPublic

Authored by ziqingluo-90 on Dec 15 2022, 4:59 PM.

Details

Summary

Add a pair of clang pragmas:
#pragma clang unsafe_buffer_usage begin
#pragma clang unsafe_buffer_usage end,
which specify the start and end of an (unsafe buffer checking) opt-out region.

Behaviors of opt-out regions conform to the following rules:

  1. No nested nor overlapped opt-out regions are allowed. One cannot start an opt-out region with ... unsafe_buffer_usage begin but never close it with ... unsafe_buffer_usage end. Mis-use of the pragmas will be warned.
  2. Warnings raised from unsafe buffer operations inside such an opt-out region will always be suppressed. This behavior CANNOT be changed by clang diagnostic pragmas or command-line flags.
  3. Warnings raised from unsafe operations outside of such opt-out regions may be reported on declarations inside opt-out regions. These warnings are NOT suppressed.
  4. An un-suppressed unsafe operation warning may be attached with notes. These notes are NOT suppressed as well regardless of whether they are in opt-out regions.

The implementation maintains a separate sequence of location pairs representing opt-out regions in Preprocessor.
The UnsafeBufferUsage analyzer reads the region sequence to check if an unsafe operation is in an opt-out region. If it is, discard the warning raised from the operation immediately.

Examples,

void f() {
  int * p = new int [10];
#pragma clang unsafe_buffer_usage begin
  p[5];
#pragma clang unsafe_buffer_usage end
}

Nothing will be warned in the code above since the only unsafe operation p[5] is in an opt-out region.

void f() {
#pragma clang unsafe_buffer_usage begin
   int * p = new int [10];  // expect a warning on `p` for the unsafe operation below
#pragma clang unsafe_buffer_usage end
   p[5]; // may have a note here which is an attachment of the warning above
}

In the example above, a warning triggered by p[5], which is not in an opt-out region, will be reported on the declaration. Although the declaration is in an opt-out region, the warning is NOT suppressed.

Diff Detail

Event Timeline

ziqingluo-90 created this revision.Dec 15 2022, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 4:59 PM
ziqingluo-90 requested review of this revision.Dec 15 2022, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2022, 4:59 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ziqingluo-90 edited the summary of this revision. (Show Details)Dec 15 2022, 5:01 PM
ziqingluo-90 retitled this revision from [WIP][-Wunsafe-buffer-usage] Add safe buffer opt-out pragma to [WIP][-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas.Dec 15 2022, 5:03 PM
ziqingluo-90 edited the summary of this revision. (Show Details)
NoQ added inline comments.Dec 15 2022, 6:57 PM
clang/include/clang/Basic/DiagnosticLexKinds.td
947

IIUC ExtWarn means it's a warning of the form "warning: XXX is a language extension". It's not just a warning that has something to do with language extensions, it's a warning that tries to warn the user about the very fact that it's an extension. So we should just use Warning or Error.

Also about wording: we already have a somewhat similar #pragma assume_nonnull, can we use similar text? And probably prefer Error because that's what the other pragma uses too:

def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">;
def err_pp_double_begin_of_assume_nonnull : Error<
  "already inside '#pragma clang assume_nonnull'">;
def err_pp_unmatched_end_of_assume_nonnull : Error<
  "not currently inside '#pragma clang assume_nonnull'">;
def err_pp_include_in_assume_nonnull : Error<
  "cannot %select{#include files|import headers}0 "
  "inside '#pragma clang assume_nonnull'">;
def err_pp_eof_in_assume_nonnull : Error<
  "'#pragma clang assume_nonnull' was not ended within this file">;
clang/lib/Sema/AnalysisBasedWarnings.cpp
2379 ↗(On Diff #483332)

I believe this check should be performed *much earlier*. It's not about how we display unsafe usages to the user; we can exclude variables from analysis entirely when all their unsafe uses are guarded by the pragma. I suspect that we can drop these gadgets as early as in findGadgets() phase (assuming that D140062 causes us to never rely on unsafe gadgets for fixits).

Fixing bugs in my test.

jkorous added inline comments.Dec 16 2022, 10:40 AM
clang/lib/Sema/AnalysisBasedWarnings.cpp
2379 ↗(On Diff #483332)

Let's addres this in a follow-up patch.

Addressing comments.

ziqingluo-90 added inline comments.Dec 16 2022, 1:09 PM
clang/include/clang/Basic/DiagnosticLexKinds.td
947

Fixed!

clang/lib/Sema/AnalysisBasedWarnings.cpp
2379 ↗(On Diff #483332)

I agree to both of you. Moving the check into UnsafeBufferUsage.cpp may depends on this https://reviews.llvm.org/D140062 patch. So I will make it a follow-up patch.

Rebase the patch.

Move the check of whether a node is in an opt-out region to an earlier stage---the AST matching stage.

NoQ added inline comments.Jan 18 2023, 4:59 PM
clang/include/clang/Basic/Diagnostic.h
1040–1043

Ok, now I no longer see why this data should live in DiagnosticEngine. It's mostly about analysis, right? The pragma simply makes our analysis produce different results, regardless of whether these results are used for producing diagnostics or something else. Maybe let's keep it all in Preprocessor?

clang/lib/Analysis/UnsafeBufferUsage.cpp
543

This prevents safe fixable gadgets from being found in the opt-out zone. I think this clause should only apply to warning gadgets.

551–555

I think we should match all DeclStmts as well, because otherwise we may be unable to find the variable to fix.

ziqingluo-90 added inline comments.Jan 19 2023, 2:14 PM
clang/include/clang/Basic/Diagnostic.h
1040–1043

make sense to me!

clang/lib/Analysis/UnsafeBufferUsage.cpp
543

You are right! Fixables should be found regardless of whether they are in an opt-out zone. A Fixable could later be immediately discarded once we know that the variable declaration associated to the Fixable is in an opt-out zone.

551–555

In case we are unable to find the variable to fix, it means that the variable declaration is in an opt-out zone. So we don't fix the variable anyway, right?

Or do you mean that a variable may still get fixed even if its declaration is in an opt-out zone? I could imagine it is possible if the variable is involved in some assignments that we want to fix.

NoQ added inline comments.Jan 19 2023, 3:51 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
551–555

do you mean that a variable may still get fixed even if its declaration is in an opt-out zone?

Yes I think that's the case. We do have tests about this right?:

#pragma clang unsafe_buffer_usage begin
  ...
  int *p3 = new int[10]; // expected-warning{{'p3' is an unsafe pointer used for buffer access}}

#pragma clang unsafe_buffer_usage end
  ...
  p3[5]; //expected-note{{used in buffer access here}}

And I think it's safe to assume that every time we emit a warning, we also want to emit a fixit.

If only we had any fixits implemented, this code would have been much easier to refactor because we'd have some actual tests covering it 🤔

ziqingluo-90 added inline comments.Jan 19 2023, 3:57 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
543

Oh wait, scratch what I said above.

FixableGadgets should be found regardless of whether they are in an opt-out zone. They will be attached to variable declarations. The emission of an Unsafe Buffer diagnostic of a variable declaration only depends on WarningGadgets. So FixableGadgets have nothing to do with opt-out regions.

ziqingluo-90 added inline comments.Jan 19 2023, 4:06 PM
clang/lib/Analysis/UnsafeBufferUsage.cpp
551–555

sorry I didn't think it through when I reply the comments. You are right.
Fixables and variable declarations should be irrelevant to opt-out regions.

I'll rebase this patch w.r.t. https://reviews.llvm.org/D139737 so that we will have tests covering it.

ziqingluo-90 retitled this revision from [WIP][-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas to [-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas.

Put the implementation of the opt-out pragma completely in Preprocessor.
Add an opt-out pragma test for fix-its.

NoQ accepted this revision.Jan 26 2023, 1:00 PM

Ok I think I'm happy with the patch! Let's give other folks a few days to comment and then land?

The commit message will need to be updated to remove mentions of DiagnosticsEngine.

I'm somewhat surprised that there is no centralized documentation for pragmas, like there is for attributes. But I hope we'll provide enough documentation as part of D136811, which would also supply release notes.


It's worth saying out loud that this patch doesn't implement the opt-in pragma (#pragma only_safe_buffers begin ... end) as proposed in D136811, but only the opt-out pragma. We're actually downprioritizing it (possibly dropping entirely) because we've discovered a complete lack of symmetry between these two pragmas. While the opt-out pragma is straightforward, the opt-in pragma is very hard to get right, as it implies the possibility to turn on compiler warnings that were disabled by all other means. Moreover, because warnings enabled this way don't necessarily live between begin and end, this makes it tricky to avoid running unsafe buffer usage analysis on the entire translation unit even when the warning is completely disabled and there are no opt-in pragmas anywhere in the translation unit. So there are a lot of potential scary interactions that we'll have to address before we consider implementing the opt-in pragma. They probably can be addressed, but it'll take us some time to come up with a good design.

This revision is now accepted and ready to land.Jan 26 2023, 1:00 PM

Change the fix-it test style

ziqingluo-90 edited the summary of this revision. (Show Details)Feb 3 2023, 2:00 PM
This revision was landed with ongoing or failed builds.Feb 7 2023, 4:54 PM
This revision was automatically updated to reflect the committed changes.