[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus added a comment.

We have no solid evidence to conclude that such an event should not occur at a 
`BlockEntrance`, so fixing this error mustn't be that bad. I certainly 
should've used `llvm::isa_or_nonnull`, so the patch overall makes sense, so I'm 
commiting it as is. With that said, this test case highlights a fundamental 
flaw in how loop wideining is implemented (hence it being off-by-default), and 
it should be addressed later separately.

I have talked to @NoQ about this behind the scenes, which is why I'm not 
concerned with the green check mark :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL372269: [analyzer] PR43102: Fix an assertion and an 
out-of-bounds error for diagnostic… (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66716?vs=217041=220760#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66716

Files:
  cfe/trunk/lib/Analysis/PathDiagnostic.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  cfe/trunk/test/Analysis/loop-widening.cpp


Index: cfe/trunk/test/Analysis/loop-widening.cpp
===
--- cfe/trunk/test/Analysis/loop-widening.cpp
+++ cfe/trunk/test/Analysis/loop-widening.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config widen-loops=true \
+// RUN:   -analyzer-config track-conditions=false \
+// RUN:   -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+  void m_fn1();
+};
+bool g;
+void fn1() {
+  A a;
+  A *b = 
+
+  for (;;) { // expected-note{{Loop condition is true.  Entering loop body}}
+ // expected-note@-1{{Loop condition is true.  Entering loop body}}
+ // expected-note@-2{{Value assigned to 'b'}}
+ // no crash during bug report construction
+
+g = !b; // expected-note{{Assuming 'b' is null}}
+b->m_fn1(); // expected-warning{{Called C++ object pointer is null}}
+// expected-note@-1{{Called C++ object pointer is null}}
+  }
+}
+} // end of namespace pr43102
Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1438,6 +1438,7 @@
 
   if (!StoreSite)
 return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1802,7 +1803,7 @@
   if (ControlDeps.isControlDependent(OriginB, NB)) {
 // We don't really want to explain for range loops. Evidence suggests that
 // the only thing that leads to is the addition of calls to operator!=.
-if (isa(NB->getTerminator()))
+if (llvm::isa_and_nonnull(NB->getTerminatorStmt()))
   return nullptr;
 
 if (const Expr *Condition = NB->getLastCondition()) {
Index: cfe/trunk/lib/Analysis/PathDiagnostic.cpp
===
--- cfe/trunk/lib/Analysis/PathDiagnostic.cpp
+++ cfe/trunk/lib/Analysis/PathDiagnostic.cpp
@@ -695,14 +695,18 @@
 return PathDiagnosticLocation(
 CEB->getLocationContext()->getDecl()->getSourceRange().getEnd(), SMng);
   } else if (Optional BE = P.getAs()) {
-CFGElement BlockFront = BE->getBlock()->front();
-if (auto StmtElt = BlockFront.getAs()) {
-  return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
-} else if (auto NewAllocElt = BlockFront.getAs()) {
-  return PathDiagnosticLocation(
-  NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+if (Optional BlockFront = BE->getFirstElement()) {
+  if (auto StmtElt = BlockFront->getAs()) {
+return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
+  } else if (auto NewAllocElt = BlockFront->getAs()) {
+return PathDiagnosticLocation(
+NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+  }
+  llvm_unreachable("Unexpected CFG element at front of block");
 }
-llvm_unreachable("Unexpected CFG element at front of block");
+
+return PathDiagnosticLocation(
+BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
   } else if (Optional FE = P.getAs()) {
 return PathDiagnosticLocation(FE->getStmt(), SMng,
   FE->getLocationContext());


Index: cfe/trunk/test/Analysis/loop-widening.cpp
===
--- cfe/trunk/test/Analysis/loop-widening.cpp
+++ cfe/trunk/test/Analysis/loop-widening.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config widen-loops=true \
+// RUN:   -analyzer-config track-conditions=false \
+// RUN:   -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+  void m_fn1();
+};
+bool g;
+void fn1() {
+  A a;
+  A *b = 
+
+  for (;;) { // expected-note{{Loop condition is true.  Entering loop body}}
+ // expected-note@-1{{Loop condition is true.  Entering loop body}}
+ // expected-note@-2{{Value assigned to 'b'}}
+ // no crash during bug report construction

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus added a comment.

While we're there, could you please see whether the included test case (note 
how condition tracking is turned off) fails on your platform without the rest 
of the patch applied (it definitely does on mine, which is why I was surprised 
to see this bug report pop up only now)? If not, I can just push a small commit 
with the `llvm::isa_and_nonnull` change to get some breathing room.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus added a comment.

In D66716#1671105 , @TWeaver wrote:

> Hi there,
>
> just checking in, is this patch still going ahead?
>
> thanks
>  Tom W


Unfortunately, it seems like the correct solution is a bit more complex than 
these if branches, so it might take just a bit longer, but I'm definitely on 
it! I'm also talking to folks behind to scenes to catch up with how loop 
widening works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

[Tom Weaver via Phabricator via cfe-commits]

TWeaver added a comment.

Hi there,

just checking in, is this patch still going ahead?

thanks
Tom W


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus added a comment.

I'm still working on this, just been kinda busy. I'll try to get it out of the 
way asap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Do you think its ever okay to find a last store in a `BlockEdge`?

*In a `BlockEntrance` into an empty block (that has no elements but does have a 
terminator). At least that's what your code is fixing.

Attaching store notes to such program points is most likely not ok. We can 
always make ourselves a `WideningPoint` for that specific purpose, or maybe 
even `PreWidening` / `PostWidening`. Program points are not set in stone, we 
are free to make as many kinds of them as we need (which is why we also have 
tags).

Being able to attach an event note at all to such program point is most likely 
ok. Any program point can potentially represent an interesting event. So this 
is anyway a good change. I'd love to have some more careful testing, maybe a 
unittest (or somehow trick -verify with line breaks), so that to know where 
exactly does the note go in this case (is it the jump from the bottom of the 
loop? is it jump from increment to condition?). This way we'll make sure that 
the implementation is correct.

I'd still want you to figure out why is this widening-specific. Do i understand 
correctly that we're doing widening in an inappropriate moment of time? I'd 
prefer to have this confirmed. Or can we reproduce this crash / incorrect 
behavior / false positive without widening?




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1805
 // the only thing that leads to is the addition of calls to operator!=.
-if (isa(NB->getTerminator()))
   return nullptr;

Why did this even compile? I thought i deleted these conversions in D61814 >.<



Comment at: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp:774-775
+
+return PathDiagnosticLocation(
+BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
   } else if (Optional FE = P.getAs()) {

What exactly is the terminator here in your case? Is it `NullStmt`? Is there 
always a terminator and/or a terminator statement?

What if that's a `BlockEntrance` to the exit-block? (do we even make such 
program points? i hope we don't)

I think this code needs comments in order to explain what picture do we have in 
mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus added a comment.

In D66716#1647668 , @NoQ wrote:

> I don't understand. Isn't widening supposed to happen //after we exit the 
> loop//? The loop isn't over yet when the bug is being reported. Why is this 
> problem widening-specific? Given that we also have a weird invalidation of 
> `b`, i suspect that we're doing widening in a wrong moment of time.


Something is totally off here. Do you think its ever okay to find a last store 
in a `BlockEdge`? Should I rather fix this by changing how loop widening works?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I don't understand. Isn't widening supposed to happen //after we exit the 
loop//? The loop isn't over yet when the bug is being reported. Why is this 
problem widening-specific? Given that we also have a weird invalidation of `b`, 
i suspect that we're doing widening in a wrong moment of time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

[Tom Weaver via Phabricator via cfe-commits]

TWeaver added a comment.

Hi there!

thanks so much for this patch, can confirm that with this patch applied my 
local machine no longer crashes for the loop-widening.cpp test.

LGTM for me, but this isn't really my area of expertise so I can't judge one 
whether these changes are the correct ones to make.

Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus added a comment.

Note how I am partially at fault here, which is why the bug reports state that 
the crash originates from `TrackControlDependencyVisitor`, but even after that 
is fixed, the diagnostics construction flopped on an out-of-bounds error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66716



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

[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

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

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, Charusso, 
dcoughlin, rnkovacs, TWeaver.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.

https://bugs.llvm.org/show_bug.cgi?id=43102

In today's edition of "Is this any better now that it isn't crashing?", I'd 
like to show you a very interesting test case with loop widening.

Looking at the included test case, it's immediately obvious that this is not 
only a false positive, but also a very bad bug report in general. We can see 
how the analyzer mistakenly invalidated `b`, instead of its pointee, resulting 
in it reporting a null pointer dereference error. Not only that, the point at 
which this change of value is noted at is at the loop, rather then at the 
method call.

It turns out that `FindLastStoreVisitor` works correctly, rather the supplied 
explodedgraph is faulty, because `BlockEdge` really is the `ProgramPoint` where 
this happens.
F9855739: image.png 
So it's fair to say that this needs improving on multiple fronts. In any case, 
at least the crash is gone.

Full ExplodedGraph: F9855743: loop.dot 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66716

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/loop-widening.cpp


Index: clang/test/Analysis/loop-widening.cpp
===
--- /dev/null
+++ clang/test/Analysis/loop-widening.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config widen-loops=true \
+// RUN:   -analyzer-config track-conditions=false \
+// RUN:   -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+  void m_fn1();
+};
+bool g;
+void fn1() {
+  A a;
+  A *b = 
+
+  for (;;) { // expected-note{{Loop condition is true.  Entering loop body}}
+ // expected-note@-1{{Loop condition is true.  Entering loop body}}
+ // expected-note@-2{{Value assigned to 'b'}}
+ // no crash during bug report construction
+
+g = !b; // expected-note{{Assuming 'b' is null}}
+b->m_fn1(); // expected-warning{{Called C++ object pointer is null}}
+// expected-note@-1{{Called C++ object pointer is null}}
+  }
+}
+} // end of namespace pr43102
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -761,14 +761,18 @@
 return PathDiagnosticLocation(
 CEB->getLocationContext()->getDecl()->getSourceRange().getEnd(), SMng);
   } else if (Optional BE = P.getAs()) {
-CFGElement BlockFront = BE->getBlock()->front();
-if (auto StmtElt = BlockFront.getAs()) {
-  return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
-} else if (auto NewAllocElt = BlockFront.getAs()) {
-  return PathDiagnosticLocation(
-  NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+if (Optional BlockFront = BE->getFirstElement()) {
+  if (auto StmtElt = BlockFront->getAs()) {
+return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
+  } else if (auto NewAllocElt = BlockFront->getAs()) {
+return PathDiagnosticLocation(
+NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+  }
+  llvm_unreachable("Unexpected CFG element at front of block");
 }
-llvm_unreachable("Unexpected CFG element at front of block");
+
+return PathDiagnosticLocation(
+BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
   } else if (Optional FE = P.getAs()) {
 return PathDiagnosticLocation(FE->getStmt(), SMng,
   FE->getLocationContext());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1441,6 +1441,7 @@
 
   if (!StoreSite)
 return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1802,7 +1803,7 @@
   if (ControlDeps.isControlDependent(OriginB, NB)) {
 // We don't really want to explain for range loops. Evidence suggests that
 // the only thing that leads to is the addition of calls to operator!=.
-if (isa(NB->getTerminator()))
+if (llvm::isa_and_nonnull(NB->getTerminatorStmt()))
   return nullptr;
 
 if (const Expr *Condition = NB->getLastCondition()) {


Index: clang/test/Analysis/loop-widening.cpp