Page MenuHomePhabricator

[analyzer] Explain why analyzer report is not generated (fix for PR12421).
ClosedPublic

Authored by ayartsev on Jul 18 2016, 6:07 PM.

Details

Summary

Hi all,

Currently if the path diagnostic consumer (e.g HTMLDiagnostics and PlistDiagnostics) do not support cross file diagnostics then the path diagnostic report is just silently omitted in the case of cross file diagnostics. If the analyzer finds an issue the missing report looks like a Clang bug as an issue is successfully reported to stdout but no report is generated.
The patch adds a little verbosity to Clang in the case considered above.
PR12421 is devoted to the issue.
Please review!
Tried to write tests that would cover warnings inside the 'for' loop, but failed. Any ideas?

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 64431.Jul 18 2016, 6:07 PM
ayartsev retitled this revision from to [analyzer] Explain why analyzer report is not generated (fix for PR12421)..
ayartsev updated this object.
ayartsev added reviewers: zaks.anna, dcoughlin.
ayartsev added a subscriber: cfe-commits.
asl added a subscriber: asl.Jul 20 2016, 5:14 AM
zaks.anna added inline comments.Jul 22 2016, 10:39 AM
test/Analysis/PR12421.c
11 ↗(On Diff #64431)

We should use the name of the diagnostic consumer here - that will only be legible for the developers working on the attic analyzer core.

test/Analysis/PR12421.h
1 ↗(On Diff #64431)

Please. do not use the PR as a file name. Use the purpose of the test instead,

zaks.anna requested changes to this revision.Jul 22 2016, 10:39 AM
zaks.anna edited edge metadata.
This revision now requires changes to proceed.Jul 22 2016, 10:39 AM
ayartsev updated this revision to Diff 65519.Jul 26 2016, 7:53 AM
ayartsev edited edge metadata.
ayartsev marked 2 inline comments as done.
ayartsev added inline comments.
test/Analysis/PR12421.c
11 ↗(On Diff #64431)

Done. As for me the name of the diagnostic consumer also makes the warning more clear and helpful for an ordinary user. From the consumer name he can see what report format is talked about and maybe change the scan-build (which setups the '-analyzer-output' frontend option internally) options. Do you still want to remove the consumer name from the warning?

zaks.anna added inline comments.Jul 27 2016, 9:42 PM
test/Analysis/PR12421.c
11 ↗(On Diff #64431)

"HTMLDiagnostics" is not a name a user would be familiar with. You should use only familiar terms in diagnostics.

test/Analysis/PR12421.h
1 ↗(On Diff #64431)

this does not seem to be done.

ayartsev added inline comments.Jul 27 2016, 11:49 PM
test/Analysis/PR12421.c
11 ↗(On Diff #64431)

Ok.

test/Analysis/PR12421.h
1 ↗(On Diff #64431)

Did you see an updated diff? Or I'm probably missing something. Maybe you mean something like this: check-diag-cross-file-boundaries-no-report.* ?

zaks.anna added inline comments.Jul 29 2016, 5:37 PM
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
216

Can/should we be specific about what the user-specified output format is?

ayartsev added inline comments.Aug 5 2016, 4:46 AM
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
216

It's unable to extract information about user-specified output format from the "PathDiagnosticConsumer" interface. And this class seem to be too generic to contain "AnalyzerOptions" member or to have e.g. "pure virtual getOutputFormatName()".
So the only way I see to get info about output format is to use "PathDiagnosticConsumer::getName()".
Maybe it makes sense just to add a hint to use "--analyzer-output" driver option to change output format. However this option is not documented at all and is not displayed in clang help. What do you think?

Gentle ping.

zaks.anna added inline comments.Aug 31 2016, 3:11 PM
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
216

I think mentioning the option is the best option. What is that option called in scan-build?

ayartsev added inline comments.Aug 31 2016, 3:32 PM
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
216

scan-build (both perl and python versions) has two options: "-plist" and "-plist-html" that are translated to "-analyzer-output=plist" and "-analyzer-output=plist-html" frontend options respectively.

ayartsev added inline comments.Aug 31 2016, 3:37 PM
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
216

I suggest to document the "--analyzer-output" option and to mention this option in the warning.

zaks.anna added inline comments.Aug 31 2016, 4:38 PM
lib/StaticAnalyzer/Core/PathDiagnostic.cpp
216

That sounds good to me.

NoQ added a subscriber: NoQ.Aug 31 2016, 11:33 PM
ayartsev updated this revision to Diff 70772.Sep 8 2016, 5:52 PM
ayartsev edited edge metadata.

Updated the patch, added help entry for the "--analyzer-output" driver option. Please review.

zaks.anna accepted this revision.Sep 12 2016, 10:57 AM
zaks.anna edited edge metadata.
This revision is now accepted and ready to land.Sep 12 2016, 10:57 AM
ayartsev updated this revision to Diff 71056.Sep 12 2016, 2:43 PM
ayartsev edited edge metadata.

Updated the patch. Important change in Options.td was missing in the last patch + indentation fixed.
Still Ok to commit?

LGTM. Thanks.

ayartsev closed this revision.Oct 14 2016, 8:45 AM

Closed by r283499.