-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang][analysis] Fix flaky clang/test/Analysis/live-stmts.cpp test #126913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Multiple people reported flaky bot failures tied to clang/test/Analysis/live-stmts.cpp I tried reproducing the flaky behavior on my Linux x86_64 system, but the tests appears to be stable in my context. Only by looking at the failures reported, I could formulate a potential diagnosis. The output always looked almost the same, except that the Exprs dumped per Basic block were shuffled compared to my expectation. This suggests to me some ordering issue. If you look at the backing storage of `blocksEndToLiveness[B].liveExprs`, it uses `llvm::ImmutableSet<const Expr *>`. That container likely uses the pointer values as keys, thus the runtime values of the addresses influence the iteration order. To fix this, before dumping, I sort the expressions by their "beginLocs". It should be efficient enough for a debug checker, where there is no performance constraint. This should hopefully fix the flaky behavior on systems where ASLR works differently than (my) Linux system. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Balazs Benics (steakhal) ChangesMultiple people reported flaky bot failures tied to Only by looking at the failures reported, I could formulate a potential diagnosis. If you look at the backing storage of To fix this, before dumping, I sort the expressions by their "beginLocs". It should be efficient enough for a debug checker, where there is no performance constraint. This should hopefully fix the flaky behavior on systems where ASLR works differently than (my) Linux system. Hopefully fixes #126619 Full diff: https://github.com/llvm/llvm-project/pull/126913.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/LiveVariables.cpp b/clang/lib/Analysis/LiveVariables.cpp
index 481932ee59c8e..af563702b77bf 100644
--- a/clang/lib/Analysis/LiveVariables.cpp
+++ b/clang/lib/Analysis/LiveVariables.cpp
@@ -16,7 +16,9 @@
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/DataflowWorklist.h"
+#include "clang/Basic/SourceManager.h"
#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/raw_ostream.h"
#include <algorithm>
#include <optional>
@@ -662,12 +664,19 @@ void LiveVariables::dumpExprLiveness(const SourceManager &M) {
}
void LiveVariablesImpl::dumpExprLiveness(const SourceManager &M) {
+ auto ByBeginLoc = [&M](const Expr *L, const Expr *R) {
+ return M.isBeforeInTranslationUnit(L->getBeginLoc(), R->getBeginLoc());
+ };
+
// Don't iterate over blockEndsToLiveness directly because it's not sorted.
for (const CFGBlock *B : *analysisContext.getCFG()) {
llvm::errs() << "\n[ B" << B->getBlockID()
<< " (live expressions at block exit) ]\n";
- for (const Expr *E : blocksEndToLiveness[B].liveExprs) {
+ std::vector<const Expr *> LiveExprs;
+ llvm::append_range(LiveExprs, blocksEndToLiveness[B].liveExprs);
+ llvm::sort(LiveExprs, ByBeginLoc);
+ for (const Expr *E : LiveExprs) {
llvm::errs() << "\n";
E->dump();
}
diff --git a/clang/test/Analysis/live-stmts.cpp b/clang/test/Analysis/live-stmts.cpp
index 33b8d59305d3d..8034d3a30436e 100644
--- a/clang/test/Analysis/live-stmts.cpp
+++ b/clang/test/Analysis/live-stmts.cpp
@@ -1,6 +1,3 @@
-// Flaky on aarch64: http://llvm.org/PR126619
-// UNSUPPORTED: target=aarch64{{.*}}
-
// RUN: %clang_analyze_cc1 -w -analyzer-checker=debug.DumpLiveExprs %s 2>&1\
// RUN: | FileCheck %s
@@ -29,34 +26,36 @@ int testThatDumperWorks(int x, int y, int z) {
// CHECK-EMPTY:
// CHECK: [ B2 (live expressions at block exit) ]
// CHECK-EMPTY:
-// CHECK-NEXT: DeclRefExpr {{.*}} 'y' 'int'
-// CHECK-EMPTY:
-// CHECK-NEXT: DeclRefExpr {{.*}} 'z' 'int'
-// CHECK-EMPTY:
// CHECK-NEXT: ImplicitCastExpr {{.*}} <IntegralToBoolean>
// CHECK-NEXT: `-ImplicitCastExpr {{.*}} <LValueToRValue>
// CHECK-NEXT: `-DeclRefExpr {{.*}} 'x' 'int'
// CHECK-EMPTY:
-// CHECK-EMPTY:
-// CHECK: [ B3 (live expressions at block exit) ]
-// CHECK-EMPTY:
// CHECK-NEXT: DeclRefExpr {{.*}} 'y' 'int'
// CHECK-EMPTY:
// CHECK-NEXT: DeclRefExpr {{.*}} 'z' 'int'
// CHECK-EMPTY:
+// CHECK-EMPTY:
+// CHECK: [ B3 (live expressions at block exit) ]
+// CHECK-EMPTY:
// CHECK-NEXT: ImplicitCastExpr {{.*}} <IntegralToBoolean>
// CHECK-NEXT: `-ImplicitCastExpr {{.*}} <LValueToRValue>
// CHECK-NEXT: `-DeclRefExpr {{.*}} 'x' 'int'
-// CHECK: [ B4 (live expressions at block exit) ]
// CHECK-EMPTY:
// CHECK-NEXT: DeclRefExpr {{.*}} 'y' 'int'
// CHECK-EMPTY:
// CHECK-NEXT: DeclRefExpr {{.*}} 'z' 'int'
// CHECK-EMPTY:
+// CHECK-EMPTY:
+// CHECK: [ B4 (live expressions at block exit) ]
+// CHECK-EMPTY:
// CHECK-NEXT: ImplicitCastExpr {{.*}} <IntegralToBoolean>
// CHECK-NEXT: `-ImplicitCastExpr {{.*}} <LValueToRValue>
// CHECK-NEXT: `-DeclRefExpr {{.*}} 'x' 'int'
// CHECK-EMPTY:
+// CHECK-NEXT: DeclRefExpr {{.*}} 'y' 'int'
+// CHECK-EMPTY:
+// CHECK-NEXT: DeclRefExpr {{.*}} 'z' 'int'
+// CHECK-EMPTY:
// CHECK-EMPTY:
// CHECK: [ B5 (live expressions at block exit) ]
// CHECK-EMPTY:
@@ -226,15 +225,15 @@ int logicalOpInTernary(bool b) {
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
-// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
-// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
-// CHECK-EMPTY:
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
+// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
+// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
+// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 0
// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 1
@@ -245,15 +244,15 @@ int logicalOpInTernary(bool b) {
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
-// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
-// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
-// CHECK-EMPTY:
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
+// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
+// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
+// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 0
// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 1
@@ -264,15 +263,15 @@ int logicalOpInTernary(bool b) {
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
-// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
-// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
-// CHECK-EMPTY:
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
+// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
+// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
+// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 0
// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 1
@@ -283,15 +282,15 @@ int logicalOpInTernary(bool b) {
// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
-// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
-// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
-// CHECK-EMPTY:
// CHECK: BinaryOperator {{.*}} '_Bool' '||'
// CHECK: |-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: | `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK: `-ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
// CHECK-EMPTY:
+// CHECK: ImplicitCastExpr {{.*}} '_Bool' <LValueToRValue>
+// CHECK: `-DeclRefExpr {{.*}} '_Bool' lvalue ParmVar {{.*}} 'b' '_Bool'
+// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 0
// CHECK-EMPTY:
// CHECK: IntegerLiteral {{.*}} 'int' 1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for fixing the test
Unfortunately the test still seems to be flaky. With EXPENSIVE_CHECKS I often see it fail on the RHEL8 x86_64 machines I use.
|
Oh cmmon. :( |
Yep |
What the heck, your logs refer to I'll just disable this test on all platforms, and try to debug this with the parties who reported flaky behavior. |
@steakhal : Sure I can do some tests if you come up with a fix. Right now, with an EXPENSIVE_CHECKS build it fails in 30-40% of the runs for me so it should be pretty quick to see if a fix works or not. |
I could reproduce the flakyness on my system using expensive checks. Thanks for the hint. |
I had previous attempts for fixing this flaky test. Let's admit I failed so far, and disable this until we have a permanent fix. See the discussion at: llvm#126913 (comment)
I had previous attempts for fixing this flaky test. Let's admit I failed so far, and disable this until we have a permanent fix. See the discussion at: #126913 (comment)
…ky (#127034) I had previous attempts for fixing this flaky test. Let's admit I failed so far, and disable this until we have a permanent fix. See the discussion at: llvm/llvm-project#126913 (comment)
…lvm#126913) Multiple people reported flaky bot failures tied to `clang/test/Analysis/live-stmts.cpp` I tried reproducing the flaky behavior on my Linux x86_64 system, but the tests appears to be stable in my context. Only by looking at the failures reported, I could formulate a potential diagnosis. The output always looked almost the same, except that the Exprs dumped per Basic block were shuffled compared to my expectation. This suggests to me some ordering issue. If you look at the backing storage of `blocksEndToLiveness[B].liveExprs`, it uses `llvm::ImmutableSet<const Expr *>`. That container likely uses the pointer values as keys, thus the runtime values of the addresses influence the iteration order. To fix this, before dumping, I sort the expressions by their "beginLocs". It should be efficient enough for a debug checker, where there is no performance constraint. This should hopefully fix the flaky behavior on systems where ASLR works differently than (my) Linux system. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804
…7034) I had previous attempts for fixing this flaky test. Let's admit I failed so far, and disable this until we have a permanent fix. See the discussion at: llvm#126913 (comment)
For what it worth this change made it persistently failing in my setup. |
I'm not sure I follow you. |
The commit you linked belongs to a backend code change in LLVM, not this patch. Also, "unsupported platform" should show up as unsupported in LIT, not as failures. Are you using some custom |
oh sorry, I meant be25d61 :) |
Let me know if this test still causes any issues on main. Otherwise, I'll consider this thread concluded. |
…lvm#126913) Multiple people reported flaky bot failures tied to `clang/test/Analysis/live-stmts.cpp` I tried reproducing the flaky behavior on my Linux x86_64 system, but the tests appears to be stable in my context. Only by looking at the failures reported, I could formulate a potential diagnosis. The output always looked almost the same, except that the Exprs dumped per Basic block were shuffled compared to my expectation. This suggests to me some ordering issue. If you look at the backing storage of `blocksEndToLiveness[B].liveExprs`, it uses `llvm::ImmutableSet<const Expr *>`. That container likely uses the pointer values as keys, thus the runtime values of the addresses influence the iteration order. To fix this, before dumping, I sort the expressions by their "beginLocs". It should be efficient enough for a debug checker, where there is no performance constraint. This should hopefully fix the flaky behavior on systems where ASLR works differently than (my) Linux system. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804
…7034) I had previous attempts for fixing this flaky test. Let's admit I failed so far, and disable this until we have a permanent fix. See the discussion at: llvm#126913 (comment)
…2nd attempt) In my previous attempt (llvm#126913) of fixing the flaky case was on a good track when I used the begin locations as a stable ordering. However, I forgot to consider the case when the begin locations are the same among the Exprs. In an `EXPENSIVE_CHECKS` build, arrays are randomly shuffled prior to sorting them. This exposed the flaky behavior much more often basically breaking the "stability" of the vector - as it should. To fix this, I I use this time `Expr::getID` for a stable ID for an Expr. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804
…2nd attempt) (#127406) In my previous attempt (#126913) of fixing the flaky case was on a good track when I used the begin locations as a stable ordering. However, I forgot to consider the case when the begin locations are the same among the Exprs. In an `EXPENSIVE_CHECKS` build, arrays are randomly shuffled prior to sorting them. This exposed the flaky behavior much more often basically breaking the "stability" of the vector - as it should. Because of this, I had to revert the previous fix attempt in #127034. To fix this, I use this time `Expr::getID` for a stable ID for an Expr. Hopefully fixes #126619 Hopefully fixes #126804
FYI I just merged my second attempt of enabling and fixing the flaky test. Let me know if it works. @metaflow @mikaelholmen |
No failures so far :) |
…lvm#126913) Multiple people reported flaky bot failures tied to `clang/test/Analysis/live-stmts.cpp` I tried reproducing the flaky behavior on my Linux x86_64 system, but the tests appears to be stable in my context. Only by looking at the failures reported, I could formulate a potential diagnosis. The output always looked almost the same, except that the Exprs dumped per Basic block were shuffled compared to my expectation. This suggests to me some ordering issue. If you look at the backing storage of `blocksEndToLiveness[B].liveExprs`, it uses `llvm::ImmutableSet<const Expr *>`. That container likely uses the pointer values as keys, thus the runtime values of the addresses influence the iteration order. To fix this, before dumping, I sort the expressions by their "beginLocs". It should be efficient enough for a debug checker, where there is no performance constraint. This should hopefully fix the flaky behavior on systems where ASLR works differently than (my) Linux system. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804
…7034) I had previous attempts for fixing this flaky test. Let's admit I failed so far, and disable this until we have a permanent fix. See the discussion at: llvm#126913 (comment)
…2nd attempt) (llvm#127406) In my previous attempt (llvm#126913) of fixing the flaky case was on a good track when I used the begin locations as a stable ordering. However, I forgot to consider the case when the begin locations are the same among the Exprs. In an `EXPENSIVE_CHECKS` build, arrays are randomly shuffled prior to sorting them. This exposed the flaky behavior much more often basically breaking the "stability" of the vector - as it should. Because of this, I had to revert the previous fix attempt in llvm#127034. To fix this, I use this time `Expr::getID` for a stable ID for an Expr. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804
…2nd attempt) (llvm#127406) In my previous attempt (llvm#126913) of fixing the flaky case was on a good track when I used the begin locations as a stable ordering. However, I forgot to consider the case when the begin locations are the same among the Exprs. In an `EXPENSIVE_CHECKS` build, arrays are randomly shuffled prior to sorting them. This exposed the flaky behavior much more often basically breaking the "stability" of the vector - as it should. Because of this, I had to revert the previous fix attempt in llvm#127034. To fix this, I use this time `Expr::getID` for a stable ID for an Expr. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804 (cherry picked from commit f378e52)
…2nd attempt) (llvm#127406) In my previous attempt (llvm#126913) of fixing the flaky case was on a good track when I used the begin locations as a stable ordering. However, I forgot to consider the case when the begin locations are the same among the Exprs. In an `EXPENSIVE_CHECKS` build, arrays are randomly shuffled prior to sorting them. This exposed the flaky behavior much more often basically breaking the "stability" of the vector - as it should. Because of this, I had to revert the previous fix attempt in llvm#127034. To fix this, I use this time `Expr::getID` for a stable ID for an Expr. Hopefully fixes llvm#126619 Hopefully fixes llvm#126804 (cherry picked from commit f378e52)
Multiple people reported flaky bot failures tied to
clang/test/Analysis/live-stmts.cpp
I tried reproducing the flaky behavior on my Linux x86_64 system, but the tests appears to be stable in my context.Only by looking at the failures reported, I could formulate a potential diagnosis.
The output always looked almost the same, except that the Exprs dumped per Basic block were shuffled compared to my expectation. This suggests to me some ordering issue.
If you look at the backing storage of
blocksEndToLiveness[B].liveExprs
,it uses
llvm::ImmutableSet<const Expr *>
.That container likely uses the pointer values as keys, thus the runtime values of the addresses influence the iteration order.
To fix this, before dumping, I sort the expressions by their "beginLocs". It should be efficient enough for a debug checker, where there is no performance constraint.
This should hopefully fix the flaky behavior on systems where ASLR works differently than (my) Linux system.
Hopefully fixes #126619
Hopefully fixes #126804