This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] CheckerContext: Make the Preprocessor available
ClosedPublic

Authored by Charusso on Nov 1 2019, 12:49 PM.

Details

Summary

This patch hooks the Preprocessor trough BugReporter to the
CheckerContext so the checkers could look for macro definitions.

Diff Detail

Event Timeline

Charusso created this revision.Nov 1 2019, 12:49 PM
Charusso marked an inline comment as done.Nov 1 2019, 12:51 PM

It is needed for my work, and also I have seen other checkers in need of that.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
196

I have dropped the const because the methods of Preprocessor are non-const because of the BumpPtrAllocator.

NoQ accepted this revision.Nov 1 2019, 1:24 PM

We probably need this because every time we try to deal with macros we struggle quite a bit.

I'm not sure though - because we somehow survived without this for like 10 years. Eg. BugReporterVisitors.cpp:

250 static bool isFunctionMacroExpansion(SourceLocation Loc,
251                                 const SourceManager &SM) {
252   if (!Loc.isMacroID())
253     return false;
254   while (SM.isMacroArgExpansion(Loc))
255     Loc = SM.getImmediateExpansionRange(Loc).getBegin();
256   std::pair<FileID, unsigned> TLInfo = SM.getDecomposedLoc(Loc);
257   SrcMgr::SLocEntry SE = SM.getSLocEntry(TLInfo.first);
258   const SrcMgr::ExpansionInfo &EInfo = SE.getExpansion();
259   return EInfo.isFunctionMacroExpansion();
260 }

I'd love to see some actual use before committing.

This revision is now accepted and ready to land.Nov 1 2019, 1:24 PM
NoQ added a comment.Nov 1 2019, 1:24 PM

Also path-insensitive checkers will probably benefit a lot more from this info.

In D69731#1730784, @NoQ wrote:

I'm not sure though - because we somehow survived without this for like 10 years. Eg. BugReporterVisitors.cpp: [...]
I'd love to see some actual use before committing.

"Teaser":

const Preprocessor &PP = C.getPreprocessor();
Optional<bool> WantSafeFunctions;
  
if (PP.isMacroDefined("__STDC_LIB_EXT1__")) {
  MacroDefinition MD = PP.getMacroDefinition("__STDC_WANT_LIB_EXT1__");
  if (const MacroInfo *MI = MD.getMacroInfo()) {
    const Token &T = MI->tokens().back();
    StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
    llvm::APInt IntValue;
    ValueStr.getAsInteger(10, IntValue);
    WantSafeFunctions = IntValue.getZExtValue();
  }
}
NoQ added a comment.Nov 1 2019, 2:40 PM

It sounds like the code is querying the temporary internal state of the preprocessor which is supposed to be queried during preprocessing, not after it. Say, PP.isMacroDefined("__STDC_LIB_EXT1__") clearly depends on the location, as a macro can be un-defined and re-defined as many times as possible. But it doesn't receive the location as an argument, so it's probably implied to be "the location that the preprocessor is currently preprocessing"(?) Like, i've no idea how does the Preprocessor class work, but the code clearly looks too primitive to solve the problem. Clang-Tidy's PPCallbacks subsystem looks much more realistic.

In D69731#1730899, @NoQ wrote:

Clang-Tidy's PPCallbacks subsystem looks much more realistic.

I wanted to make it available from the AnalysisManager so that I can obtain the macro definitions once before the analysis starts. Nor the Clang Tidy's PPCallback, nor the AnalysisManager could retrieve the necessary information at that point in the analysis. However, at that point when we analyze something all the necessary information is available. Most of the Tidy checkers grab their own PP reference to use it later on. The Tidy devs agree to retrieve such information even in the main check() call which going to rerun in every TranslationUnit. So even it is so primitive, there is no better solution as I know. I hope we could find something better / cache it. At the moment this code-snippet I have just showed you runs in every evalCall().

I am thinking of a callback which is something like:

void checkBeginAnalysis(const Decl *D, BugReporter &BR) const;

so it would be easy and meaningful to have a place for the Preprocessor logic. Do you think it would worth it?

Charusso updated this revision to Diff 227557.Nov 1 2019, 6:41 PM
  • Use less const, it prevented the usage of non-const methods.

I am thinking of a callback which is something like:

void checkBeginAnalysis(const Decl *D, BugReporter &BR) const;

so it would be easy and meaningful to have a place for the Preprocessor logic. Do you think it would worth it?

Well, it made me mad to create setters / public fields and to rely on only the AnalysisManager. I am going to publish my first real checker, and it turns out being cool: D69745

This revision was automatically updated to reflect the committed changes.