Page MenuHomePhabricator

[analyzer] Insert checker options into AnalyzerOption::ConfigTable
Needs ReviewPublic

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

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
166

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
327

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

374

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.