Skip to content

[clang][analysis] Fix flaky clang/test/Analysis/live-stmts.cpp test (2nd attempt) #127406

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

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

steakhal
Copy link
Contributor

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

…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
@steakhal steakhal added clang Clang issues not falling into any other category clang:static analyzer clang:analysis labels Feb 16, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 16, 2025

@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Balazs Benics (steakhal)

Changes

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


Full diff: https://github.com/llvm/llvm-project/pull/127406.diff

2 Files Affected:

  • (modified) clang/lib/Analysis/LiveVariables.cpp (+4-4)
  • (modified) clang/test/Analysis/live-stmts.cpp (+22-25)
diff --git a/clang/lib/Analysis/LiveVariables.cpp b/clang/lib/Analysis/LiveVariables.cpp
index af563702b77bf..c7d3451d37cf6 100644
--- a/clang/lib/Analysis/LiveVariables.cpp
+++ b/clang/lib/Analysis/LiveVariables.cpp
@@ -664,18 +664,18 @@ 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());
+  const ASTContext &Ctx = analysisContext.getASTContext();
+  auto ByIDs = [&Ctx](const Expr *L, const Expr *R) {
+    return L->getID(Ctx) < R->getID(Ctx);
   };
 
   // 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";
     std::vector<const Expr *> LiveExprs;
     llvm::append_range(LiveExprs, blocksEndToLiveness[B].liveExprs);
-    llvm::sort(LiveExprs, ByBeginLoc);
+    llvm::sort(LiveExprs, ByIDs);
     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 9cac815e65de1..ca2ff6da8b133 100644
--- a/clang/test/Analysis/live-stmts.cpp
+++ b/clang/test/Analysis/live-stmts.cpp
@@ -1,6 +1,3 @@
-// Disabling this flaky test, see https://github.com/llvm/llvm-project/pull/126913#issuecomment-2655850766
-// UNSUPPORTED: true
-
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=debug.DumpLiveExprs %s 2>&1\
 // RUN:   | FileCheck %s
 
@@ -29,36 +26,36 @@ int testThatDumperWorks(int x, int y, int z) {
 // CHECK-EMPTY:
 // CHECK: [ B2 (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: [ B3 (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: [ B3 (live expressions at block exit) ]
 // CHECK-EMPTY:
+// CHECK-NEXT: DeclRefExpr {{.*}} 'y' 'int'
 // CHECK-EMPTY:
-// CHECK: [ B4 (live expressions at block exit) ]
+// 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: [ B4 (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: [ B5 (live expressions at block exit) ]
 // CHECK-EMPTY:
@@ -228,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
@@ -247,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
@@ -266,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
@@ -285,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

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hope that this fixes the issue.

@steakhal steakhal merged commit f378e52 into llvm:main Feb 17, 2025
12 checks passed
@steakhal steakhal deleted the bb/fix-flaky-live-stmts-test branch February 17, 2025 10:13
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
…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
@tstellar tstellar added this to the LLVM 20.X Release milestone Feb 26, 2025
@tstellar
Copy link
Collaborator

/cherry-pick f378e52

@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

Failed to cherry-pick: f378e52

https://github.com/llvm/llvm-project/actions/runs/13534670364

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

@tstellar tstellar moved this from Needs Triage to Needs Pull Request in LLVM Release Status Mar 11, 2025
@tstellar
Copy link
Collaborator

Is there interest in backporting this to the release branch? If so, can someone manually create a PR?

steakhal added a commit to steakhal/llvm-project that referenced this pull request May 12, 2025
…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)
@steakhal
Copy link
Contributor Author

Is there interest in backporting this to the release branch? If so, can someone manually create a PR?

Thanks for the heads up. Here is the manual backport: #139591 @tstellar

swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request May 13, 2025
…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)
@tstellar tstellar moved this from Needs Backport PR to Done in LLVM Release Status May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Clang test Analysis/live-stmts.cpp randomly fails on MacOS clang/test/Analysis/live-stmts.cpp frequently flaky on aarch64
4 participants