Page MenuHomePhabricator

[analyzer] Insert checker options into AnalyzerOption::ConfigTable
ClosedPublic

Authored by Szelethus on Feb 7 2019, 1:22 PM.

Details

Summary

The more entries we have in AnalyzerOptions::ConfigTable, the more helpful debug.ConfigDumper is. With this patch, I'm pretty confident that it'll now emit the entire state of the analyzer, minus the frontend flags.

It would be nice to reserve the config table specifically to checker options only, as storing the regular analyzer configs is kinda redundant.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Feb 7 2019, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 1:22 PM
Szelethus marked an inline comment as done.Feb 7 2019, 1:25 PM
Szelethus added inline comments.
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
183 ↗(On Diff #185853)

alpha.security.MmapWriteExec:MmapProtRead expects it's option to be of a hexadecimal value -- as of now, using that option crashes.

From the doxygen:

llvm::StringRef::getAsInteger(unsigned Radix, T & Result) const

Parse the current string as an integer of the specified radix.

If Radix is specified as zero, this does radix autosensing using extended C rules: 0 is octal, 0x is hex, 0b is binary.

This revision is now accepted and ready to land.Feb 11 2019, 3:19 AM
Szelethus added a comment.EditedFeb 14 2019, 3:06 AM

Hmm, supplying the default value for AnalyzerOptions::getChecker*Option is not only redundant with this patch, but also extremely bug-prone. I should remove them too in the same breath.

Szelethus updated this revision to Diff 189107.Mar 3 2019, 5:24 PM

Remove the default value parameters from getChecker*Option, as they are no longer necessary. Note the changes to the unit test file: since the current thinking is that all options must've been validated by CheckerRegistry, if any irregularities are detected within those methods, we no longer return default values or even dream of a peaceful termination, as it clearly indicates a programming error, hence the removal of some test cases as asserts can't be tested.

This revision now requires review to proceed.Mar 3 2019, 5:24 PM
Szelethus planned changes to this revision.Mar 7 2019, 2:40 PM

Uhh, we still need to restore the default value -- I might end up splitting this patch a little further.

Szelethus updated this revision to Diff 190021.Mar 10 2019, 2:03 PM

Removed every piece of code not directly related to inserting checker option to the config table, moving it to a new patch. This essentially restores the revision to it's earliest state, except that it's even simpler: no changes are made to AnalyzerOptions.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
332 ↗(On Diff #190021)

Twine(CheckerFullName) + ":" + OptionName. However, since the constructor Twine(const StringRef &Str) is implicit, CheckerFullName + ":" + OptionName results the same and is more readable.

354 ↗(On Diff #190021)

Same as above.

Szelethus updated this revision to Diff 190628.Mar 14 2019, 7:46 AM

Add a test case for checker plugins.

This looks good to me. It is great to see this tested!

Did you consider separating the checker options from the non-checker options in the config dumper output? That would probably be easier to read.

I think this is good. Patch still marked as Needs review for some reason. 😦 Can we look up this blocking review thing? Perhaps this could be marked ready to roll once the dependency patch is ironed out.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
332 ↗(On Diff #190021)

This comment is Done.

NoQ added inline comments.May 13 2019, 2:31 PM
lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
314 ↗(On Diff #190991)

Mmm, const rvalue reference? Is this even a thing?

331 ↗(On Diff #190991)

Wait, how did you manage to move from a const &?

This type doesn't look very move-friendly in general.

test/Analysis/analyzer-config.c
1 ↗(On Diff #190991)

If i understand correctly, you should be able to simplify this line dramatically.

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2019, 2:27 AM
This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 2:27 AM

I committed but changed the moves to copies. Singel CheckerRegisrty::CmdLineOption is only filled with StringRefs, it shouldn't really matter.

lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
314 ↗(On Diff #190991)

Apparently :D