[PATCH] D105637: [clang][Analyzer] Add symbol uninterestingness to bug report.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2260-2261
+  InterestingSymbols.erase(sym);
+  if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
+    markNotInteresting(meta->getRegion());
+}
----------------
NoQ wrote:
> 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.
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2260-2261
+  InterestingSymbols.erase(sym);
+  if (const auto *meta = dyn_cast<SymbolMetadata>(sym))
+    markNotInteresting(meta->getRegion());
+}
----------------
balazske wrote:
> NoQ wrote:
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105637/new/

https://reviews.llvm.org/D105637