[analyzer] Unknown array lvalue element in Store #133381

T-Gruber · 2025-03-28T06:49:14Z

Remove the early return for BaseRegions of type ElementRegion. Return meaningful MemRegionVal for these cases as well.
Previous discussion: https://discourse.llvm.org/t/lvalueelement-returns-unknownval-for-multi-dimensional-arrays/85476

llvmbot · 2025-03-28T06:49:44Z

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

Author: None (T-Gruber)

Changes

Remove the early return for BaseRegions of type ElementRegion. Return meaningful MemRegionVal for these cases as well.
Previous discussion: https://discourse.llvm.org/t/lvalueelement-returns-unknownval-for-multi-dimensional-arrays/85476

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

3 Files Affected:

(modified) clang/lib/StaticAnalyzer/Core/Store.cpp (+1-5)
(modified) clang/unittests/StaticAnalyzer/CMakeLists.txt (+1)
(added) clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp (+146)

diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp
index 5f30fae4b7047..da6885ecd0ec5 100644
--- a/clang/lib/StaticAnalyzer/Core/Store.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -511,13 +511,9 @@ SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset,
   // Only allow non-integer offsets if the base region has no offset itself.
   // FIXME: This is a somewhat arbitrary restriction. We should be using
   // SValBuilder here to add the two offsets without checking their types.
