[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 11 inline comments as done.
NoQ added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:75
 /// individual bug reports.
 class BugReport : public llvm::ilist_node {
 public:

gribozavr wrote:
> Szelethus wrote:
> > Shouldn't we make this an abstract class?
> I'm not sure that intrusive linked list is the right data structure for the 
> job. I'd personally put bug reports into a vector and make a custom data 
> structure if a vector becomes a performance problem.
> Shouldn't we make this an abstract class?

Mmm, yeah, moved virtual methods from `BugReport` to `BasicBugReport` whenever 
`PathSensitiveBugReport` immediately overrides them and it looks much better 
now!

> I'm not sure that intrusive linked list is the right data structure for the 
> job.

Me neither, but it seems to be orthogonal to this patch, and this patch is 
already huge, so i'll do this in a follow-up patch, ok?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:122
+  /// Get the location on which the report should be uniqued.
+  virtual PathDiagnosticLocation getUniqueingLocation() const {
+return Location;

gribozavr wrote:
> Where can I read about the uniqueing logic? Does it mean that out of two bug 
> reports with the same location only one gets displayed, regardless of other 
> properties?
Added some comments ^^



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:126-135
+  /// Get the declaration containing the uniqueing location.
+  virtual const Decl *getUniqueingDecl() const {
+return DeclWithIssue;
+  }
+
+  /// Return the canonical declaration, be it a method or class, where
+  /// this issue semantically occurred.

gribozavr wrote:
> I don't think `getUniqueingDecl()` and `getDeclWithIssue()` make sense for 
> most ClangTidy checkers.
`getDeclWithIssue()` corresponds to the "Function/Method" column we have in our 
scan-build index page:

{F9881207}

This column is not super important but probably kinda nice to have. I don't 
mind leaving it empty for clang-tidy checks, or possibly auto-detect it post 
factum when possible (e.g., find the smallest decl in which the warning 
location is contained).

`getUniqueingDecl()` is, as far as i understand, only currently used by the 
issue hash mechanism which governs deduplication across translation units 
(e.g., we emit a warning against the same header in multiple translation units 
- it's vital for the static analyzer because it's not uncommon for an execution 
path that starts in the main file to end in a header). And even then, it's only 
present in plist output. I'm not convinced it's useful at all. It's likely that 
we can remove it.

Also clang-tidy checks wouldn't need to specify their `UniqueingDecl` manually 
as it silently defaults to their `DeclWithIssue`.

So basically we have these two methods on the base class for bureaucratic 
reasons: our existing algorithms handle all kinds of warnings uniformly based 
on these computed properties of bug reports. I think we should be perfectly 
fine if some kinds of bug reports return null from them, indicating that "this 
property doesn't make sense for them". We could instead reinvent protocols on 
top of our custom RTTI ("does this warning conform to `Uniqueable`? if so, i'll 
try to unique it"), but i don't think it's worth the complexity.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:159
+  // FIXME: Instead of making an overload, we could have default-initialized
+  // Ranges with {}, however it crashes the MSVC 2013 compiler.
+  void addNote(StringRef Msg, const PathDiagnosticLocation ) {

gribozavr wrote:
> We don't care about MSVC 2013 now.
Woohoo!



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:186
+  /// ranges.
+  void addRange(SourceRange R) {
+assert((R.isValid() || Ranges.empty()) && "Invalid range can only be used "

gribozavr wrote:
> Ranges should be associated with a message.
Mmm, what do you mean?

Currently these ranges are attached to the warning message, path note messages 
can't have ranges, and extra path-insensitive notes can have a separate set of 
ranges attached to them by passing them through `addNote()`.


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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

Sorry for the silly comments, but my main point is, I guess, that I don't quite 
understand the design towards which you are refactoring, and to meaningfully 
review patches I need to be on the same page.




Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:75
 /// individual bug reports.
 class BugReport : public llvm::ilist_node {
 public:

Szelethus wrote:
> Shouldn't we make this an abstract class?
I'm not sure that intrusive linked list is the right data structure for the 
job. I'd personally put bug reports into a vector and make a custom data 
structure if a vector becomes a performance problem.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:122
+  /// Get the location on which the report should be uniqued.
+  virtual PathDiagnosticLocation getUniqueingLocation() const {
+return Location;

Where can I read about the uniqueing logic? Does it mean that out of two bug 
reports with the same location only one gets displayed, regardless of other 
properties?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:126-135
+  /// Get the declaration containing the uniqueing location.
+  virtual const Decl *getUniqueingDecl() const {
+return DeclWithIssue;
+  }
+
+  /// Return the canonical declaration, be it a method or class, where
+  /// this issue semantically occurred.

I don't think `getUniqueingDecl()` and `getDeclWithIssue()` make sense for most 
ClangTidy checkers.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:159
+  // FIXME: Instead of making an overload, we could have default-initialized
+  // Ranges with {}, however it crashes the MSVC 2013 compiler.
+  void addNote(StringRef Msg, const PathDiagnosticLocation ) {

We don't care about MSVC 2013 now.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:174
+  ///  This location is used by clients rendering diagnostics.
+  virtual PathDiagnosticLocation getLocation(const SourceManager ) const {
+assert(Location.isValid());

Another location-related method... I guess I would appreciate a more abstract 
writeup about what BugReport's data model is (in its doc comment).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:186
+  /// ranges.
+  void addRange(SourceRange R) {
+assert((R.isValid() || Ranges.empty()) && "Invalid range can only be used "

Ranges should be associated with a message.


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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:75
 /// individual bug reports.
 class BugReport : public llvm::ilist_node {
 public:

Shouldn't we make this an abstract class?


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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:223
+  using visitor_iterator = VisitorList::iterator;
+  using visitor_range = llvm::iterator_range;
+

Szelethus wrote:
> I think I added a visitor range not long ago?
Yup, i just moved it from `BugReport` to `PathSensitiveBugReport`.
(hint: this yellow bar on the left of the insertion indicates that, you can 
also hover it to see where was the code moved from)


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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

You got me convinced.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:223
+  using visitor_iterator = VisitorList::iterator;
+  using visitor_range = llvm::iterator_range;
+

I think I added a visitor range not long ago?


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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66572#1640518 , @Szelethus wrote:

> Super high level question: `CheckerManager` knows whether a checker, and I 
> suspect a checker callback is path sensitive or not, do you think we can 
> automate this decision (whether the bug report is path sensitive of 
> syntactic)? For the purposes of clang-tidy, can we say such a thing that if 
> we can't prove a bug report to be path sensitive, it will not be that?


I want to reserve the ability for path-sensitive checkers to occasionally emit 
path-insensitive reports. This is due to my pipe dream of making a poor man's 
data flow engine by simply trusting ExprEngine when it says that it has 
explored all execution paths through a function. Cf. `UnreachableCodeChecker`, 
`TestAfterDivZeroChecker`. There are no other reasons because if we're not 
presenting a path we must admit that the problem happens on all paths, which 
implies some sort of must-problem, which implies no proper symbolic execution.

Maybe also we can consider reporting path-insensitive reports when we are about 
to emit a path-sensitive report but look at the AST at the last minute and it 
turns out that the problem is entirely local.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Super high level question: `CheckerManager` knows whether a checker, and I 
suspect a checker callback is path sensitive or not, do you think we can 
automate this decision (whether the bug report is path sensitive of syntactic)? 
For the purposes of clang-tidy, can we say such a thing that if we can't prove 
a bug report to be path sensitive, it will not be that?




Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h:56
 
-class RefCountReport : public BugReport {
+class RefCountReport : public PathSensitiveBugReport {
 protected:

NoQ wrote:
> I'm not pointing any fingers...
Mhm, I can see that :^)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66572: [analyzer] BugReporter Separation Ep.I.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 6 inline comments as done.
NoQ added a comment.

Most of the patch is boring but here are a few places that i wanted attract 
attention to.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:225
+
+protected:
+  /// The ExplodedGraph node against which the report was thrown. It 
corresponds

Yes, we allow inheriting from these specific `BugReport` sub-classes, because 
there turned out to be one checker that was using it.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h:56
 
-class RefCountReport : public BugReport {
+class RefCountReport : public PathSensitiveBugReport {
 protected:

I'm not pointing any fingers...



Comment at: 
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h:77
 
 class RefLeakReport : public RefCountReport {
   const MemRegion* AllocBinding;

...and still not pointing any fingers.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2713
+
+  BugReporter::emitReport(std::move(R));
+}

I vaguely remember that this pattern is frowned upon. Does anybody want me to 
extract this into a common non-virtual function?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2983-2984
+  if (isa(exampleReport))
+return BugReporter::generateDiagnosticForConsumerMap(exampleReport,
+ consumers, 
bugReports);
 

Same question about extracting into a non-virtual function.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2993-2996
+  // Avoid copying the whole array because there may be a lot of reports.
+  ArrayRef convertedArrayOfReports(
+  reinterpret_cast(&*bugReports.begin()),
+  reinterpret_cast(&*bugReports.end()));

//*crosses fingers*//


Repository:
  rC Clang

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

https://reviews.llvm.org/D66572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits