[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC361682: [analyzer] Add a prunable note for skipping vbase 
inits in subclasses. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61817?vs=201359=201364#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61817

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  lib/StaticAnalyzer/Core/CoreEngine.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/diagnostics/initializer.cpp

Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2501,7 +2501,9 @@
   if (Optional Msg = T->generateMessage(BRC, R)) {
 PathDiagnosticLocation Loc =
 PathDiagnosticLocation::create(PP, BRC.getSourceManager());
-return std::make_shared(Loc, *Msg);
+auto Piece = std::make_shared(Loc, *Msg);
+Piece->setPrunable(T->isPrunable());
+return Piece;
   }
 
   return nullptr;
Index: lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,25 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch
+  // and we're taking the path that skips virtual base constructors.
+  if (L.getSrc()->getTerminator().isVirtualBaseBranch() &&
+  L.getDst() == *L.getSrc()->succ_begin()) {
+ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+[](BugReporterContext &, BugReport &) -> std::string {
+  // TODO: Just call out the name of the most derived class
+  // when we know it.
+  return "Virtual base initialization skipped because "
+ "it has already been handled by the most derived class";
+}, /*IsPrunable=*/true));
+// Perform the transition.
+ExplodedNodeSet Dst;
+NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+if (!Pred)
+  return;
+  }
+
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 assert(L.getLocationContext()->getCFG()->getExit().empty() &&
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().isVirtualBaseBranch()) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -156,8 +156,6 @@
   /// The flag, which specifies the mode of inlining for the engine.
   InliningModes HowToInline;
 
-  NoteTag::Factory NoteTags;
-
 public:
   ExprEngine(cross_tu::CrossTranslationUnitContext , AnalysisManager ,
  SetOfConstDecls *VisitedCalleesIn,
@@ -399,7 +397,7 @@
   SymbolManager () { return SymMgr; }
   MemRegionManager () { return MRMgr; }
 
-  NoteTag::Factory () { return NoteTags; }
+  NoteTag::Factory () { return Engine.getNoteTags(); }
 
 
   // Functions for external checking of whether we have unfinished work
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -19,6 +19,7 @@
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/BlockCounter.h"
 #include 

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 201359.
NoQ added a comment.

Rebase.


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

https://reviews.llvm.org/D61817

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/diagnostics/initializer.cpp

Index: clang/test/Analysis/diagnostics/initializer.cpp
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/initializer.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-output=text \
+// RUN:   -verify %s
+
+namespace note_on_skipped_vbases {
+struct A {
+  int x;
+  A() : x(0) {} // expected-note{{The value 0 is assigned to 'c.x'}}
+  A(int x) : x(x) {}
+};
+
+struct B : virtual A {
+  int y;
+  // This note appears only once, when this constructor is called from C.
+  // When this constructor is called from D, this note is still correct but
+  // it doesn't appear because it's pruned out because it's irrelevant to the
+  // bug report.
+  B(): // expected-note{{Virtual base initialization skipped because it has already been handled by the most derived class}}
+A(1),
+y(1 / x) // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+  {}
+};
+
+struct C : B {
+  C(): // expected-note{{Calling default constructor for 'A'}}
+   // expected-note@-1{{Returning from default constructor for 'A'}}
+B() // expected-note{{Calling default constructor for 'B'}}
+  {}
+};
+
+void test_note() {
+  C c; // expected-note{{Calling default constructor for 'C'}}
+}
+
+struct D: B {
+  D() : A(1), B() {}
+};
+
+void test_prunability() {
+  D d;
+  1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+}
+} // namespace note_on_skipped_vbases
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().isVirtualBaseBranch()) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,25 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch
+  // and we're taking the path that skips virtual base constructors.
+  if (L.getSrc()->getTerminator().isVirtualBaseBranch() &&
+  L.getDst() == *L.getSrc()->succ_begin()) {
+ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+[](BugReporterContext &, BugReport &) -> std::string {
+  // TODO: Just call out the name of the most derived class
+  // when we know it.
+  return "Virtual base initialization skipped because "
+ "it has already been handled by the most derived class";
+}, /*IsPrunable=*/true));
+// Perform the transition.
+ExplodedNodeSet Dst;
+NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+if (!Pred)
+  return;
+  }
+
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 assert(L.getLocationContext()->getCFG()->getExit().empty() &&
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2501,7 +2501,9 @@
   if (Optional Msg = T->generateMessage(BRC, R)) {
 PathDiagnosticLocation Loc =
 PathDiagnosticLocation::create(PP, BRC.getSourceManager());
-return std::make_shared(Loc, *Msg);
+auto Piece = std::make_shared(Loc, *Msg);
+Piece->setPrunable(T->isPrunable());
+return Piece;
   }
 
   return nullptr;

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 199534.
NoQ marked an inline comment as done.
NoQ added a comment.

> What about "the most derived class"

Sounds great!

> It would be cool to say "by A" or something more simple and precise.

We can't do that in all cases (we don't necessarily see the constructor of the 
most derived class) but in some cases we might be able to do it (when we're 
sure that we actually see it). Added a TODO.


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

