Control analysis-based diagnostics with #pragma #136323
Conversation
Previously, analysis-based diagnostics (like -Wconsumed) had to be enabled at file scope in order to be run at the end of each function body. This meant that they did not respect #pragma clang diagnostic enabling or disabling the diagnostic. Now, these pragmas can control the diagnostic emission. However, the behavior requires some explanation (which is why the documentation is being updated). Analysis-based warnings run at the end of a function body, so the diagnostic needs to be enabled after the closing } for the function in order to run the analysis at all. However, if the analysis emits a diagnostic for line in the function, the diagnostic still needs to be enabled on that specific line in order to be shown to the user. Fixes llvm#42199
AaronBallman
added
clang:frontend
AaronBallman
requested review from
zygoloid,
gribozavr,
sgatev,
erichkeane,
ymand and
Sirraide
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang-analysis Author: Aaron Ballman (AaronBallman) ChangesPreviously, analysis-based diagnostics (like -Wconsumed) had to be enabled at file scope in order to be run at the end of each function body. This meant that they did not respect #pragma clang diagnostic enabling or disabling the diagnostic. Now, these pragmas can control the diagnostic emission. However, the behavior requires some explanation (which is why the documentation is being updated). Analysis-based warnings run at the end of a function body, so the diagnostic needs to be enabled after the closing } for the function in order to run the analysis at all. However, if the analysis emits a diagnostic for line in the function, the diagnostic still needs to be enabled on that specific line in order to be shown to the user. Fixes #42199 Full diff: https://github.com/llvm/llvm-project/pull/136323.diff 7 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f5cd1fbeabcfe..8d139123b47ce 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -395,6 +395,9 @@ Improvements to Clang's diagnostics
constructors to initialize their non-modifiable members. The diagnostic is
not new; being controlled via a warning group is what's new. Fixes #GH41104
+- Analysis-based diagnostics (like ``-Wconsumed`` or ``-Wunreachable-code``)
+ can now be correctly controlled by ``#pragma clang diagnostic``. #GH42199
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index 69256527f40c9..0ba4e2dedd4eb 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -1202,6 +1202,30 @@ Clang also allows you to push and pop the current warning state. This is
particularly useful when writing a header file that will be compiled by
other people, because you don't know what warning flags they build with.
+Note that the following diagnostic groups, which are ones based on analyzing
+the control flow graph for a function, require the diagnostic group to be
+enabled at the end of the function body (after the closing ``}``) in order to
+run the analysis, in addition to requiring the diagnostic group to be enabled
+at the line being diagnosed:
+
+ * ``-Wconsumed``
+ * ``-Wthread-safety-analysis``
+ * ``-Wunreachable-code``, ``-Wunreachable-code-aggressive``, or warnings
+ controlled by either of those flags
+
+thus, it is generally better for a ``push`` and ``pop`` pair of pragmas
+controlling behavior for an entire function be placed outside of the function
+body rather than within it. e.g.,
+
+.. code-block:: c
+
+ #pragma clang diagnostic push
+ #pragma clang diagnostic warning "-Wwhatever"
+ int d() {
+ // Better to put the pragmas outside of the function rather than within it.
+ }
+ #pragma clang diagnostic pop
+
In the below example :option:`-Wextra-tokens` is ignored for only a single line
of code, after which the diagnostics return to whatever state had previously
existed.
diff --git a/clang/include/clang/Sema/AnalysisBasedWarnings.h b/clang/include/clang/Sema/AnalysisBasedWarnings.h
index aafe227b84084..49023863f4503 100644
--- a/clang/include/clang/Sema/AnalysisBasedWarnings.h
+++ b/clang/include/clang/Sema/AnalysisBasedWarnings.h
@@ -49,7 +49,6 @@ class AnalysisBasedWarnings {
private:
Sema &S;
- Policy DefaultPolicy;
class InterProceduralData;
std::unique_ptr<InterProceduralData> IPData;
@@ -103,7 +102,8 @@ class AnalysisBasedWarnings {
// Issue warnings that require whole-translation-unit analysis.
void IssueWarnings(TranslationUnitDecl *D);
- Policy getDefaultPolicy() { return DefaultPolicy; }
+ // Gets the default policy which is in effect at the given source location.
+ Policy getPolicyInEffectAt(SourceLocation Loc);
void PrintStats() const;
};
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 34045a7274021..72962acee66d3 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2493,8 +2493,9 @@ class sema::AnalysisBasedWarnings::InterProceduralData {
CalledOnceInterProceduralData CalledOnceData;
};
-static unsigned isEnabled(DiagnosticsEngine &D, unsigned diag) {
- return (unsigned)!D.isIgnored(diag, SourceLocation());
+static bool isEnabled(DiagnosticsEngine &D, unsigned diag,
+ SourceLocation Loc) {
+ return !D.isIgnored(diag, Loc);
}
sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s)
@@ -2504,24 +2505,28 @@ sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s)
NumUninitAnalysisVariables(0), MaxUninitAnalysisVariablesPerFunction(0),
NumUninitAnalysisBlockVisits(0),
MaxUninitAnalysisBlockVisitsPerFunction(0) {
+}
+
+// We need this here for unique_ptr with forward declared class.
+sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() = default;
+sema::AnalysisBasedWarnings::Policy
+sema::AnalysisBasedWarnings::getPolicyInEffectAt(SourceLocation Loc) {
using namespace diag;
DiagnosticsEngine &D = S.getDiagnostics();
+ Policy P;
- DefaultPolicy.enableCheckUnreachable =
- isEnabled(D, warn_unreachable) || isEnabled(D, warn_unreachable_break) ||
- isEnabled(D, warn_unreachable_return) ||
- isEnabled(D, warn_unreachable_loop_increment);
+ P.enableCheckUnreachable = isEnabled(D, warn_unreachable, Loc) ||
+ isEnabled(D, warn_unreachable_break, Loc) ||
+ isEnabled(D, warn_unreachable_return, Loc) ||
+ isEnabled(D, warn_unreachable_loop_increment, Loc);
- DefaultPolicy.enableThreadSafetyAnalysis = isEnabled(D, warn_double_lock);
+ P.enableThreadSafetyAnalysis = isEnabled(D, warn_double_lock, Loc);
- DefaultPolicy.enableConsumedAnalysis =
- isEnabled(D, warn_use_in_invalid_state);
+ P.enableConsumedAnalysis = isEnabled(D, warn_use_in_invalid_state, Loc);
+ return P;
}
-// We need this here for unique_ptr with forward declared class.
-sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() = default;
-
static void flushDiagnostics(Sema &S, const sema::FunctionScopeInfo *fscope) {
for (const auto &D : fscope->PossiblyUnreachableDiags)
S.Diag(D.Loc, D.PD);
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 46933c5c43168..d28a2107d58a9 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16150,7 +16150,13 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
if (FSI->UsesFPIntrin && FD && !FD->hasAttr<StrictFPAttr>())
FD->addAttr(StrictFPAttr::CreateImplicit(Context));
- sema::AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
+ SourceLocation AnalysisLoc;
+ if (Body)
+ AnalysisLoc = Body->getEndLoc();
+ else if (FD)
+ AnalysisLoc = FD->getEndLoc();
+ sema::AnalysisBasedWarnings::Policy WP =
+ AnalysisWarnings.getPolicyInEffectAt(AnalysisLoc);
sema::AnalysisBasedWarnings::Policy *ActivePolicy = nullptr;
// If we skip function body, we can't tell if a function is a coroutine.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 9cd1a7e45fb64..a9f353be07ad2 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -16574,7 +16574,8 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
BD->setCaptures(Context, Captures, BSI->CXXThisCaptureIndex != 0);
// Pop the block scope now but keep it alive to the end of this function.
- AnalysisBasedWarnings::Policy WP = AnalysisWarnings.getDefaultPolicy();
+ AnalysisBasedWarnings::Policy WP =
+ AnalysisWarnings.getPolicyInEffectAt(Body->getEndLoc());
PoppedFunctionScopePtr ScopeRAII = PopFunctionScopeInfo(&WP, BD, BlockTy);
BlockExpr *Result = new (Context)
diff --git a/clang/test/Analysis/pragma-diag-control.cpp b/clang/test/Analysis/pragma-diag-control.cpp
new file mode 100644
index 0000000000000..c41793c15bbcc
--- /dev/null
+++ b/clang/test/Analysis/pragma-diag-control.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Werror=unreachable-code-aggressive %s
+
+// Test that analysis-based warnings honor #pragma diagnostic controls. These
+// diagnostics are triggered at the end of a function body, so the pragma needs
+// to be enabled through to the closing curly brace in order for the diagnostic
+// to be emitted.
+
+struct [[clang::consumable(unconsumed)]] Linear {
+ [[clang::return_typestate(unconsumed)]]
+ Linear() {}
+ [[clang::callable_when(consumed)]]
+ ~Linear() {}
+};
+
+int a() {
+ Linear l;
+ return 0; // No -Wconsumed diagnostic, analysis is not enabled.
+ return 1; // expected-error {{'return' will never be executed}}
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wconsumed"
+int b() {
+ Linear l;
+ return 0; // expected-error {{invalid invocation of method '~Linear' on object 'l' while it is in the 'unconsumed' state}}
+ return 1; // expected-error {{'return' will never be executed}}
+}
+#pragma clang diagnostic pop
+
+int c() {
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wconsumed"
+ Linear l;
+ return 0; // No -Wconsumed diagnostic, analysis is disabled before the closing brace
+ return 1; // expected-error {{'return' will never be executed}}
+#pragma clang diagnostic pop
+}
+
+int d() {
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wconsumed"
+#pragma clang diagnostic ignored "-Wunreachable-code-aggressive"
+ Linear l;
+ return 0; // expected-error {{invalid invocation of method '~Linear' on object 'l' while it is in the 'unconsumed' state}}
+ return 1; // Diagnostic is ignored
+}
+#pragma clang diagnostic pop
+
+int e() {
+#pragma clang diagnostic push
+#pragma clang diagnostic error "-Wconsumed"
+#pragma clang diagnostic ignored "-Wunreachable-code-aggressive"
+ Linear l;
+ return 0;
+ return 1;
+ // Both diagnostics are ignored because analysis is disabled before the
+ // closing brace.
+#pragma clang diagnostic pop
+}
+
+int f() {
+ Linear l;
+ return 0; // No -Wconsumed diagnostic, analysis is not enabled at } so it never runs to produce the diagnostic
+ return 1; // Diagnostic ignores because it was disabled at the }
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunreachable-code-aggressive"
+}
+#pragma clang diagnostic pop
+
+int g() {
+ Linear l;
+ return 0; // No -Wconsumed diagnostic, the diagnostic generated at } is not enabled on this line.
+ return 1; // expected-error {{'return' will never be executed}}
+#pragma clang diagnostic push
+#pragma clang diagnostic warning "-Wconsumed"
+}
+#pragma clang diagnostic pop
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- clang/test/Analysis/pragma-diag-control.cpp clang/include/clang/Sema/AnalysisBasedWarnings.h clang/lib/Sema/AnalysisBasedWarnings.cpp clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaExpr.cppView the diff from clang-format here.diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 6dcec5008..a9980149f 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2505,8 +2505,7 @@ sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s)
MaxCFGBlocksPerFunction(0), NumUninitAnalysisFunctions(0),
NumUninitAnalysisVariables(0), MaxUninitAnalysisVariablesPerFunction(0),
NumUninitAnalysisBlockVisits(0),
- MaxUninitAnalysisBlockVisitsPerFunction(0) {
-}
+ MaxUninitAnalysisBlockVisitsPerFunction(0) {}
// We need this here for unique_ptr with forward declared class.
sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() = default;
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 403960161..3a2cb992f 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -223,7 +223,8 @@ public:
AnalysisBasedWarnings::Policy &Override =
S->AnalysisWarnings.getPolicyOverrides();
switch (K) {
- default: break;
+ default:
+ break;
case diag::warn_unreachable:
case diag::warn_unreachable_break:
case diag::warn_unreachable_return:
|
| // We need this here for unique_ptr with forward declared class. | ||
| sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() = default; | ||
| void sema::AnalysisBasedWarnings::clearOverrides() { | ||
| PolicyOverrides.enableCheckUnreachable = false; |
There was a problem hiding this comment.
Tests show this working but.... where on earth are these set?
There was a problem hiding this comment.
The overrides are set in SemaPPCallback::PragmaDiagnostic(), the others come from AnalysisBasedWarnings::getPolicyInEffectAt()
There was a problem hiding this comment.
So from the user's perspective we're checking the flag twice: once at the } to see if we need to ramp up the analysis, then again at the diagnostic location to see if the diagnostic should be discarded. The warning will only be emitted if it's enabled at both source locations.
I like this. I think this is a very good practical tradeoff.
It behaves very counter-intuitively if you try to enable the diagnostic at one line with a pragma but it doesn't show up until you wrap your entire function in a pragma. But this is a very unlikely situation to occur naturally. Even if you do use the pragma to enable the diagnostic, why would you minimize the scope this way? (I also really like @cor3ntin's idea of emitting an explanatory warning if we can detect this case.)
The opposite situation, where the diagnostic is disabled on an individual line but enabled globally, is significantly more likely. And it is also the one that acts in an intuitive manner!
@ziqingluo-90 @jkorous-apple You might want to take a look at how this interacts with -Wunsafe-buffer-usage. If you still need to analyze the entire TU at once, the approach proposed in this patch probably won't work for you.
| AnalysisBasedWarnings::Policy &Override = | ||
| S->AnalysisWarnings.getPolicyOverrides(); | ||
| switch (K) { | ||
| default: break; |
There was a problem hiding this comment.
So every new analysis-based warning needs to be added to this list right? How weird are the consequences of forgetting it?
It might be a good idea to make a unified list of analysis-based warnings. (Or like, make it a flag in Diagnostic...Kinds.td.) Then it may be possible to build a runtime assertion to confirm that as long as AnalysisBasedWarnings::IssueWarnings() is on the stack, all warnings emitted by clang are on that list.
(Probably out of scope for this patch. Sounds like a lot of work. There could be easier ways to check this. Could also be too restrictive in practice.)
There was a problem hiding this comment.
At the moment, yes. As a follow-up, I would wonder if we could auto-generate this from tablegen.
Though, in this patch, we could perhaps instead of doing this override, have the policy store a LIST of override diagnostics, that we then check are in the list. So this would instead of this whole switch do:
Override.DiagOverrideList.push_back(K);
That would work for ALL diagnostics, at the expense of a somewhat significant size increase.
The current patch actually WILL work with it ONLY enabled on the line inside of the function, which I think is much more intuitive. The previous one (before Aaron implemented my tracking suggestion) did as you mentioned. My proposal was to track throughout the function if it was EVER turned on, so start the analysis if it is EVER turned on, then emit it if it is in the source-lcoation at the place of diagnostic.
|
Ooo whoa this is awesome!! |
Previously, analysis-based diagnostics (like -Wconsumed) had to be enabled at file scope in order to be run at the end of each function body. This meant that they did not respect #pragma clang diagnostic enabling or disabling the diagnostic.
Now, these pragmas can control the diagnostic emission. However, the behavior requires some explanation (which is why the documentation is being updated). Analysis-based warnings run at the end of a function body, so the diagnostic needs to be enabled after the closing } for the function in order to run the analysis at all. However, if the analysis emits a diagnostic for line in the function, the diagnostic still needs to be enabled on that specific line in order to be shown to the user.
Fixes #42199