[analyzer] Fix a crash from element region construction during `ArrayInitLoopExpr` analysis #113570

isuckatcs · 2024-10-24T14:07:38Z

This patch generalizes the way element regions are constructed when an ArrayInitLoopExpr is being analyzed. Previously the base region of the ElementRegion was determined with pattern matching, which led to crashes, when an unhandled pattern was encountered.

Fixes #112813

llvmbot · 2024-10-24T14:08:13Z

@llvm/pr-subscribers-clang

Author: None (isuckatcs)

Changes

This patch generalizes the way element regions are constructed when an ArrayInitLoopExpr is being analyzed. Previously the base region of the ElementRegion was determined with pattern matching, which led to crashes, when an unhandled pattern was encountered.

Fixes #112813

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

2 Files Affected:

(modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+12-57)
(modified) clang/test/Analysis/array-init-loop.cpp (+38)

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index c50db1e0e2f863..6ddb9b44ddcf91 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -513,70 +513,25 @@ ProgramStateRef ExprEngine::updateObjectsUnderConstruction(
 static ProgramStateRef
 bindRequiredArrayElementToEnvironment(ProgramStateRef State,
                                       const ArrayInitLoopExpr *AILE,
-                                      const LocationContext *LCtx, SVal Idx) {
-  // The ctor in this case is guaranteed to be a copy ctor, otherwise we hit a
-  // compile time error.
-  //
-  //  -ArrayInitLoopExpr                <-- we're here
-  //   |-OpaqueValueExpr
-  //   | `-DeclRefExpr                  <-- match this
-  //   `-CXXConstructExpr
-  //     `-ImplicitCastExpr
-  //       `-ArraySubscriptExpr
-  //         |-ImplicitCastExpr
-  //         | `-OpaqueValueExpr
-  //         |   `-DeclRefExpr
-  //         `-ArrayInitIndexExpr
-  //
-  // The resulting expression might look like the one below in an implicit
-  // copy/move ctor.
-  //
-  //   ArrayInitLoopExpr                <-- we're here
-  //   |-OpaqueValueExpr
-  //   | `-MemberExpr                   <-- match this
-  //   |  (`-CXXStaticCastExpr)         <-- move ctor only
-  //   |     `-DeclRefExpr
-  //   `-CXXConstructExpr
-  //     `-ArraySubscriptExpr
-  //       |-ImplicitCastExpr
-  //       | `-OpaqueValueExpr
-  //       |   `-MemberExpr
-  //       |     `-DeclRefExpr
-  //       `-ArrayInitIndexExpr
-  //
-  // The resulting expression for a multidimensional array.
-  // ArrayInitLoopExpr                  <-- we're here
-  // |-OpaqueValueExpr
-  // | `-DeclRefExpr                    <-- match this
-  // `-ArrayInitLoopExpr
-  //   |-OpaqueValueExpr
-  //   | `-ArraySubscriptExpr
-  //   |   |-ImplicitCastExpr
-  //   |   | `-OpaqueValueExpr
-  //   |   |   `-DeclRefExpr
-  //   |   `-ArrayInitIndexExpr
-  //   `-CXXConstructExpr             <-- extract this
-  //     ` ...
-
-  const auto *OVESrc = AILE->getCommonExpr()->getSourceExpr();
+                                      const LocationContext *LCtx, NonLoc Idx) {
+  SValBuilder &SVB = State->getStateManager().getSValBuilder();
+  MemRegionManager &MRMgr = SVB.getRegionManager();
+  ASTContext &Ctx = SVB.getContext();
 
   // HACK: There is no way we can put the index of the array element into the
   // CFG unless we unroll the loop, so we manually select and bind the required
   // parameter to the environment.
-  const auto *CE =
+  const Expr *SourceArray = AILE->getCommonExpr()->getSourceExpr();
+  const auto *Ctor =
       cast<CXXConstructExpr>(extractElementInitializerFromNestedAILE(AILE));
 
-  SVal Base = UnknownVal();
-  if (const auto *ME = dyn_cast<MemberExpr>(OVESrc))
-    Base = State->getSVal(ME, LCtx);
-  else if (const auto *DRE = dyn_cast<DeclRefExpr>(OVESrc))
-    Base = State->getLValue(cast<VarDecl>(DRE->getDecl()), LCtx);
-  else
-    llvm_unreachable("ArrayInitLoopExpr contains unexpected source expression");
-
-  SVal NthElem = State->getLValue(CE->getType(), Idx, Base);
+  const SubRegion *SourceArrayRegion =
+      cast<SubRegion>(State->getSVal(SourceArray, LCtx).getAsRegion());
+  const ElementRegion *ElementRegion =
+      MRMgr.getElementRegion(Ctor->getType(), Idx, SourceArrayRegion, Ctx);
 
-  return State->BindExpr(CE->getArg(0), LCtx, NthElem);
+  return State->BindExpr(Ctor->getArg(0), LCtx,
+                         loc::MemRegionVal(ElementRegion));
 }
 
 void ExprEngine::handleConstructor(const Expr *E,
diff --git a/clang/test/Analysis/array-init-loop.cpp b/clang/test/Analysis/array-init-loop.cpp
index 4ab4489fc882f3..64b0aec1daf9d2 100644
--- a/clang/test/Analysis/array-init-loop.cpp
+++ b/clang/test/Analysis/array-init-loop.cpp
@@ -330,3 +330,41 @@ void no_crash() {
 }
 
 } // namespace crash
+
+namespace array_subscript_initializer {
+
+struct S
+{
+  int x;
+};
+
+void no_crash() {
+  S arr[][2] = {{1, 2}};
+
+  const auto [a, b] = arr[0];
+
+  clang_analyzer_eval(a.x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(b.x == 2); // expected-warning{{TRUE}}
+}
+
+} // namespace array_subscript_initializer
+
+namespace iterator_initializer {
+
+struct S
+{
+  int x;
+};
+
+void no_crash() {
+  S arr[][2] = {{1, 2}, {3, 4}};
+
+  int i = 0;
+  for (const auto [a, b] : arr) {
+    clang_analyzer_eval(a.x == (i == 0 ? 1 : 3)); // expected-warning{{TRUE}}
+    clang_analyzer_eval(b.x == (i == 0 ? 2 : 4)); // expected-warning{{TRUE}}
+    ++i;
+  }
+}
+
+} // namespace iterator_initializer
\ No newline at end of file

llvmbot · 2024-10-24T14:08:14Z

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

Author: None (isuckatcs)

Changes

This patch generalizes the way element regions are constructed when an ArrayInitLoopExpr is being analyzed. Previously the base region of the ElementRegion was determined with pattern matching, which led to crashes, when an unhandled pattern was encountered.

Fixes #112813

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

2 Files Affected:

(modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+12-57)
(modified) clang/test/Analysis/array-init-loop.cpp (+38)

diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index c50db1e0e2f863..6ddb9b44ddcf91 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -513,70 +513,25 @@ ProgramStateRef ExprEngine::updateObjectsUnderConstruction(
 static ProgramStateRef
 bindRequiredArrayElementToEnvironment(ProgramStateRef State,
                                       const ArrayInitLoopExpr *AILE,
-                                      const LocationContext *LCtx, SVal Idx) {
-  // The ctor in this case is guaranteed to be a copy ctor, otherwise we hit a
-  // compile time error.
-  //
-  //  -ArrayInitLoopExpr                <-- we're here
-  //   |-OpaqueValueExpr
-  //   | `-DeclRefExpr                  <-- match this
-  //   `-CXXConstructExpr
-  //     `-ImplicitCastExpr
-  //       `-ArraySubscriptExpr
-  //         |-ImplicitCastExpr
-  //         | `-OpaqueValueExpr
-  //         |   `-DeclRefExpr
-  //         `-ArrayInitIndexExpr
-  //
-  // The resulting expression might look like the one below in an implicit
-  // copy/move ctor.
-  //
-  //   ArrayInitLoopExpr                <-- we're here
-  //   |-OpaqueValueExpr
-  //   | `-MemberExpr                   <-- match this
-  //   |  (`-CXXStaticCastExpr)         <-- move ctor only
-  //   |     `-DeclRefExpr
-  //   `-CXXConstructExpr
-  //     `-ArraySubscriptExpr
-  //       |-ImplicitCastExpr
-  //       | `-OpaqueValueExpr
-  //       |   `-MemberExpr
-  //       |     `-DeclRefExpr
-  //       `-ArrayInitIndexExpr
-  //
-  // The resulting expression for a multidimensional array.
-  // ArrayInitLoopExpr                  <-- we're here
-  // |-OpaqueValueExpr
-  // | `-DeclRefExpr                    <-- match this
-  // `-ArrayInitLoopExpr
-  //   |-OpaqueValueExpr
-  //   | `-ArraySubscriptExpr
-  //   |   |-ImplicitCastExpr
-  //   |   | `-OpaqueValueExpr
-  //   |   |   `-DeclRefExpr
-  //   |   `-ArrayInitIndexExpr
-  //   `-CXXConstructExpr             <-- extract this
-  //     ` ...
-
-  const auto *OVESrc = AILE->getCommonExpr()->getSourceExpr();
+                                      const LocationContext *LCtx, NonLoc Idx) {
+  SValBuilder &SVB = State->getStateManager().getSValBuilder();
+  MemRegionManager &MRMgr = SVB.getRegionManager();
+  ASTContext &Ctx = SVB.getContext();
 
   // HACK: There is no way we can put the index of the array element into the
   // CFG unless we unroll the loop, so we manually select and bind the required
   // parameter to the environment.
-  const auto *CE =
+  const Expr *SourceArray = AILE->getCommonExpr()->getSourceExpr();
+  const auto *Ctor =
       cast<CXXConstructExpr>(extractElementInitializerFromNestedAILE(AILE));
 
-  SVal Base = UnknownVal();
-  if (const auto *ME = dyn_cast<MemberExpr>(OVESrc))
-    Base = State->getSVal(ME, LCtx);
-  else if (const auto *DRE = dyn_cast<DeclRefExpr>(OVESrc))
-    Base = State->getLValue(cast<VarDecl>(DRE->getDecl()), LCtx);
-  else
-    llvm_unreachable("ArrayInitLoopExpr contains unexpected source expression");
-
-  SVal NthElem = State->getLValue(CE->getType(), Idx, Base);
+  const SubRegion *SourceArrayRegion =
+      cast<SubRegion>(State->getSVal(SourceArray, LCtx).getAsRegion());
+  const ElementRegion *ElementRegion =
+      MRMgr.getElementRegion(Ctor->getType(), Idx, SourceArrayRegion, Ctx);
 
-  return State->BindExpr(CE->getArg(0), LCtx, NthElem);
+  return State->BindExpr(Ctor->getArg(0), LCtx,
+                         loc::MemRegionVal(ElementRegion));
 }
 
 void ExprEngine::handleConstructor(const Expr *E,
diff --git a/clang/test/Analysis/array-init-loop.cpp b/clang/test/Analysis/array-init-loop.cpp
index 4ab4489fc882f3..64b0aec1daf9d2 100644
--- a/clang/test/Analysis/array-init-loop.cpp
+++ b/clang/test/Analysis/array-init-loop.cpp
@@ -330,3 +330,41 @@ void no_crash() {
 }
 
 } // namespace crash
+
+namespace array_subscript_initializer {
+
+struct S
+{
+  int x;
+};
+
+void no_crash() {
+  S arr[][2] = {{1, 2}};
+
+  const auto [a, b] = arr[0];
+
+  clang_analyzer_eval(a.x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(b.x == 2); // expected-warning{{TRUE}}
+}
+
+} // namespace array_subscript_initializer
+
+namespace iterator_initializer {
+
+struct S
+{
+  int x;
+};
+
+void no_crash() {
+  S arr[][2] = {{1, 2}, {3, 4}};
+
+  int i = 0;
+  for (const auto [a, b] : arr) {
+    clang_analyzer_eval(a.x == (i == 0 ? 1 : 3)); // expected-warning{{TRUE}}
+    clang_analyzer_eval(b.x == (i == 0 ? 2 : 4)); // expected-warning{{TRUE}}
+    ++i;
+  }
+}
+
+} // namespace iterator_initializer
\ No newline at end of file

github-actions · 2024-10-24T14:09:32Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

NagyDonat

LGTM, thanks for fixing this crash!

I also happy to see that you simplify the logic and clean up the variable names. (I was a bit curious about the original reason that led to introduce this pattern matching, but I see that you're the original author of this code so I presume that you understand its role.)

isuckatcs · 2024-10-24T15:52:18Z

I presume that you understand its role

I don't remember why I did it with pattern matching. I guess, I wasn't aware that getSVal() will return the binding even if it's enveloped by other expressions.

steakhal

Could you please add no-crash comment in the test code where we would have crashed?

Other than this, this looks good to me.
Thabks for the prompt fix!

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

clang/test/Analysis/array-init-loop.cpp

steakhal

Still looks good.
I couldnt find the "no-crash" comments in the tests at the statements where they crashed but im fine without them too.

isuckatcs · 2024-10-25T17:16:51Z

clang/test/Analysis/array-init-loop.cpp

+void no_crash() {
+  S arr[][2] = {{1, 2}};
+
+  const auto [a, b] = arr[0]; // no-crash


@steakhal // no-crash at the end of the line. Similarly in the other test case.

My bad. Im too tired now.

…InitLoopExpr` analysis This patch generalizes the way element regions are constructed when an `ArrayInitLoopExpr` is being analyzed. Previously the base region of the `ElementRegion` was determined with pattern matching, which led to crashes, when an unhandled pattern was encountered. Fixes llvm#112813

isuckatcs · 2024-10-26T15:44:39Z

@steakhal I wanted to wait for the pipeline to pass before merging. Shouldn't we only merge patches with a green pipeline?

steakhal · 2024-10-26T15:48:27Z

@steakhal I wanted to wait for the pipeline to pass before merging. Shouldn't we only merge patches with a green pipeline?

The full pipeline will never be green. Downstream I wouldn't do this, but here I think we did what is reasonable. Waited enough. We can always revert with a single push of a button.

llvm-ci · 2024-10-26T16:18:22Z

LLVM Buildbot has detected a new failure on builder openmp-offload-libc-amdgpu-runtime running on omp-vega20-1 while building clang at step 10 "Add check check-offload".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7599

Here is the relevant piece of the build log for the reference

Step 10 (Add check check-offload) failure: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
...
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug47654.cpp (866 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/test_libc.cpp (867 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49779.cpp (868 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug50022.cpp (869 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/wtime.c (870 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/bug49021.cpp (871 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/std_complex_arithmetic.cpp (872 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/complex_reduction.cpp (873 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49021.cpp (874 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/std_complex_arithmetic.cpp (875 of 879)
command timed out: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1237.972714

…InitLoopExpr` analysis (llvm#113570) This patch generalizes the way element regions are constructed when an `ArrayInitLoopExpr` is being analyzed. Previously the base region of the `ElementRegion` was determined with pattern matching, which led to crashes, when an unhandled pattern was encountered. Fixes llvm#112813

isuckatcs requested review from NagyDonat, Szelethus, Xazax-hun, haoNoQ and steakhal October 24, 2024 14:07

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Oct 24, 2024

isuckatcs force-pushed the pr112813 branch from 902980e to ba10a34 Compare October 24, 2024 14:08

isuckatcs force-pushed the pr112813 branch from ba10a34 to a5fbcd5 Compare October 24, 2024 14:10

NagyDonat approved these changes Oct 24, 2024

View reviewed changes

steakhal approved these changes Oct 25, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp Outdated Show resolved Hide resolved

clang/test/Analysis/array-init-loop.cpp Show resolved Hide resolved

isuckatcs force-pushed the pr112813 branch from a5fbcd5 to f6f54e9 Compare October 25, 2024 12:28

isuckatcs requested a review from steakhal October 25, 2024 12:34

steakhal approved these changes Oct 25, 2024

View reviewed changes

isuckatcs commented Oct 25, 2024

View reviewed changes

isuckatcs requested a review from steakhal October 25, 2024 17:16

isuckatcs force-pushed the pr112813 branch from f6f54e9 to dfb862d Compare October 25, 2024 17:25

steakhal approved these changes Oct 26, 2024

View reviewed changes

steakhal merged commit a917ae0 into llvm:main Oct 26, 2024
6 of 8 checks passed

[analyzer] Fix a crash from element region construction during ArrayInitLoopExpr analysis #113570

[analyzer] Fix a crash from element region construction during ArrayInitLoopExpr analysis #113570

Uh oh!

Conversation

isuckatcs commented Oct 24, 2024

Uh oh!

llvmbot commented Oct 24, 2024

Uh oh!

llvmbot commented Oct 24, 2024

Uh oh!

github-actions bot commented Oct 24, 2024

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

isuckatcs commented Oct 24, 2024

Uh oh!

steakhal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steakhal left a comment

Choose a reason for hiding this comment

Uh oh!

isuckatcs Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

steakhal Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isuckatcs commented Oct 26, 2024

Uh oh!

steakhal commented Oct 26, 2024

Uh oh!

llvm-ci commented Oct 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[analyzer] Fix a crash from element region construction during `ArrayInitLoopExpr` analysis #113570

[analyzer] Fix a crash from element region construction during `ArrayInitLoopExpr` analysis #113570