fix(analysis): read local constants from their MemorySSA clobber

CHANGELOG.md

@@ -4,6 +4,13 @@ Notable changes are grouped by release. See [all releases](https://github.com/Co
 
 ## Unreleased
 
+- VLA sizes, local stack sizes and the "this access appears unreachable" note on buffer
+  overflows read a local variable from the store that last wrote it, as MemorySSA finds it, and
+  apply casts to it. They used to take the last constant stored anywhere in the function. A
+  store after the read, a store on one branch only, or a store of another width changed the
+  result then, and `(unsigned char)300` read as 300. The note now also requires every edge into
+  the access to be dead. It no longer appears on accesses that run, such as an access after a
+  loop.
 - Building requires LLVM 20. CMake rejects an older LLVM when it configures the build, where
   LLVM 19 passed that check and then failed to compile. On macOS, `scripts/build.sh` looks for
   Homebrew `llvm@20` before `llvm`, a later version, and no longer for `llvm@19` or `llvm@18`.

include/analysis/IRValueUtils.hpp

@@ -16,6 +16,8 @@ namespace llvm
 
 namespace ctrace::stack::analysis
 {
+    class FunctionFacts;
+
     enum class LeadingUnderscores
     {
         One,
@@ -48,7 +50,13 @@ namespace ctrace::stack::analysis
 
     AllocaOrigin classifyAllocaOrigin(const llvm::AllocaInst* AI);
 
-    const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const llvm::Function& F);
+    /// The integer constant @p V holds wherever it is used, or null when that is not established.
+    ///
+    /// Follows casts, applying them to the constant, and loads whose MemorySSA clobber is a
+    /// store of the same type to the same pointer, so the store wrote exactly the bytes the load
+    /// reads. Any other clobber, such as a merge of several stores, a store that may only
+    /// overlap, a call or the function entry, gives null.
+    const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const FunctionFacts& facts);
 
     /// The sole store to a slot whose other uses are loads, debug or lifetime intrinsics.
     /// Returns null for multiple stores, address escapes or unsupported uses.

src/analysis/AllocaUsage.cpp

@@ -171,7 +171,7 @@ namespace ctrace::stack::analysis
                         issue.sizeIsConst = true;
                         issue.sizeBytes = C->getZExtValue() * elemSize;
                     }
