Skip to content

Control analysis-based diagnostics with #pragma #136323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
----------------------------------

Expand Down
14 changes: 12 additions & 2 deletions clang/include/clang/Sema/AnalysisBasedWarnings.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class QualType;
class Sema;
namespace sema {
class FunctionScopeInfo;
class SemaPPCallbacks;
}

namespace sema {
Expand All @@ -33,6 +34,7 @@ class AnalysisBasedWarnings {
public:
class Policy {
friend class AnalysisBasedWarnings;
friend class SemaPPCallbacks;
// The warnings to run.
LLVM_PREFERRED_TYPE(bool)
unsigned enableCheckFallThrough : 1;
Expand All @@ -49,14 +51,16 @@ class AnalysisBasedWarnings {

private:
Sema &S;
Policy DefaultPolicy;

class InterProceduralData;
std::unique_ptr<InterProceduralData> IPData;

enum VisitFlag { NotVisited = 0, Visited = 1, Pending = 2 };
llvm::DenseMap<const FunctionDecl*, VisitFlag> VisitedFD;

Policy PolicyOverrides;
void clearOverrides();

/// \name Statistics
/// @{

Expand Down Expand Up @@ -103,7 +107,13 @@ 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);

// Get the policies we may want to override due to things like #pragma clang
// diagnostic handling. If a caller sets any of these policies to true, that
// will override the policy used to issue warnings.
Policy &getPolicyOverrides() { return PolicyOverrides; }

void PrintStats() const;
};
Expand Down
43 changes: 31 additions & 12 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2493,9 +2493,11 @@ class sema::AnalysisBasedWarnings::InterProceduralData {
CalledOnceInterProceduralData CalledOnceData;
};

static unsigned isEnabled(DiagnosticsEngine &D, unsigned diag) {
return (unsigned)!D.isIgnored(diag, SourceLocation());
}
template <typename... Ts>
static bool areAnyEnabled(DiagnosticsEngine &D, SourceLocation Loc,
Ts... Diags) {
return (!D.isIgnored(Diags, Loc) || ...);
};

sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s)
: S(s), IPData(std::make_unique<InterProceduralData>()),
Expand All @@ -2504,23 +2506,37 @@ 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);
// Note: The enabled checks should be kept in sync with the switch in
// SemaPPCallbacks::PragmaDiagnostic().
P.enableCheckUnreachable =
PolicyOverrides.enableCheckUnreachable ||
areAnyEnabled(D, Loc, warn_unreachable, warn_unreachable_break,
warn_unreachable_return, warn_unreachable_loop_increment);

DefaultPolicy.enableThreadSafetyAnalysis = isEnabled(D, warn_double_lock);
P.enableThreadSafetyAnalysis = PolicyOverrides.enableThreadSafetyAnalysis ||
areAnyEnabled(D, Loc, warn_double_lock);

DefaultPolicy.enableConsumedAnalysis =
isEnabled(D, warn_use_in_invalid_state);
P.enableConsumedAnalysis = PolicyOverrides.enableConsumedAnalysis ||
areAnyEnabled(D, Loc, warn_use_in_invalid_state);
return P;
}

// We need this here for unique_ptr with forward declared class.
sema::AnalysisBasedWarnings::~AnalysisBasedWarnings() = default;
void sema::AnalysisBasedWarnings::clearOverrides() {
PolicyOverrides.enableCheckUnreachable = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests show this working but.... where on earth are these set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overrides are set in SemaPPCallback::PragmaDiagnostic(), the others come from AnalysisBasedWarnings::getPolicyInEffectAt()

PolicyOverrides.enableConsumedAnalysis = false;
PolicyOverrides.enableThreadSafetyAnalysis = false;
}

static void flushDiagnostics(Sema &S, const sema::FunctionScopeInfo *fscope) {
for (const auto &D : fscope->PossiblyUnreachableDiags)
Expand Down Expand Up @@ -2871,6 +2887,9 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
AC.getCFG();
}

// Clear any of our policy overrides.
clearOverrides();

// Collect statistics about the CFG if it was built.
if (S.CollectStats && AC.isCFGBuilt()) {
++NumFunctionsAnalyzed;
Expand Down
37 changes: 37 additions & 0 deletions clang/lib/Sema/Sema.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,43 @@ class SemaPPCallbacks : public PPCallbacks {
break;
}
}
void PragmaDiagnostic(SourceLocation Loc, StringRef Namespace,
diag::Severity Mapping, StringRef Str) override {
// If one of the analysis-based diagnostics was enabled while processing
// a function, we want to note it in the analysis-based warnings so they
// can be run at the end of the function body even if the analysis warnings
// are disabled at that point.
SmallVector<diag::kind, 256> GroupDiags;
diag::Flavor Flavor =
Str[1] == 'W' ? diag::Flavor::WarningOrError : diag::Flavor::Remark;
StringRef Group = Str.substr(2);

if (S->PP.getDiagnostics().getDiagnosticIDs()->getDiagnosticsInGroup(
Flavor, Group, GroupDiags))
return;

for (diag::kind K : GroupDiags) {
// Note: the cases in this switch should be kept in sync with the
// diagnostics in AnalysisBasedWarnings::getPolicyInEffectAt().
AnalysisBasedWarnings::Policy &Override =
S->AnalysisWarnings.getPolicyOverrides();
switch (K) {
default: break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So every new analysis-based warning needs to be added to this list right?

Correct, but thankfully we don't add those very often.

How weird are the consequences of forgetting it?

If you add it to AnalysisBasedDiagnostics.cpp and not to Sema.cpp, the analysis will work everywhere except for pragma regions like this. If you add it to Sema.cpp but not AnalysisBasicDiagnostics.cpp, the only time the analysis will work will be within pragma regions. So either is weird, but one seems more likely to slip through the cracks than the other.

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.)

Yeah, it would be nice if this could be tablegenned. But there are so few analysis-based warnings and there's no good mechanism to know when you forget to add the correct marking to the .td file that I can think of so far.

Probably out of scope for this patch.

Yeah, I'd prefer to punt on that. :-)

case diag::warn_unreachable:
case diag::warn_unreachable_break:
case diag::warn_unreachable_return:
case diag::warn_unreachable_loop_increment:
Override.enableCheckUnreachable = true;
break;
case diag::warn_double_lock:
Override.enableThreadSafetyAnalysis = true;
break;
case diag::warn_use_in_invalid_state:
Override.enableConsumedAnalysis = true;
break;
}
}
}
};

} // end namespace sema
Expand Down
8 changes: 7 additions & 1 deletion clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
83 changes: 83 additions & 0 deletions clang/test/Analysis/pragma-diag-control.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// RUN: %clang_cc1 -fsyntax-only -verify -Werror=unreachable-code-aggressive %s

// Test that analysis-based warnings honor #pragma diagnostic controls.

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; // 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 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; // 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 f() {
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 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

int h() {
#pragma clang diagnostic push
#pragma clang diagnostic error "-Wconsumed"
#pragma clang diagnostic ignored "-Wunreachable-code-aggressive"
#pragma clang diagnostic pop

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}}
}
Loading