https://reviews.llvm.org/D61817

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/diagnostics/initializer.cpp

Index: clang/test/Analysis/diagnostics/initializer.cpp
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/initializer.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-output=text \
+// RUN:   -verify %s
+
+namespace note_on_skipped_vbases {
+struct A {
+  int x;
+  A() : x(0) {} // expected-note{{The value 0 is assigned to 'c.x'}}
+  A(int x) : x(x) {}
+};
+
+struct B : virtual A {
+  int y;
+  // This note appears only once, when this constructor is called from C.
+  // When this constructor is called from D, this note is still correct but
+  // it doesn't appear because it's pruned out because it's irrelevant to the
+  // bug report.
+  B(): // expected-note{{Virtual base initialization skipped because it has already been handled by the most derived class}}
+A(1),
+y(1 / x) // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+  {}
+};
+
+struct C : B {
+  C(): // expected-note{{Calling default constructor for 'A'}}
+   // expected-note@-1{{Returning from default constructor for 'A'}}
+B() // expected-note{{Calling default constructor for 'B'}}
+  {}
+};
+
+void test_note() {
+  C c; // expected-note{{Calling default constructor for 'C'}}
+}
+
+struct D: B {
+  D() : A(1), B() {}
+};
+
+void test_prunability() {
+  D d;
+  1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+}
+} // namespace note_on_skipped_vbases
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().isVirtualBasesBranch()) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,25 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch
+  // and we're taking the path that skips virtual base constructors.
+  if (L.getSrc()->getTerminator().isVirtualBasesBranch() &&
+  L.getDst() == *L.getSrc()->succ_begin()) {
+ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+[](BugReporterContext &, BugReport &) -> std::string {
+  // TODO: Just call out the name of the most derived class
+  // when we know it.
+  return "Virtual base initialization skipped because "
+ "it has already been handled by the most derived class";
+}, /*IsPrunable=*/true));
+// Perform the transition.
+ExplodedNodeSet Dst;
+NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+if (!Pred)
+  return;
+  }
+
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 assert(L.getLocationContext()->getCFG()->getExit().empty() &&
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ 

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

What about "the most derived class" or "a superclass" instead of "the 
superclass"? (https://en.cppreference.com/w/cpp/language/derived_class) 
May the sentence is a little bit too long. It would be cool to say "by `A`" or 
something more simple and precise.




Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:223
+// But only if we're actually skipping the virtual constructors.
+if (L.getDst() == *L.getSrc()->succ_begin()) {
+  ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(

I think it is better to reduce the number of `if`s and merge the related 
comment as it emphasizes there is //only one thing// happen.


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

https://reviews.llvm.org/D61817



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

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 199351.
NoQ added a comment.

Improve note text so that to avoid the singular/plural problem.


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

https://reviews.llvm.org/D61817

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/diagnostics/initializer.cpp

Index: clang/test/Analysis/diagnostics/initializer.cpp
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/initializer.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-output=text \
+// RUN:   -verify %s
+
+namespace note_on_skipped_vbases {
+struct A {
+  int x;
+  A() : x(0) {} // expected-note{{The value 0 is assigned to 'c.x'}}
+  A(int x) : x(x) {}
+};
+
+struct B : virtual A {
+  int y;
+  // This note appears only once, when this constructor is called from C.
+  // When this constructor is called from D, this note is still correct but
+  // it doesn't appear because it's pruned out because it's irrelevant to the
+  // bug report.
+  B(): // expected-note{{Virtual base initialization skipped because it has already been handled by the superclass}}
+A(1),
+y(1 / x) // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+  {}
+};
+
+struct C : B {
+  C(): // expected-note{{Calling default constructor for 'A'}}
+   // expected-note@-1{{Returning from default constructor for 'A'}}
+B() // expected-note{{Calling default constructor for 'B'}}
+  {}
+};
+
+void test_note() {
+  C c; // expected-note{{Calling default constructor for 'C'}}
+}
+
+struct D: B {
+  D() : A(1), B() {}
+};
+
+void test_prunability() {
+  D d;
+  1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+}
+} // namespace note_on_skipped_vbases
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().getKind() == CFGTerminator::VirtualBasesBranch) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,26 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch.
+  if (L.getSrc()->getTerminator().getKind() ==
+  CFGTerminator::VirtualBasesBranch) {
+// But only if we're actually skipping the virtual constructors.
+if (L.getDst() == *L.getSrc()->succ_begin()) {
+  ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+  [](BugReporterContext &, BugReport &) -> std::string {
+return "Virtual base initialization skipped because "
+   "it has already been handled by the superclass";
+  },
+  /*IsPrunable=*/true));
+  // Perform the transition.
+  ExplodedNodeSet Dst;
+  NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+  Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+  if (!Pred)
+return;
+}
+  }
+
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 assert(L.getLocationContext()->getCFG()->getExit().empty() &&
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2501,7 +2501,9 @@
   if (Optional Msg = T->generateMessage(BRC, R)) {
 PathDiagnosticLocation Loc =
 PathDiagnosticLocation::create(PP, BRC.getSourceManager());
-return std::make_shared(Loc, *Msg);
+auto Piece = std::make_shared(Loc, *Msg);
+Piece->setPrunable(T->isPrunable());
+

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This is how it looks:

F8864205: Screen Shot 2019-05-10 at 8.23.34 PM.png 



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

https://reviews.llvm.org/D61817



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

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, 
mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.
Herald added a project: clang.
NoQ updated this revision to Diff 199121.
NoQ added a comment.
NoQ added a parent revision: D61816: [CFG] [analyzer] pr41300: Add a branch to 
skip virtual base initializers when they are handled by the superclass..

Fix formatting in tests.


When initialization of base classes is skipped as in D61816 
, we might as well tell the user about it, 
because this aspect of C++ isn't very well-known.

I used the new note tags feature (D58367 ) in 
order to implement it. In order to make use of it, i had to allow note tags to 
produce prunable notes, and i also moved the note tag factory to `CoreEngine`.


https://reviews.llvm.org/D61817

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/diagnostics/initializer.cpp

Index: clang/test/Analysis/diagnostics/initializer.cpp
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/initializer.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-output=text \
+// RUN:   -verify %s
+
+namespace note_on_skipped_vbases {
+struct A {
+  int x;
+  A() : x(0) {} // expected-note{{The value 0 is assigned to 'c.x'}}
+  A(int x) : x(x) {}
+};
+
+struct B : virtual A {
+  int y;
+  // This note appears only once, when this constructor is called from C.
+  // When this constructor is called from D, this note is still correct but
+  // it doesn't appear because it's pruned out because it's irrelevant to the
+  // bug report.
+  B(): // expected-note{{Virtual base initializer is skipped because it has already been initialized by the superclass}}
+A(1),
+y(1 / x) // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+  {}
+};
+
+struct C : B {
+  C(): // expected-note{{Calling default constructor for 'A'}}
+   // expected-note@-1{{Returning from default constructor for 'A'}}
+B() // expected-note{{Calling default constructor for 'B'}}
+  {}
+};
+
+void test_note() {
+  C c; // expected-note{{Calling default constructor for 'C'}}
+}
+
+struct D: B {
+  D() : A(1), B() {}
+};
+
+void test_prunability() {
+  D d;
+  1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+}
+} // namespace note_on_skipped_vbases
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().getKind() == CFGTerminator::VirtualBasesBranch) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,26 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch.
+  if (L.getSrc()->getTerminator().getKind() ==
+  CFGTerminator::VirtualBasesBranch) {
+// But only if we're actually skipping the virtual constructors.
+if (L.getDst() == *L.getSrc()->succ_begin()) {
+  ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+  [](BugReporterContext &, BugReport &) -> std::string {
+return "Virtual base initializer is skipped because "
+   "it has already been initialized by the superclass";
+  },
+  /*IsPrunable=*/true));
+  // Perform the transition.
+  ExplodedNodeSet Dst;
+  NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+  Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+  if (!Pred)
+return;
+}
+ 

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 199121.
NoQ added a comment.

