Page MenuHomePhabricator

Check possible warnings on global initializers for reachability
Needs ReviewPublic

Authored by Nathan-Huckleberry on Jun 27 2019, 12:29 PM.

Details

Summary

Create CFG for initializers and perform analysis based warnings on global variables

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 12:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pirama added inline comments.Jul 1 2019, 10:30 AM
clang/include/clang/Sema/AnalysisBasedWarnings.h
117

Fix indentation.

clang/lib/Sema/AnalysisBasedWarnings.cpp
2002

Should PossiblyUnreachableDiags be const?

Also, fix indentation.

2010

How about returning early if PossiblyUnreachableDiags here is empty? That'll avoid putting the entire logic of this function in the true branch.

2026

getCFGReachablityAnalysis has a typo. It's missing an 'i'. Consider fixing this in a separate patch.

2046

Rewrite this as the else clause for if (AC.getCFG())? In the current structure, it's not immediately clear that flushDiagnostics is called iff AC.getCFG() fails to return a valid CFG.

Upon further reading, this seems to be refactored from another function below so it probably makes sense to leave it as-is.

nickdesaulniers added inline comments.
clang/include/clang/Sema/AnalysisBasedWarnings.h
104

git-clang-format HEAD~<number of commits>

The formal parameter should be abbreviated, maybe PUD?

116

How about making this a proper method of AnalysisBasedWarnings rather than a free floating function that's only ever called from other methods of AnalysisBasedWarnings? That way you don't have to pass in a Sema.

clang/lib/Analysis/AnalysisDeclContext.cpp
124

The const_cast doesn't look necessary here. Is it?

clang/lib/Sema/AnalysisBasedWarnings.cpp
2007

having the full namespace spelled out here in the definition smells funny. Is there a pair of namespace blocks below for clang and sema where the definition of emitPossiblyUnreachableDiags could be moved into?

Actually looking at the file, I see you simply matched the existing style, but line 49 currently has a using statement that should inject the clang namespace into the current scope. That's why you see sema:: used in other places in this translation unit without the clang namespace prefix. The whole file should remove the clang:: prefixes or additionally replace the using statement with explicit namespace blocks.

2257

PUD

2272

If you make emitPossiblyUnreachableDiags then it doesn't need all the prefixes.

clang/lib/Sema/SemaDeclCXX.cpp
347

Use punctuation in your comments.

clang/lib/Sema/SemaExpr.cpp
16679

Punctuation.

clang/test/Sema/warn-unreachable-warning-var-decl.cpp
40–41

ok and also ok can probably be removed?

clang/lib/Analysis/AnalysisDeclContext.cpp
124

VD->getInit returns a const Expr *. In order to remove the const_cast I would need to make getBody() const and every call to getBody(). The change required to add const seemed too large to be included in this patch.

  • Small functional and formatting changes
Nathan-Huckleberry marked 9 inline comments as done.Jul 3 2019, 4:36 PM
nickdesaulniers added inline comments.Jul 3 2019, 4:41 PM
clang/include/clang/Sema/AnalysisBasedWarnings.h
95

Methods should be UpperCamelCased.

112

UpperCamelCase

clang/lib/Sema/SemaExpr.cpp
16672

does this need the clang:: qualifier?

16701

does this need clang::?

  • Stylistic fixes of function names and removal of namespace prefixes
Nathan-Huckleberry marked 4 inline comments as done.Jul 3 2019, 4:52 PM
Nathan-Huckleberry marked an inline comment as done.Jul 8 2019, 10:13 AM
rsmith added inline comments.Sun, Jul 21, 4:53 AM
clang/lib/Sema/SemaDeclCXX.cpp
340

We may need to delay the diagnostics here until the default argument is *used*: if a default argument references a template instantiation, the instantiation is not performed until that point, which may mean that our semantic checking can't complete correctly until use.

clang/lib/Sema/SemaExpr.cpp
16694

It's not correct to assume that the last declaration in the context is the right one to be considering here. Instead, you should track whether you're in a variable initializer (and for what) in Sema. Examples:

int x = ([]{}(), x); // in C++; last decl is the lambda
int y = (struct Q { int n; }){y}; // in C; last decl is the struct
  • Add tracking of declaration of initializers in Sema.
clang/include/clang/Sema/Sema.h
1689

might be nice to return the result, but maybe YAGNI?

clang/include/clang/Sema/Sema.h
1698

Does:
return DeclForInitializer.empty() ? DeclForInitializer.back() : nullptr;
fit on one line?

clang/lib/Sema/AnalysisBasedWarnings.cpp
2036

I'm not sure that analyzed, block, or cra follow the naming conventions used throughout the codebase.

clang/lib/Sema/AnalysisBasedWarnings.cpp
2009

is clang namespace required here?

Nathan-Huckleberry marked an inline comment as done.Mon, Aug 5, 1:24 PM
Nathan-Huckleberry marked an inline comment as done.Mon, Aug 5, 4:38 PM
Nathan-Huckleberry added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
340

Currently this patch really only works with globals. There are many places where the following check is made instead of calling ParseInitializer. These should probably be rewritten into a single function and do something similar with pushing/popping declarations. Not sure if that change should be made in this patch or not.

if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace)) {
            Diag(Tok, diag::warn_cxx98_compat_generalized_initializer_lists);
            DefArgResult = ParseBraceInitializer();
          } else
            DefArgResult = ParseAssignmentExpression();
rsmith added inline comments.Thu, Aug 15, 4:45 PM
clang/include/clang/Sema/Sema.h
1684–1686

I don't think a simple list of these can work. Consider a case like:

auto x = [] {
  // something with a runtime diagnostic
};

Here, we'll have a DeclForInitializer set, so we'll suppress warnings on the body of the lambda, but we shouldn't: the body of the lambda is a completely different evaluation context from the initialization of the variable x.

Can you store the declaration on the ExpressionEvaluationContextRecord instead of storing a list of them directly on Sema? (You shouldn't need a list there, just a single pointer should work, since we can't parse two nested initializers without an intervening evaluation context.)

clang/lib/Sema/AnalysisBasedWarnings.cpp
2015

Please capitalize the names of variables.

clang/lib/Sema/SemaExpr.cpp
16678–16695

Please get rid of the pre-existing hacky use of ManglingContextDecl here and move this into the getDeclForInitializer block below.

  • Use ExprEvalContext and remove mangling context code
Nathan-Huckleberry marked 4 inline comments as done.Fri, Aug 16, 3:52 PM
Nathan-Huckleberry added inline comments.
clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
360

These new warnings seem valid.