This is an archive of the discontinued LLVM Phabricator instance.

[clang][Analyzer] Add symbol uninterestingness to bug report.
ClosedPublic

Authored by balazske on Jul 8 2021, 8:49 AM.

Details

Summary

PathSensitiveBughReport has a function to mark a symbol as interesting but
it was not possible to clear this flag. This can be useful in some cases,
so the functionality is added.

Diff Detail

Event Timeline

balazske created this revision.Jul 8 2021, 8:49 AM
balazske requested review of this revision.Jul 8 2021, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 8:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Could not make a simple test for the change. This file

is what I could do, but it prints the needed text to the console. Is it possible to get the note texts in the program?

The original change D104300 contained these modifications but is currently not committed. The "uninterestingness" is really a separate patch that belongs to another change. For me it is not a problem is D104300 is committed, otherwise I would need this to continue with D104925.

NoQ added a comment.Jul 9 2021, 10:41 AM

Could not make a simple test for the change. This file

is what I could do, but it prints the needed text to the console. Is it possible to get the note texts in the program?

You could define your own diagnostic consumer in the unittest and intercept all the notes. But at this point i'd rather turn this into a LIT test by turning your checker mock into a debug.ExprInspection item and using -analyzer-output=text to test notes:

void foo(x) {
  clang_analyzer_noteIfInteresting(x); // no-note
  clang_analyzer_markNotInteresting(x);
  clang_analyzer_noteIfInteresting(x); // expected-note{{INTERESTING}}
  clang_analyzer_markInteresting(x);
  clang_analyzer_warnIfReached(); // expected-warning{{TRUE}}
}                                 // expected-note@-1{{TRUE}}
NoQ added a comment.Jul 9 2021, 12:33 PM

You could define your own diagnostic consumer in the unittest and intercept all the notes.

(you can find an example of this in D94476) (I can't believe I haven't landed these patches yet)

But at this point i'd rather turn this into a LIT test by turning your checker mock into a debug.ExprInspection item

(dunno how reusable it'll be, maybe unittest is indeed the way to go)

balazske updated this revision to Diff 357872.Jul 12 2021, 2:33 AM

Added the unit test.

balazske updated this revision to Diff 357873.Jul 12 2021, 2:35 AM

Removed "garbage" from end of test file.

NoQ added a comment.Jul 12 2021, 8:24 PM

Looks great, thanks for reusing the reusables!

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2262–2263

You're saying, e.g., "If string length is not interesting then neither is the string itself". Or, dunno, "If the raw pointer value is not interesting then neither is a smart pointer that was holding it".

I'm not sure about that. I'm pretty sure that no checkers are currently affected by this code but I still don't understand your point.

I don't understand the original code in markInteresting() either; do we have even one test to cover it?

Also note that what you've written is not a correct negation of the original code. The correct negation (that would keep the region-metadata relationship in sync as originally intended) would be "if the region loses interestingness, so does the metadata". Or it has to go both ways. I'm still not sure if any of this matters though.

Maybe we should eliminate these extra clauses entirely. If you're not willing to investigate whether this is all dead code or it actually does something, I'd like to suggest a "FIXME: Is this necessary?" (or something like that) both here and in the original code.

clang/unittests/StaticAnalyzer/Reusables.h
132

We already have VerifyDiagnosticConsumer. Maybe VerifyPathDiagnosticConsumer to put emphasis on path-sensitivity (which implies usefulness in unittests because VerifyDiagnosticConsumer operates when all diagnostics are already indistinguishably flattened) and on its similarity to VerifyDiagnosticConsumer?

balazske added inline comments.Jul 13 2021, 8:57 AM
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2262–2263

The metadata interestingness was added in rG735724fb1e78 long ago, there is not more information. All test pass if the code is removed (but it may have impacts on bug paths). Is it not the job of the checker to mark the interestingness of objects? I assume that the meaning of meta->getRegion() is checker dependent (the string or smart pointer or something totally different?) and the checker should know what to make interesting (or not). The markInteresting() has no test (what I could extend here), it may be implicitly tested by the other checker tests.

2262–2263

The mark of associated region as interesting may not work correct if there are multiple metadata symbols for the same region, which should be a possible case. Specially the remove of interestingness is not correct in this way. So I would remove the if (const auto *meta = dyn_cast<SymbolMetadata>(sym)) part.
A better approach can be to make the interestingness depend totally on the associated region for metadata symbol. If a symbol is metadata, its interestingness is that of the region. And only the region is inserted into the map. isInteresting can be updated for this purpose.

NoQ added inline comments.Jul 13 2021, 9:49 PM
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2262–2263

I assume that the meaning of meta->getRegion() is checker dependent

I completely agree with your reasoning. One way or another, that old commit didn't come with a test, or any explanation whatsoever. I think we have all the rights to question it.

balazske updated this revision to Diff 358545.Jul 14 2021, 1:54 AM

Rename of DiagnosticVerifyConsumer.
Changes related to handling of metadata symbols.

NoQ accepted this revision.Jul 14 2021, 10:03 AM

Amazing, thanks!

This revision is now accepted and ready to land.Jul 14 2021, 10:03 AM
This revision was landed with ongoing or failed builds.Jul 15 2021, 12:50 AM
This revision was automatically updated to reflect the committed changes.