-  if (!isa<nonloc::ConcreteInt>(Offset)) {
-    if (isa<ElementRegion>(BaseRegion->StripCasts()))
-      return UnknownVal();
-
+  if (!isa<nonloc::ConcreteInt>(Offset))
     return loc::MemRegionVal(MRMgr.getElementRegion(
         elementType, Offset, cast<SubRegion>(ElemR->getSuperRegion()), Ctx));
-  }
 
   const llvm::APSInt& OffI = Offset.castAs<nonloc::ConcreteInt>().getValue();
   assert(BaseIdxI.isSigned());
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 3b01a4e9e5327..c95dc39c0c001 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -19,6 +19,7 @@ add_clang_unittest(StaticAnalysisTests
   ParamRegionTest.cpp
   RangeSetTest.cpp
   RegisterCustomCheckersTest.cpp
+  StoreManagerLValueElement.cpp
   StoreTest.cpp
   SymbolReaperTest.cpp
   SValSimplifyerTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp b/clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp
new file mode 100644
index 0000000000000..c00e431d53515
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp
@@ -0,0 +1,146 @@
+//===- LValueElementTest.cpp -----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class LValueElementChecker
+    : public Checker<check::PreStmt<ArraySubscriptExpr>> {
+public:
+  void checkPreStmt(const ArraySubscriptExpr *ASE, CheckerContext &CC) const {
+    const Expr *BaseEx = ASE->getBase()->IgnoreParens();
+    const Expr *IdxEx = ASE->getIdx()->IgnoreParens();
+
+    SVal BaseVal = CC.getSVal(BaseEx);
+    SVal IdxVal = CC.getSVal(IdxEx);
+
+    auto IdxNonLoc = IdxVal.getAs<NonLoc>();
+    ASSERT_TRUE(IdxNonLoc) << "Expect NonLoc as index SVal\n";
+
+    QualType ArrayT = ASE->getType();
+    SVal LValue =
+        CC.getStoreManager().getLValueElement(ArrayT, *IdxNonLoc, BaseVal);
+
+    if (ExplodedNode *Node = CC.generateNonFatalErrorNode(CC.getState())) {
+      std::string TmpStr;
+      llvm::raw_string_ostream TmpStream{TmpStr};
+      LValue.dumpToStream(TmpStream);
+      auto Report = std::make_unique<PathSensitiveBugReport>(Bug, TmpStr, Node);
+      CC.emitReport(std::move(Report));
+    }
+  }
+
+private:
+  const BugType Bug{this, "LValueElementBug"};
+};
+
+void addLValueElementChecker(AnalysisASTConsumer &AnalysisConsumer,
+                             AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"LValueElementChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<LValueElementChecker>("LValueElementChecker", "Desc",
+                                              "DocsURI");
+  });
+}
+
+bool runLValueElementChecker(StringRef Code, std::string &Output) {
+  return runCheckerOnCode<addLValueElementChecker>(Code.str(), Output,
+                                                   /*OnlyEmitWarnings=*/true);
+}
+
+TEST(LValueElementTest, IdxConInt) {
+  StringRef Code = R"cpp(
+const int index = 1;
+extern int array[3];
+
+void top() {
+  array[index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(Output, "LValueElementChecker: &Element{array,1 S64b,int}\n");
+}
+
+TEST(LValueElementTest, IdxSymVal) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int array[3];
+
+void top() {
+  array[un_index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(Output,
+            "LValueElementChecker: &Element{array,reg_$0<int un_index>,int}\n");
+}
+
+TEST(LValueElementTest, IdxConIntSymVal) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int matrix[3][3];
+
+void top() {
+  matrix[1][un_index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(Output, "LValueElementChecker: &Element{Element{matrix,1 "
+                    "S64b,int[3]},reg_$0<int un_index>,int}\n"
+                    "LValueElementChecker: &Element{matrix,1 S64b,int[3]}\n");
+}
+
+TEST(LValueElementTest, IdxSymValConInt) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int matrix[3][3];
+
+void top() {
+  matrix[un_index][1];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(
+      Output,
+      "LValueElementChecker: &Element{Element{matrix,reg_$0<int "
+      "un_index>,int[3]},1 S64b,int}\n"
+      "LValueElementChecker: &Element{matrix,reg_$0<int un_index>,int[3]}\n");
+}
+
+TEST(LValueElementTest, IdxSymValSymVal) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int matrix[3][3];
+
+void top() {
+  matrix[un_index][un_index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(
+      Output,
+      "LValueElementChecker: &Element{Element{matrix,reg_$0<int "
+      "un_index>,int[3]},reg_$0<int un_index>,int}\n"
+      "LValueElementChecker: &Element{matrix,reg_$0<int un_index>,int[3]}\n");
+}
+
+} // namespace

llvmbot · 2025-03-28T06:49:44Z

@llvm/pr-subscribers-clang

Author: None (T-Gruber)

Changes

Remove the early return for BaseRegions of type ElementRegion. Return meaningful MemRegionVal for these cases as well.
Previous discussion: https://discourse.llvm.org/t/lvalueelement-returns-unknownval-for-multi-dimensional-arrays/85476

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

3 Files Affected:

(modified) clang/lib/StaticAnalyzer/Core/Store.cpp (+1-5)
(modified) clang/unittests/StaticAnalyzer/CMakeLists.txt (+1)
(added) clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp (+146)

diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp
index 5f30fae4b7047..da6885ecd0ec5 100644
--- a/clang/lib/StaticAnalyzer/Core/Store.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -511,13 +511,9 @@ SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset,
   // Only allow non-integer offsets if the base region has no offset itself.
   // FIXME: This is a somewhat arbitrary restriction. We should be using
   // SValBuilder here to add the two offsets without checking their types.
-  if (!isa<nonloc::ConcreteInt>(Offset)) {
-    if (isa<ElementRegion>(BaseRegion->StripCasts()))
-      return UnknownVal();
-
+  if (!isa<nonloc::ConcreteInt>(Offset))
     return loc::MemRegionVal(MRMgr.getElementRegion(
         elementType, Offset, cast<SubRegion>(ElemR->getSuperRegion()), Ctx));
-  }
 
   const llvm::APSInt& OffI = Offset.castAs<nonloc::ConcreteInt>().getValue();
   assert(BaseIdxI.isSigned());
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 3b01a4e9e5327..c95dc39c0c001 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -19,6 +19,7 @@ add_clang_unittest(StaticAnalysisTests
   ParamRegionTest.cpp
   RangeSetTest.cpp
   RegisterCustomCheckersTest.cpp
+  StoreManagerLValueElement.cpp
   StoreTest.cpp
   SymbolReaperTest.cpp
   SValSimplifyerTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp b/clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp
new file mode 100644
index 0000000000000..c00e431d53515
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp
@@ -0,0 +1,146 @@
+//===- LValueElementTest.cpp -----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class LValueElementChecker
+    : public Checker<check::PreStmt<ArraySubscriptExpr>> {
+public:
+  void checkPreStmt(const ArraySubscriptExpr *ASE, CheckerContext &CC) const {
+    const Expr *BaseEx = ASE->getBase()->IgnoreParens();
+    const Expr *IdxEx = ASE->getIdx()->IgnoreParens();
+
+    SVal BaseVal = CC.getSVal(BaseEx);
+    SVal IdxVal = CC.getSVal(IdxEx);
+
+    auto IdxNonLoc = IdxVal.getAs<NonLoc>();
+    ASSERT_TRUE(IdxNonLoc) << "Expect NonLoc as index SVal\n";
+
+    QualType ArrayT = ASE->getType();
+    SVal LValue =
+        CC.getStoreManager().getLValueElement(ArrayT, *IdxNonLoc, BaseVal);
+
+    if (ExplodedNode *Node = CC.generateNonFatalErrorNode(CC.getState())) {
+      std::string TmpStr;
+      llvm::raw_string_ostream TmpStream{TmpStr};
+      LValue.dumpToStream(TmpStream);
+      auto Report = std::make_unique<PathSensitiveBugReport>(Bug, TmpStr, Node);
+      CC.emitReport(std::move(Report));
+    }
+  }
+
+private:
+  const BugType Bug{this, "LValueElementBug"};
+};
+
+void addLValueElementChecker(AnalysisASTConsumer &AnalysisConsumer,
+                             AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"LValueElementChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+    Registry.addChecker<LValueElementChecker>("LValueElementChecker", "Desc",
+                                              "DocsURI");
+  });
+}
+
+bool runLValueElementChecker(StringRef Code, std::string &Output) {
+  return runCheckerOnCode<addLValueElementChecker>(Code.str(), Output,
+                                                   /*OnlyEmitWarnings=*/true);
+}
+
+TEST(LValueElementTest, IdxConInt) {
+  StringRef Code = R"cpp(
+const int index = 1;
+extern int array[3];
+
+void top() {
+  array[index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(Output, "LValueElementChecker: &Element{array,1 S64b,int}\n");
+}
+
+TEST(LValueElementTest, IdxSymVal) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int array[3];
+
+void top() {
+  array[un_index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(Output,
+            "LValueElementChecker: &Element{array,reg_$0<int un_index>,int}\n");
+}
+
+TEST(LValueElementTest, IdxConIntSymVal) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int matrix[3][3];
+
+void top() {
+  matrix[1][un_index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(Output, "LValueElementChecker: &Element{Element{matrix,1 "
+                    "S64b,int[3]},reg_$0<int un_index>,int}\n"
+                    "LValueElementChecker: &Element{matrix,1 S64b,int[3]}\n");
+}
+
+TEST(LValueElementTest, IdxSymValConInt) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int matrix[3][3];
+
+void top() {
+  matrix[un_index][1];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(
+      Output,
+      "LValueElementChecker: &Element{Element{matrix,reg_$0<int "
+      "un_index>,int[3]},1 S64b,int}\n"
+      "LValueElementChecker: &Element{matrix,reg_$0<int un_index>,int[3]}\n");
+}
+
+TEST(LValueElementTest, IdxSymValSymVal) {
+  StringRef Code = R"cpp(
+extern int un_index;
+extern int matrix[3][3];
+
+void top() {
+  matrix[un_index][un_index];
+})cpp";
+
+  std::string Output;
+  ASSERT_TRUE(runLValueElementChecker(Code, Output));
+  EXPECT_EQ(
+      Output,
+      "LValueElementChecker: &Element{Element{matrix,reg_$0<int "
+      "un_index>,int[3]},reg_$0<int un_index>,int}\n"
+      "LValueElementChecker: &Element{matrix,reg_$0<int un_index>,int[3]}\n");
+}
+
+} // namespace

NagyDonat

I'm really happy to see a fix for this weakness of the static analyzer, and overall I'm satisfied with the commit, but I didn't have time to think about the implications of this change (and picking the right representation: nested ElementRegions vs a single ElementRegion with a SymSymExpr index), so I'm not giving a formal approval yet.

I'll try to revisit this PR next week; feel free to ping me if I forget. If somebody else gives an approval, there is no need to wait for me.

clang/test/Analysis/ArrayBound/assumption-reporting.c

clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp

clang/lib/StaticAnalyzer/Core/Store.cpp

steakhal

LGTM, thanks.

Remove the early return for BaseRegions of type ElementRegion. Return meaningful MemRegionVal for these cases as well. Previous discussion: https://discourse.llvm.org/t/lvalueelement-returns-unknownval-for-multi-dimensional-arrays/85476

T-Gruber added 4 commits March 28, 2025 07:37

Remove early return for UnknownVal

241c95a

Unittests for StoreManager::getLValueElement

d993a7f

Run clang-format

0fefdd7

Remove unneeded includes

975bad2

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Mar 28, 2025

Adapt assumption report for ArrayBound

f55db17

NagyDonat requested review from Xazax-hun, haoNoQ, steakhal and NagyDonat March 28, 2025 09:42

NagyDonat reviewed Mar 28, 2025

View reviewed changes

clang/test/Analysis/ArrayBound/assumption-reporting.c Show resolved Hide resolved

clang/unittests/StaticAnalyzer/StoreManagerLValueElement.cpp Outdated Show resolved Hide resolved

clang/lib/StaticAnalyzer/Core/Store.cpp Show resolved Hide resolved

steakhal reviewed Mar 28, 2025

View reviewed changes

steakhal changed the title ~~Unknown array lvalue element~~ [analyzer] Unknown array lvalue element in Store Mar 28, 2025

T-Gruber added 2 commits March 31, 2025 07:19

Remove unittests

4ed4661

Implement lit tests

bdeffce

steakhal approved these changes Mar 31, 2025

View reviewed changes

steakhal merged commit d63cc4c into llvm:main Mar 31, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[analyzer] Unknown array lvalue element in Store #133381

[analyzer] Unknown array lvalue element in Store #133381

Uh oh!

T-Gruber commented Mar 28, 2025

Uh oh!

llvmbot commented Mar 28, 2025

Uh oh!

llvmbot commented Mar 28, 2025

Uh oh!

NagyDonat left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steakhal left a comment

Uh oh!

Uh oh!

Uh oh!

[analyzer] Unknown array lvalue element in Store #133381

[analyzer] Unknown array lvalue element in Store #133381

Uh oh!

Conversation

T-Gruber commented Mar 28, 2025

Uh oh!

llvmbot commented Mar 28, 2025

Uh oh!

llvmbot commented Mar 28, 2025

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steakhal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!