Page MenuHomePhabricator

[analyzer][NFC] Don't allow dependency checkers to emit diagnostics
AcceptedPublic

Authored by Szelethus on Apr 14 2020, 9:27 AM.

Details

Summary

The thrilling conclusion to the barrage of patches I uploaded lately! This is a big milestone towards the goal set out in http://lists.llvm.org/pipermail/cfe-dev/2019-August/063070.html. I hope to accompany this with a patch where the a coreModeling package is added, from which package diagnostics aren't allowed either, is an implicit dependency of all checkers, and the core package for the first time can be safely disabled.

Diff Detail

Event Timeline

Szelethus created this revision.Apr 14 2020, 9:27 AM
balazske added inline comments.Apr 15 2020, 12:31 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
153

Probably too much of assert here (but it works)? (There is not a way to get the dependency data "dynamically", like "Checker->isDependentOn(OtherChecker)"?)

Szelethus marked an inline comment as done.Apr 15 2020, 1:30 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
153

Wow, you're totally correct, I didn't even consider that. Indeed, this doesn't work with plugins (or checkers from unit tests), but it should be doable, since CheckerManager owns CheckerRegistry (D75360). This is kind of ironic, considering how much I tend to block patches based on this :^)

Szelethus marked an inline comment as done.Apr 15 2020, 2:09 AM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
153

Oh, to actually answer your question. The checker dependency information is stored in CheckerRegistry in this small struct:

struct CheckerInfo {
  enum class StateFromCmdLine {
    // This checker wasn't explicitly enabled or disabled.
    State_Unspecified,
    // This checker was explicitly disabled.
    State_Disabled,
    // This checker was explicitly enabled.
    State_Enabled
  };

  InitializationFunction Initialize = nullptr;
  ShouldRegisterFunction ShouldRegister = nullptr;
  StringRef FullName;
  StringRef Desc;
  StringRef DocumentationUri;
  CmdLineOptionList CmdLineOptions;
  bool IsHidden = false;
  StateFromCmdLine State = StateFromCmdLine::State_Unspecified;

  ConstCheckerInfoList Dependencies;
  //...
};

So the nice solution should make this info a bit more visible.

martong added inline comments.Apr 15 2020, 5:57 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
153

I have no serious bad feelings with the macro solution, but if you can come up with a better structure then that would be so cool! :)

Szelethus updated this revision to Diff 262387.May 6 2020, 8:23 AM

Make the solution handle dependencies from plugins as well via CheckerRegistry.

Szelethus marked 3 inline comments as done.May 6 2020, 8:24 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2126–2130

I'll put this in the function call the release bots wont complain.

martong accepted this revision.May 21 2020, 4:52 AM

LGTM!

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2132

Oh yeah! :)

This revision is now accepted and ready to land.May 21 2020, 4:52 AM
This revision was automatically updated to reflect the committed changes.
saugustine added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
43

Unfortunately, this include creates a circular dependency from StaticAnalyzer/Core to StaticAnalyzer/Frontend back to StaticAnalyzer/Core.

I'm hoping to figure out a fix that doesn't involve reverting it, but if you have a quick fix, that would be really great.

Szelethus marked an inline comment as done.Fri, Jun 12, 11:33 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
43

Yup, this is a common theme with these changes, the divide in between the analyzer's libraries isn't too well defined.

CheckerRegistry definitely belongs to the frontend, and BugReporter isn't really moving anywhere either. The registry is responsible for parsing the command line arguments (which checker is enabled, values of checker options, resolving dependencies, etc), but this information is really desired in the rest of the analyzer as well.

I think the obvious (to me, that is) solution would be to move the containers to the Core library, and leave the rest of the logic in the frontend. While I dare claiming that I have great knowledge about this part of the codebase (its usually just me maintaining it), I would be more comfortable doing that without needing to rush the solution :)

An uncomfortable hack would be to simply implement the new functions in the frontend library -- but the former solution better describes where the data CheckerRegistry holds should lie.

I'll revert this. Thanks!

This revision is now accepted and ready to land.Fri, Jun 12, 12:52 PM