[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/16031
MSan doesn't catch it >.<


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6bbca3411b38: [CFG] Add a test for a flaky crash in 
CFGBlock::getLastCondition(). (authored by dergachev.a).

Changed prior to commit:
  https://reviews.llvm.org/D69962?vs=228292&id=230575#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69962

Files:
  clang/test/Analysis/a_flaky_crash.cpp


Index: clang/test/Analysis/a_flaky_crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/a_flaky_crash.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+struct S {
+  S();
+  ~S();
+};
+
+bool bar(S);
+
+// no-crash during diagnostic construction.
+void foo() {
+  int x;
+  if (true && bar(S()))
+++x; // expected-warning{{The expression is an uninitialized value. The 
computed value will also be garbage}}
+}


Index: clang/test/Analysis/a_flaky_crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/a_flaky_crash.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+struct S {
+  S();
+  ~S();
+};
+
+bool bar(S);
+
+// no-crash during diagnostic construction.
+void foo() {
+  int x;
+  if (true && bar(S()))
+++x; // expected-warning{{The expression is an uninitialized value. The computed value will also be garbage}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

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

Szelethus added a comment.

In D69962#1744679 , @NoQ wrote:

> In D69962#1742291 , @Szelethus wrote:
>
> > Try to add a non-sanitizer built tablegen: 
> > `-DCLANG_TABLEGEN=/path/to/non/sanitized/clang-tblgen` 
> > `-DLLVM_TABLEGEN=/path/to/non/sanitized/llvm-tblgen`
>
>
> Yay nice! The clang that i built this way is still unusable though, i.e. 
> crashes on almost all tests :( I guess i'm doing something wrong.
>
> I'd like to commit the test as is and then keep looking for the solution in 
> the background.


Could you just give one last shot to commiting the known-to-be-incorrect test 
to see how the buildbots react first?


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D69962#1742291 , @Szelethus wrote:

> Try to add a non-sanitizer built tablegen: 
> `-DCLANG_TABLEGEN=/path/to/non/sanitized/clang-tblgen` 
> `-DLLVM_TABLEGEN=/path/to/non/sanitized/llvm-tblgen`


Yay nice! The clang that i built this way is still unusable though, i.e. 
crashes on almost all tests :( I guess i'm doing something wrong.

I'd like to commit the test as is and then keep looking for the solution in the 
background.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

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

Szelethus added a comment.

Accepted accidentally. Well, in any case, I trust your judgement on this! I'd 
prefer not to commit the test file as-is.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

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

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

In D69962#1741503 , @NoQ wrote:

> In D69962#1739440 , @NoQ wrote:
>
> > In D69962#1738618 , @Szelethus 
> > wrote:
> >
> > > Nice catch! Though, wouldn't the memory sanitizer buildbots break on this 
> > > reliably?
> >
> >
> > I kinda hope so; i'll take a look at home.
>
>
> Memory sanitizer at home: //*fails with 1700 uninitialized reads in 
> TableGen*//.


Try to add a non-sanitizer built tablegen: 
`-DCLANG_TABLEGEN=/path/to/non/sanitized/clang-tblgen` 
`-DLLVM_TABLEGEN=/path/to/non/sanitized/llvm-tblgen`

> I guess i could commit test first, then get buildbots to fail, then if they 
> do commit the code, otherwise commit the code and multiply the run-lines.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D69962#1739440 , @NoQ wrote:

> In D69962#1738618 , @Szelethus wrote:
>
> > Nice catch! Though, wouldn't the memory sanitizer buildbots break on this 
> > reliably?
>
>
> I kinda hope so; i'll take a look at home.


Memory sanitizer at home: //*fails with 1700 uninitialized reads in TableGen*//.

I guess i could commit test first, then get buildbots to fail, then if they do 
commit the code, otherwise commit the code and multiply the run-lines.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

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

In D69962#1738618 , @Szelethus wrote:

> Nice catch! Though, wouldn't the memory sanitizer buildbots break on this 
> reliably?


I kinda hope so; i'll take a look at home.




Comment at: clang/lib/Analysis/CFG.cpp:5882
 
+  // FIXME: Should we return the terminator here?
+  if (size() == 0)

Szelethus wrote:
> What would that even be?
Dunno. Excellent question. I'm, like, curious if there's a better behavior we 
can implement.

Here's the CFG for the current test case:

{F10670002}

FIXME: It's not particularly obvious from the dump that `[B5.6]` is a 
`MaterializeTemporaryExpr`.

I can't say i'm a big fan of how it looks. The temp dtor branch (terminator of 
`[B4]`) is unnecessary here, given that the short circuit never happens. But 
that's our typical problems with how CFG cuts off unreachable branches but 
never canonicalizes itself after that.

One way or another, we were crashing when calling `getLastCondition()` for 
`[B2]`. Its terminator is the if-statement. It probably doesn't make much sense 
to return the if-statement, but the answer that you're looking for is most 
likely `[B4.1]` rather than `nullptr`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

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

Szelethus added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:5882
 
+  // FIXME: Should we return the terminator here?
+  if (size() == 0)

What would that even be?


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

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

Szelethus added a comment.

Nice catch! Though, wouldn't the memory sanitizer buildbots break on this 
reliably?


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/a_flaky_crash.cpp:19
+
+// 256 copies of the same run-line to make it crash more often when it breaks.
+

This is a trade-off between reliability and not increasing test time too much. 
This test starts with an "a", so it fires up immediately, and on my machine 
it's not the last test to finish during `check-clang-analysis`, so i guess it 
shouldn't cause too much trouble.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69962



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

[PATCH] D69962: [CFG] Fix a flaky crash in CFGBlock::getLastCondition().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, 
baloghadamsoftware, Charusso.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Using an end iterator of an empty CFG block boiled down to dereferencing a 
garbage pointer.

This was fun to debug because the actual segfault occurs once in ~20 runs on 
the original code (on my system; on top of that, each run took several 
minutes). On the newly added test it crashes even more rarely, roughly once in 
500 runs.

CFG uses `llvm::BumpVector` for storing the list of elements. Its iterators are 
typedefs for raw pointers, so there's no way to check the correctness of the 
iterator by injecting assertions into it.

> [12:26:29] <@NoQ> I'm about to commit a fix for a flaky crash that's 
> reproducible once in ~1000 compilations. Can we make for-loops in lit?
>  [12:27:12] <@jdoerfert> @NoQ: jdenny: has an extension to do that (I think)
>  [12:36:40] <@NoQ> @jdoerfert: Thanks!
>  [12:36:59] <@jdoerfert> @NoQ: so, I doubt we have on in-tree
>  [12:37:21] <@NoQ> Mm, ok. I guess i could copy-paste the run-line :)
>  [12:37:36] <@Lebedev.RI> i remember seeing previous fixes with such idea, 
> but i don't recall how they achieved that
>  [12:37:37] <@jdoerfert> that is one way, yes ;)
>  [12:38:16] <@jdoerfert> #include <>; #include<>; #include<>; ... exponential 
> growth!


Repository:
  rC Clang

https://reviews.llvm.org/D69962

Files:
  clang/lib/Analysis/CFG.cpp
  clang/test/Analysis/a_flaky_crash.cpp

Index: clang/test/Analysis/a_flaky_crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/a_flaky_crash.cpp
@@ -0,0 +1,277 @@
+// This code used to crash but unpredictably and rarely.
+// Even with the current set of run-lines, if a buildbot tells you that
+// you broke this test there's a chance that someone else broke it
+// a few commits ago.
+
+struct S {
+  S();
+  ~S();
+};
+
+bool bar(S);
+
+void foo() {
+  int x;
+  if (true && bar(S()))
+++x; // expected-warning{{The expression is an uninitialized value. The computed value will also be garbage}}
+}
+
+// 256 copies of the same run-line to make it crash more often when it breaks.
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang