Page MenuHomePhabricator

[CTU][Analyzer]Add DisplayCTUProgress analyzer switch
ClosedPublic

Authored by martong on Nov 30 2018, 10:12 AM.

Details

Summary

With a new switch we may be able to print to stderr if a new TU is being loaded
during CTU. This is very important for higher level scripts (like CodeChecker)
to be able to parse this output so they can create e.g. a zip file in case of
a Clang crash which contains all the related TU files.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Nov 30 2018, 10:12 AM
Szelethus requested changes to this revision.EditedNov 30 2018, 10:51 AM

The idea is great!

I think this should rather be an -analyzer-config flag, since the actual analysis changes with a new output (refer to AnalyzerOption's doxygen comments). Please note that I'm about to land some patches that modifies AnalyzerOptions.def quite a bit, but rebasing shouldn't be too bad anyways.

edit: They are now in trunk.

Also, almost everywhere CTU is capitalized, so I guess it should be in the field name too.

test/Analysis/ctu-main.cpp
6 ↗(On Diff #176159)

I think these RUN lines would really benefit from introducing line breaks.

This revision now requires changes to proceed.Nov 30 2018, 10:51 AM

Hi Gabor,
In addition to Umann remarks, there is a small comment inline.

lib/CrossTU/CrossTranslationUnit.cpp
239 ↗(On Diff #176159)

I think we can remove parens from the message.

Szelethus added a subscriber: NoQ.Dec 1 2018, 3:25 PM

Also, AnalyzerOptions.def was recently clan-formatted, feel free to run it again after the changes you make in it.

martong updated this revision to Diff 176581.Dec 4 2018, 2:46 AM
martong marked 4 inline comments as done.
  • Rename AnalyzerDisplayCtuProgress to Opts.AnalyzerDisplayCTUProgress
  • Change the CTU progress message

Also, almost everywhere CTU is capitalized, so I guess it should be in the field name too.

Ok, I renamed it to have CTU all capitalized.

lib/CrossTU/CrossTranslationUnit.cpp
239 ↗(On Diff #176159)

Ok, I removed the parens. And also changed the text from CTU loaded AST for source file to CTU loaded AST file since we dump the path to the AST, not the path to the source file from which we generated the AST file.

test/Analysis/ctu-main.cpp
6 ↗(On Diff #176159)

Yes, I agree. Unfortunately, I could not figure out how to break them. Using a simple "\" at around the 80th column gives Test has unterminated run lines (with '\'). If I use block comments with "\" the same happens. If I use block comments and don't use the "\" then the second line is not interpreted. Is it really possible to break RUN lines? I could not find anything about that in the online docs.

martong updated this revision to Diff 176582.Dec 4 2018, 2:48 AM
  • Change the CTU progress message in the test too
martong updated this revision to Diff 176590.Dec 4 2018, 3:14 AM
martong marked an inline comment as done.
  • Break long RUN line
test/Analysis/ctu-main.cpp
6 ↗(On Diff #176159)

Oh, I just have found your other comment to the other patch. So yes, it is indeed possible to break this line. I updated the patch accordingly. The other long lines which are already there I am going to change in an independent patch: (https://reviews.llvm.org/D55129).

Szelethus added inline comments.Dec 4 2018, 4:41 AM
test/Analysis/ctu-main.cpp
6 ↗(On Diff #176159)

Is there any particular reason why we use %clang_cc1 -analyze as opposed to %clang_analyze_cc1? If not, lets change it.

By the way, thanks! This looks neat.

Szelethus requested changes to this revision.Dec 4 2018, 5:51 AM

The code not directly related to adding the actual flag looks great, but this still should be an -analyzer-config option.

This revision now requires changes to proceed.Dec 4 2018, 5:51 AM

Having an analyzer config option makes sense.

martong updated this revision to Diff 176653.Dec 4 2018, 8:51 AM
  • Use clang_analyze_cc1
  • Change to be an analyzer config option

should be an -analyzer-config option.

Ok, just changed it to be.

Szelethus accepted this revision.Dec 4 2018, 10:17 AM

Thanks! LGTM! :)

This revision is now accepted and ready to land.Dec 4 2018, 10:17 AM
xazax.hun accepted this revision.Dec 7 2018, 5:13 AM

While Static Analyzer is the only client of CTU library at the moment, we might have more in the future. I would not use the phrase ANALYZE in the log message. Once this is resolved the rest looks good.

martong marked an inline comment as done.Dec 7 2018, 6:32 AM

While Static Analyzer is the only client of CTU library at the moment, we might have more in the future. I would not use the phrase ANALYZE in the log message. Once this is resolved the rest looks good.

Ok, I removed the "ANALYZE " prefix from the log message.

martong updated this revision to Diff 177204.Dec 7 2018, 6:33 AM
  • Remove 'ANALYZE ' prefix from the log message
This revision was automatically updated to reflect the committed changes.