-                    else if (auto* C = tryGetConstFromValue(arraySizeVal, F))
+                    else if (auto* C = tryGetConstFromValue(arraySizeVal, facts))
                     {
                         issue.sizeIsConst = true;
                         issue.sizeBytes = C->getZExtValue() * elemSize;

src/analysis/DynamicAlloca.cpp

@@ -8,8 +8,11 @@
 #include <llvm/IR/Value.h>
 #include <llvm/Support/raw_ostream.h>
 
+#include "analysis/FunctionFacts.hpp"
 #include "analysis/IRValueUtils.hpp"
 
+#include <optional>
+
 namespace ctrace::stack::analysis
 {
     namespace
@@ -22,6 +25,9 @@ namespace ctrace::stack::analysis
             if (F.isDeclaration())
                 return;
 
+            // Built on the first non-constant size only: most functions have no VLA.
+            std::optional<FunctionFacts> facts;
+
             for (BasicBlock& BB : F)
             {
                 for (Instruction& I : BB)
@@ -40,15 +46,15 @@ namespace ctrace::stack::analysis
                     if (llvm::isa<llvm::ConstantInt>(arraySizeVal))
                         continue; // compile-time known size, OK
 
-                    // 2) "Smart" heuristic: try to trace back to a constant
-                    //    via stores into a local variable (tryGetConstFromValue).
-                    //    Typical example:
+                    // 2) The size is a constant stored into a local variable, read back
+                    //    unchanged (tryGetConstFromValue). Typical example:
                     //      int n = 6;
                     //      char buf[n];   // in C: VLA, but here n is actually constant
                     //
                     //    In this case we don't want to spam with a VLA warning:
                     //    treat it as an effectively constant size.
-                    if (tryGetConstFromValue(arraySizeVal, F) != nullptr)
+                    if (tryGetConstFromValue(arraySizeVal, facts ? *facts : facts.emplace(F)) !=
+                        nullptr)
                         continue;
 
                     // 3) Here we consider it a real VLA / dynamic alloca
@@ -97,6 +103,7 @@ namespace ctrace::stack::analysis
     {
         std::vector<DynamicAllocaIssue> out;
         using namespace llvm;
+        std::optional<FunctionFacts> facts;
 
         for (const AllocaInst* AI : allocas)
         {
@@ -105,7 +112,10 @@ namespace ctrace::stack::analysis
             if (llvm::isa<llvm::ConstantInt>(arraySizeVal))
                 continue;
 
-            if (tryGetConstFromValue(arraySizeVal, function) != nullptr)
+            // FunctionFacts needs a mutable function for its analyses; they do not change it.
+            if (!facts)
+                facts.emplace(const_cast<Function&>(function));
+            if (tryGetConstFromValue(arraySizeVal, *facts) != nullptr)
                 continue;
 
             DynamicAllocaIssue issue;

src/analysis/IRValueUtils.cpp

@@ -1,8 +1,12 @@
 // SPDX-License-Identifier: Apache-2.0
 #include "analysis/IRValueUtils.hpp"
 
+#include "analysis/FunctionFacts.hpp"
+
 #include <llvm/ADT/SmallPtrSet.h>
 #include <llvm/ADT/SmallVector.h>
+#include <llvm/Analysis/ConstantFolding.h>
+#include <llvm/Analysis/MemorySSA.h>
 #include <llvm/IR/Constants.h>
 #include <llvm/IR/DebugInfo.h>
 #include <llvm/IR/DebugInfoMetadata.h>
@@ -299,46 +303,50 @@
         return AllocaOrigin::Unknown;
     }
 
-    const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const llvm::Function& F)
+    namespace
     {
-        using namespace llvm;
+        /// Bounds the walk; -O0 copy chains such as `int m = n;` need a few levels, and
+        /// unreachable code may hold a cycle of casts.
+        constexpr unsigned kMaxConstResolutionDepth = 16;
 
-        // First remove casts (sext/zext/trunc, etc.) to reach the real base value.
-        const Value* cur = V;
-        while (auto* cast = dyn_cast<const CastInst>(cur))
+        const llvm::ConstantInt* resolveConstant(const llvm::Value* value,
+                                                 const FunctionFacts& facts, unsigned depth)
         {
-            cur = cast->getOperand(0);
-        }
+            using namespace llvm;
 
-        // Trivial case: already an integer constant.
-        if (auto* C = dyn_cast<const ConstantInt>(cur))
-            return C;
-
-        // Typical -O0 case: comparing a load from a local variable.
-        auto* LI = dyn_cast<const LoadInst>(cur);
-        if (!LI)
-            return nullptr;
+            if (const auto* constant = dyn_cast<ConstantInt>(value))
+                return constant;
+            if (depth == 0)
+                return nullptr;
 
-        const Value* ptr = LI->getPointerOperand();
-        const ConstantInt* found = nullptr;
+            if (const auto* cast = dyn_cast<CastInst>(value))
+            {
+                const ConstantInt* operand = resolveConstant(cast->getOperand(0), facts, depth - 1);
+                if (!operand)
+                    return nullptr;
+                return dyn_cast_or_null<ConstantInt>(
+                    ConstantFoldCastOperand(cast->getOpcode(), const_cast<ConstantInt*>(operand),
+                                            cast->getType(), cast->getDataLayout()));
+            }
 
-        // Ultra-simple version: look for a constant store in the function.
-        for (const BasicBlock& BB : F)
-        {
-            for (const Instruction& I : BB)
+            const auto* load = dyn_cast<LoadInst>(value);
+            if (!load || !load->isSimple())
+                return nullptr;
+            const auto* def = dyn_cast_or_null<MemoryDef>(facts.clobberingAccess(*load));
+            const auto* store = def ? dyn_cast_or_null<StoreInst>(def->getMemoryInst()) : nullptr;
+            if (!store || !store->isSimple() ||
+                store->getPointerOperand()->stripPointerCasts() !=
+                    load->getPointerOperand()->stripPointerCasts() ||
+                store->getValueOperand()->getType() != load->getType())
             {
-                auto* SI = dyn_cast<const StoreInst>(&I);
-                if (!SI)
-                    continue;
-                if (SI->getPointerOperand() != ptr)
-                    continue;
-                if (auto* C = dyn_cast<const ConstantInt>(SI->getValueOperand()))
-                {
-                    // Keep the last constant found (if multiple stores, this is naive).
-                    found = C;
-                }
+                return nullptr;
             }
+            return resolveConstant(store->getValueOperand(), facts, depth - 1);
         }
-        return found;
+    } // namespace
+
+    const llvm::ConstantInt* tryGetConstFromValue(const llvm::Value* V, const FunctionFacts& facts)
+    {
+        return V ? resolveConstant(V, facts, kMaxConstResolutionDepth) : nullptr;
     }
 } // namespace ctrace::stack::analysis

src/analysis/Reachability.cpp

@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: Apache-2.0
 #include "analysis/Reachability.hpp"
 
+#include "analysis/FunctionFacts.hpp"
 #include "analysis/IRValueUtils.hpp"
 
+#include <optional>
+
 #include <llvm/IR/CFG.h>
 #include <llvm/IR/Constants.h>
 #include <llvm/IR/Function.h>
@@ -13,78 +16,38 @@ namespace ctrace::stack::analysis
 
     bool isStaticallyUnreachableStackAccess(const StackBufferOverflowIssue& issue)
     {
-        if (!issue.inst)
-            return false;
+        using namespace llvm;
 
-        auto* block = issue.inst->getParent();
-        if (!block)
+        const BasicBlock* block = issue.inst ? issue.inst->getParent() : nullptr;
+        if (!block || pred_empty(block))
             return false;
 
-        using namespace llvm;
-
-        for (auto* predecessor : predecessors(block))
+        std::optional<FunctionFacts> facts;
+        // Unreachable only when no edge into the block can be taken.
+        for (const BasicBlock* predecessor : predecessors(block))
         {
-            auto* branch = dyn_cast<BranchInst>(predecessor->getTerminator());
-            if (!branch || !branch->isConditional())
-                continue;
-
-            auto* compare = dyn_cast<ICmpInst>(branch->getCondition());
+            const auto* branch = dyn_cast<BranchInst>(predecessor->getTerminator());
+            const auto* compare = branch && branch->isConditional()
+                                      ? dyn_cast<ICmpInst>(branch->getCondition())
+                                      : nullptr;
             if (!compare)
-                continue;
+                return false;
 
-            const llvm::Function& function = *issue.inst->getFunction();
-            auto* lhs = analysis::tryGetConstFromValue(compare->getOperand(0), function);
-            auto* rhs = analysis::tryGetConstFromValue(compare->getOperand(1), function);
+            // FunctionFacts needs a mutable function for its analyses; they do not change it.
+            if (!facts)
+                facts.emplace(const_cast<Function&>(*block->getParent()));
+            const ConstantInt* lhs = tryGetConstFromValue(compare->getOperand(0), *facts);
+            const ConstantInt* rhs = tryGetConstFromValue(compare->getOperand(1), *facts);
             if (!lhs || !rhs)
-                continue;
-
-            bool condTrue = false;
-            const auto& lhsValue = lhs->getValue();
-            const auto& rhsValue = rhs->getValue();
-
-            switch (compare->getPredicate())
-            {
-            case ICmpInst::ICMP_EQ:
-                condTrue = (lhsValue == rhsValue);
-                break;
-            case ICmpInst::ICMP_NE:
-                condTrue = (lhsValue != rhsValue);
-                break;
-            case ICmpInst::ICMP_SLT:
-                condTrue = lhsValue.slt(rhsValue);
-                break;
-            case ICmpInst::ICMP_SLE:
-                condTrue = lhsValue.sle(rhsValue);
-                break;
-            case ICmpInst::ICMP_SGT:
-                condTrue = lhsValue.sgt(rhsValue);
-                break;
-            case ICmpInst::ICMP_SGE:
-                condTrue = lhsValue.sge(rhsValue);
-                break;
-            case ICmpInst::ICMP_ULT:
-                condTrue = lhsValue.ult(rhsValue);
-                break;
-            case ICmpInst::ICMP_ULE:
-                condTrue = lhsValue.ule(rhsValue);
-                break;
-            case ICmpInst::ICMP_UGT:
-                condTrue = lhsValue.ugt(rhsValue);
-                break;
-            case ICmpInst::ICMP_UGE:
-                condTrue = lhsValue.uge(rhsValue);
-                break;
-            default:
-                continue;
-            }
+                return false;
 
-            if (block == branch->getSuccessor(0) && !condTrue)
-                return true;
-            if (block == branch->getSuccessor(1) && condTrue)
-                return true;
+            const bool holds =
+                ICmpInst::compare(lhs->getValue(), rhs->getValue(), compare->getPredicate());
+            if (branch->getSuccessor(holds ? 0 : 1) == block)
+                return false;
         }
 
-        return false;
+        return true;
     }
 
 } // namespace ctrace::stack::analysis

src/analysis/StackComputation.cpp

@@ -22,6 +22,7 @@
 #include <llvm/Support/Alignment.h>
 
 #include "analysis/AnalyzerUtils.hpp"
+#include "analysis/FunctionFacts.hpp"
 #include "analysis/IntRanges.hpp"
 #include "analysis/IRValueUtils.hpp"
 #include "analysis/smt/SmtEncoding.hpp"
@@ -90,6 +91,8 @@ namespace ctrace::stack::analysis
                                                     const CallResolver* resolver)
         {
             LocalStackInfo info;
+            // Built on the first non-constant array size only: most functions have none.
+            std::optional<analysis::FunctionFacts> facts;
 
             for (llvm::BasicBlock& BB : F)
             {
@@ -113,7 +116,8 @@ namespace ctrace::stack::analysis
                     {
                         count = CI->getZExtValue();
                     }
-                    else if (auto* C = analysis::tryGetConstFromValue(alloca->getArraySize(), F))
+                    else if (auto* C = analysis::tryGetConstFromValue(
+                                 alloca->getArraySize(), facts ? *facts : facts.emplace(F)))
                     {
                         count = C->getZExtValue();
                     }

test/bound-storage/global-array-overflow.c

@@ -14,8 +14,10 @@ int main(void)
     // ↳ alias path: gbuf
     // ↳ index variable may go up to 10 (array last valid index: 9)
     // ↳ (this is a write access)
-    // ↳ [info] this access appears unreachable at runtime (condition is always false for this branch)
     gbuf[i] = i;
 
     return gbuf[0];
 }
+
+// The access runs after the loop with i == 10, so it is reachable.
+// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch)

test/bound-storage/unreachable-branch-dependent-store.c

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: Apache-2.0
+// i is 0 or 11 depending on c: the condition is not constant, so the access may run.
+void unreachable_branch_dependent_store(int c)
+{
+    int i = 0;
+    char test[10];
+
+    if (c)
+        i = 11;
+    if (i <= 10)
+        test[11] = 'a';
+}
+
+// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch)
+
+// at line 11, column 18
+// [ !!Warn ] potential stack buffer overflow on variable 'test' (size 10)
+// ↳ alias path: test
+// ↳ constant index 11 is out of bounds (0..9)
+// ↳ (this is a write access)

test/bound-storage/unreachable-later-store-is-not-read.c

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: Apache-2.0
+// The condition reads i before i = 11: it holds, so the access is reachable.
+char unreachable_later_store_is_not_read(void)
+{
+    int i = 5;
+    char test[10];
+
+    if (i <= 10)
+        test[11] = 'a';
+    i = 11;
+    return (char)i;
+}
+
+// not contains: [info] this access appears unreachable at runtime (condition is always false for this branch)
+
+// at line 9, column 18
+// [ !!Warn ] potential stack buffer overflow on variable 'test' (size 10)
+// ↳ alias path: test
+// ↳ constant index 11 is out of bounds (0..9)
+// ↳ (this is a write access)