https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/191168
From 472de5dc1e86e8e725830a7c2187dc48ad4e4b85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 9 Apr 2026 11:21:18 +0200 Subject: [PATCH 1/3] [analyzer][NFC] Document getVarRegion shortcomings ... more precisely, that the analyzer currently uses `NonParamVarRegion` instances to represent parameters of the entrypoint stack frame and parameters that are captured by inner lambdas or blocks. In my recent commit f40c234191802154d5b3fc3209908c3f2d6e1649 I added a FIXME note to the method `MemRegionManager::getVarRegion`, but now, as I tried to implement that I realized that the situation is more complex and would need a more through change. As I don't have time to do this right now, I'm pushing this commit to remove the inaccurate FIXME and replace it with more accurate TODO notes that explain the current (problematic) behavior of the codebase. --- .../Core/PathSensitive/MemRegion.h | 4 +++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp | 29 ++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h index c59413abc352d..df27ee3204aad 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h @@ -1007,6 +1007,10 @@ class VarRegion : public DeclRegion { } }; +// TODO: Currently MemRegionManager::getVarRegion returns NonParamVarRegion +// instances to represent the parameters of the entrypoint stack frame and +// parameters of outer stack frames that appear as captured within a lambda or +// a block. This should be overhauled. class NonParamVarRegion : public VarRegion { friend class MemRegionManager; diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp index 5544d254929e5..f0437d25214cc 100644 --- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp +++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp @@ -1047,23 +1047,36 @@ const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D, const Expr *CallSite = SFC->getCallSite(); if (CallSite) { const Decl *CalleeDecl = SFC->getDecl(); - bool ValidParam = true; + bool CurrentParam = true; if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecl)) { - ValidParam = + CurrentParam = (Index < FD->param_size() && FD->getParamDecl(Index) == PVD); + assert(CurrentParam); } else if (const auto *BD = dyn_cast<BlockDecl>(CalleeDecl)) { - ValidParam = + CurrentParam = (Index < BD->param_size() && BD->getParamDecl(Index) == PVD); } - if (ValidParam) { + if (CurrentParam) { + // If this is a parameter of the *current* stack frame, we can + // represent it with a `ParamVarRegion`. return getSubRegion<ParamVarRegion>(CallSite, Index, getStackArgumentsRegion(SFC)); + } else { + // TODO: Parameters of other stack frames (which may have been be + // captured by a lambda or a block) are currently represented by + // `NonParamVarRegion`s. This behavior is present since commit + // 98db1f990fc273adc1ae36d4ce97ce66fd27ac30 which introduced + // `ParamVarRegion` in 2020; and appears to work (at least to some + // extent); but it would be nice to clean this up (if somebody has time + // and knowledge for a proper investigation). } - // FIXME: If ValidParam was false, this method would "fall through" and - // eventually return a NonParamVarRegion for this ParamVarDecl. That - // seems to be buggy, so I strongly suspect that ValidParam always ends - // up being true. + } else { + // TODO: Parameters of the entrypoint stack frame (where `CallSite` is + // null) are currently represented by `NonParamVarRegion`s. This behavior + // is also present since 98db1f990fc273adc1ae36d4ce97ce66fd27ac30 which + // introduced `ParamVarRegion` in 2020, but it would be nice ot clean it + // up for the sake of clarity and consistency. } } From 2b09ac5d7a099ccf702aec9b4d4a1bf561a0b08a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 9 Apr 2026 15:59:22 +0200 Subject: [PATCH 2/3] Remove stray assertion I added this when I was experimenting and forgot to remove it. This was responsible for the test failure. --- clang/lib/StaticAnalyzer/Core/MemRegion.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp index f0437d25214cc..a36ef49f97160 100644 --- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp +++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp @@ -1051,7 +1051,6 @@ const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D, if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecl)) { CurrentParam = (Index < FD->param_size() && FD->getParamDecl(Index) == PVD); - assert(CurrentParam); } else if (const auto *BD = dyn_cast<BlockDecl>(CalleeDecl)) { CurrentParam = (Index < BD->param_size() && BD->getParamDecl(Index) == PVD); From 941e119c8e1467efc7f397ff3eda030de2707790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 9 Apr 2026 16:07:05 +0200 Subject: [PATCH 3/3] Improve comments --- .../clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h | 3 ++- clang/lib/StaticAnalyzer/Core/MemRegion.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h index df27ee3204aad..31813f59ab748 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h @@ -1056,7 +1056,8 @@ class NonParamVarRegion : public VarRegion { /// ParamVarRegion - Represents a region for parameters. Only parameters of the /// function in the current stack frame are represented as `ParamVarRegion`s. /// Parameters of top-level analyzed functions as well as captured paremeters -/// by lambdas and blocks are repesented as `VarRegion`s. +/// by lambdas and blocks are repesented as `NonParamVarRegion`s. +/// TODO: It would be nice to make this more consistent. // FIXME: `ParamVarRegion` only supports parameters of functions, C++ // constructors, blocks and Objective-C methods with existing `Decl`. Upon diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp index a36ef49f97160..941cb2fa2a103 100644 --- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp +++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp @@ -1074,7 +1074,7 @@ const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D, // TODO: Parameters of the entrypoint stack frame (where `CallSite` is // null) are currently represented by `NonParamVarRegion`s. This behavior // is also present since 98db1f990fc273adc1ae36d4ce97ce66fd27ac30 which - // introduced `ParamVarRegion` in 2020, but it would be nice ot clean it + // introduced `ParamVarRegion` in 2020, but it would be nice to clean it // up for the sake of clarity and consistency. } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