Fix formatting in tests.


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

https://reviews.llvm.org/D61817

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/diagnostics/initializer.cpp

Index: clang/test/Analysis/diagnostics/initializer.cpp
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/initializer.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-output=text \
+// RUN:   -verify %s
+
+namespace note_on_skipped_vbases {
+struct A {
+  int x;
+  A() : x(0) {} // expected-note{{The value 0 is assigned to 'c.x'}}
+  A(int x) : x(x) {}
+};
+
+struct B : virtual A {
+  int y;
+  // This note appears only once, when this constructor is called from C.
+  // When this constructor is called from D, this note is still correct but
+  // it doesn't appear because it's pruned out because it's irrelevant to the
+  // bug report.
+  B(): // expected-note{{Virtual base initializer is skipped because it has already been initialized by the superclass}}
+A(1),
+y(1 / x) // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+  {}
+};
+
+struct C : B {
+  C(): // expected-note{{Calling default constructor for 'A'}}
+   // expected-note@-1{{Returning from default constructor for 'A'}}
+B() // expected-note{{Calling default constructor for 'B'}}
+  {}
+};
+
+void test_note() {
+  C c; // expected-note{{Calling default constructor for 'C'}}
+}
+
+struct D: B {
+  D() : A(1), B() {}
+};
+
+void test_prunability() {
+  D d;
+  1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+}
+} // namespace note_on_skipped_vbases
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().getKind() == CFGTerminator::VirtualBasesBranch) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,26 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch.
+  if (L.getSrc()->getTerminator().getKind() ==
+  CFGTerminator::VirtualBasesBranch) {
+// But only if we're actually skipping the virtual constructors.
+if (L.getDst() == *L.getSrc()->succ_begin()) {
+  ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+  [](BugReporterContext &, BugReport &) -> std::string {
+return "Virtual base initializer is skipped because "
+   "it has already been initialized by the superclass";
+  },
+  /*IsPrunable=*/true));
+  // Perform the transition.
+  ExplodedNodeSet Dst;
+  NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+  Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+  if (!Pred)
+return;
+}
+  }
+
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 assert(L.getLocationContext()->getCFG()->getExit().empty() &&
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2501,7 +2501,9 @@
   if (Optional Msg = T->generateMessage(BRC, R)) {
 PathDiagnosticLocation Loc =
 PathDiagnosticLocation::create(PP, BRC.getSourceManager());
-return std::make_shared(Loc, *Msg);
+auto Piece = std::make_shared(Loc, *Msg);
+Piece->setPrunable(T->isPrunable());
+return Piece;
   }
 
   return