Page MenuHomePhabricator

[analyzer] Evaluate all non-checker config options before analysis
ClosedPublic

Authored by Szelethus on Oct 25 2018, 5:28 AM.

Details

Summary

This patch contains very little logic but touches a lot of lines.

Now that we're confident that all non-checker configs are gathered, I went ahead and did some actual changes to how AnalyzerOptions works.

This patch is a proposal to evaluate all config options (and possibly emit warning about them) at the beginning of the analysis, rather then whenever a getter function is called for it. This is done by changing the generated Optional<TYPE> fields' accessibility to public, evaluating all options as soon as the ConfigTable is assembled, and removing generated getters, as are they are no longer necessary. Now, every single public function (except parseConfigs) is const, as should AnalyzerOptions be after options are parsed (coming soon™ in a followup patch).

The removal of those getters implied however that every single use of those getters had to be changed (Opts.shouldDoThat() -> Opts.ShouldDoThat.getValue()), and I got to simplify the def file. I clang-formatted it, and since I don't expect the format to change for a good long while, I think it should be clang-formatted in the future whenever.

The actual the actual implementation of warnings is coming soon™.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Oct 25 2018, 5:28 AM
Szelethus added a comment.EditedOct 25 2018, 5:58 AM

The reason why I removed the getter functions and went ahead with the gigantic refactor is that getter functions actually changed the state of AnalyzerOptions, as they were responsible with the initialization of each option. Now, in the .def file, not all options had getter functions, so I couldn't just get away with calling every getter function once. Besides, a separate identifier for a field name (NAME) and getter function name (CREATE_FN) didn't make much sense anyways.

Granted, NAME and CREATE_FN were redundant when I added them myself, but the idea there was to regenerate the exact same state AnalyzerOptions was in at the time.

lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
234

This check should and will be moved to parseConfigs in a followup patch.

Szelethus updated this revision to Diff 171094.Oct 25 2018, 7:43 AM
Szelethus edited the summary of this revision. (Show Details)

What are the feelings on moving some functions to the header file? That's the one thing I'm unsure about regarding this patch. I could've moved them to CompilerInvocation.cpp, but I since I plan on making ConfigTable private, CompilerInvocation needs to be a friend of AnalyzerOptions. Not that it's the end of the world, and similar classes (e.g.: LangOptions) work similarly.

The reason why I didn't do it straight away is that

  • it's nice that AnalyzerOptions handles configs on it's own.
  • if someone would like add a new config option and also add constraints (like it has to be an existing file) to that, facing the longer compilation times will be a burden anyways (because the .def file has to be modified), so I thought the maintenance cost of parseConfig won't be THAT bad,

but I'd concede to the fact that these aren't the greatest of concerns, though.

Polite ping. :)

NoQ added a comment.Nov 1 2018, 9:16 PM

I guess i'm running out of steam for today :)

Opts.shouldDoThat() -> Opts.ShouldDoThat.getValue()

Is this really unavoidable? Like, what makes them optional when they're always non-optional? Maybe just somehow prevent un-eagerly-initialized AnalyzerConfig from appearing anywhere in the program?

In D53692#1285091, @NoQ wrote:

I guess i'm running out of steam for today :)

Opts.shouldDoThat() -> Opts.ShouldDoThat.getValue()

Is this really unavoidable? Like, what makes them optional when they're always non-optional? Maybe just somehow prevent un-eagerly-initialized AnalyzerConfig from appearing anywhere in the program?

Thats a fair point, the reason why I kept Optional<T> is the extra safety -- it took a good couple hours to make this patch, and it didn't took days thanks to the hasValue() assert, since, well, how macros get expanded is always a surprise. But sure, ideally it shouldn't be there.

Also, thank you so much for the feedback on this, and almost every single other patch I made!

xazax.hun added inline comments.Nov 2 2018, 4:34 AM
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
234

Just make sure to commit the patches together to not regress this check.

NoQ added inline comments.Nov 5 2018, 6:41 PM
test/Analysis/analyzer-config.c
3–4

The code is no longer necessary, right? All options are now initialized eagerly and there's no need to analyze any code to get them initialized.

test/Analysis/analyzer-config.cpp
1

Aaand this test should probably be removed entirely.

Szelethus updated this revision to Diff 172856.Nov 6 2018, 2:25 PM
Szelethus set the repository for this revision to rC Clang.
  • Moved initializer functions to CompilerInvocation.cpp, like every other Options-like class.
  • The fields for options are no longer Optional
  • Fixed the test files
Szelethus marked 2 inline comments as done.Nov 6 2018, 2:26 PM
Szelethus added inline comments.
test/Analysis/analyzer-config.c
3–4

Correct :)

NoQ accepted this revision.Nov 9 2018, 2:52 PM

Thanks, this is really wonderful.

test/Analysis/analyzer-config.cpp
1

I think this one's not actually done^^

This revision is now accepted and ready to land.Nov 9 2018, 2:52 PM
Szelethus marked an inline comment as done.Nov 9 2018, 3:05 PM

Thanks! This patch was the last for a while directly related to non-checker config options, I'm currently stuck on them as I unearthed countless bugs that I have to fix beforehand. (Did you know that clang unit tests do not run with check-clang-analysis?)

test/Analysis/analyzer-config.cpp
1

Uh, right, I misunderstood what you meant :D

NoQ added a comment.Nov 9 2018, 3:07 PM

Did you know that clang unit tests do not run with check-clang-analysis?

Well, *some* unit tests definitely do run under check-clang-analysis.

NoQ added a comment.Nov 29 2018, 6:45 PM
In D53692#1293781, @NoQ wrote:

Did you know that clang unit tests do not run with check-clang-analysis?

Well, *some* unit tests definitely do run under check-clang-analysis.

I think they're actually compiled but indeed not run. Ugh. I guess it should be fixed.

In D53692#1313956, @NoQ wrote:
In D53692#1293781, @NoQ wrote:

Did you know that clang unit tests do not run with check-clang-analysis?

Well, *some* unit tests definitely do run under check-clang-analysis.

I think they're actually compiled but indeed not run. Ugh. I guess it should be fixed.

Uhh, yea, forgot to send my comment with the same observation. I tried to fix it myself, but a day and a half of CMake made me appreciate even preprocessor macros more then ever.

This revision was automatically updated to reflect the committed changes.