[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)

[via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/72402 at github.com>


================
@@ -81,22 +81,21 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call,
 
   case Builtin::BI__builtin_alloca_with_align:
   case Builtin::BI__builtin_alloca: {
-    // FIXME: Refactor into StoreManager itself?
-    MemRegionManager& RM = C.getStoreManager().getRegionManager();
-    const AllocaRegion* R =
-      RM.getAllocaRegion(CE, C.blockCount(), C.getLocationContext());
-
-    // Set the extent of the region in bytes. This enables us to use the
-    // SVal of the argument directly. If we save the extent in bits, we
-    // cannot represent values like symbol*8.
-    auto Size = Call.getArgSVal(0);
-    if (Size.isUndef())
-      return true; // Return true to model purity.
-
-    state = setDynamicExtent(state, R, Size.castAs<DefinedOrUnknownSVal>(),
-                             C.getSValBuilder());
+    SValBuilder &SVB = C.getSValBuilder();
+    const loc::MemRegionVal R =
+        SVB.getAllocaRegionVal(CE, C.getLocationContext(), C.blockCount());
 
-    C.addTransition(state->BindExpr(CE, LCtx, loc::MemRegionVal(R)));
+    // Set the extent of the region in bytes. This enables us to use the SVal
+    // of the argument directly. If we saved the extent in bits, it'd be more
+    // difficult to reason about values like symbol*8.
+    auto Size = Call.getArgSVal(0);
+    if (auto DefSize = Size.getAs<DefinedOrUnknownSVal>()) {
+      state = setDynamicExtent(state, R.getRegion(), *DefSize, SVB);
+      // FIXME: perhaps the following transition should be moved out of the
----------------
DonatNagyE wrote:

I checked the situation with an assert and a straightforward testcase, and it turns out that currently the `getAs()` always succeeds, because `core.CallAndMessage` stops the execution with a bug report when an undefined argument is passed to a function. (However, if I disabled this functionality of `core.CallAndMessage`, the test -- obviously -- triggered the assertion.)

Based on this, I decided to keep the current defensive check, because I didn't want to add a hidden and marginal dependency relationship between these two checkers. I know that we may assume that `core` checkers are always on, but that doesn't mean that I should hide landmines like `assert(core.CallAndMessage is active and works as I assume)` in unrelated checkers.

I updated the comment to explain this situation, and I also moved the `addTransition()` call out of the `if` because that's the more logical place for that method call and this `supporting fire` from `core.CallAndMessage` guarantees that this change won't have unintended side effects (because the `if` succeeds in normal operation).

https://github.com/llvm/llvm-project/pull/72402