NoQ created this revision. NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet, mgorny. Herald added a project: clang.
D44934 <https://reviews.llvm.org/D44934> added modeling for `memset()` that, for the case of setting memory to 0, boils down to default-binding a concrete 0 to the memory region. Surprisingly, in some cases RegionStore fails to load default bindings from a region. That is, it assumes that the region either has a direct binding (i.e., initialized via assignment), or it is uninitialized. In particular, this applies to local variables regions of integer/pointer/float types. It seems that a lot of code (eg., `invalidateRegions()`) carefully works around this problem by adding either a direct binding or a default binding, depending on what `RegionStore` expects. This essentially duplicates `RegionStore`'s already-convoluted logic on this subject on the caller side. Our options are: 1. Duplicate this logic again in `CStringChecker`. 2. Let `RegionStore` automatically do the direct binding for such plain integers. 3. Add support for loading default bindings from variables. Option 1 is a failure to improve checker API in an aspect in which it's already very bad, so i didn't go for it. In options 2 and 3 the difference is entirely within `RegionStore`. I believe that option 2 is slightly superior because it produces a more "normalized" data structure and, additionally, because stores are more rare than loads (unless they are "dead stores"^^), so it should be slightly more performant. But it is also harder to implement, as it requires gathering the logic of when exactly do we need a direct binding in one place, so i went for option 3 in order to address the regression and simplify that logic. I did not yet try to see how other callers of `bindDefault***()` can be simplified. Repository: rC Clang https://reviews.llvm.org/D60742 Files: clang/lib/StaticAnalyzer/Core/RegionStore.cpp clang/test/Analysis/string.c clang/unittests/StaticAnalyzer/CMakeLists.txt clang/unittests/StaticAnalyzer/StoreTest.cpp
Index: clang/unittests/StaticAnalyzer/StoreTest.cpp =================================================================== --- /dev/null +++ clang/unittests/StaticAnalyzer/StoreTest.cpp @@ -0,0 +1,108 @@ +//===- unittests/StaticAnalyzer/RegionStoreTest.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 "Reusables.h" + +#include "clang/Tooling/Tooling.h" +#include "gtest/gtest.h" + +namespace clang { +namespace ento { +namespace { + +// Test that we can put a value into an int-type variable and load it +// back from that variable. Test what happens if default bindings are used. +class VariableBindConsumer : public ExprEngineConsumer { + void performTest(const Decl *D) { + StoreManager &StMgr = Eng.getStoreManager(); + SValBuilder &SVB = Eng.getSValBuilder(); + MemRegionManager &MRMgr = StMgr.getRegionManager(); + const ASTContext &ACtx = Eng.getContext(); + + const auto *VDX0 = findDeclByName<VarDecl>(D, "x0"); + const auto *VDY0 = findDeclByName<VarDecl>(D, "y0"); + const auto *VDZ0 = findDeclByName<VarDecl>(D, "z0"); + const auto *VDX1 = findDeclByName<VarDecl>(D, "x1"); + const auto *VDY1 = findDeclByName<VarDecl>(D, "y1"); + assert(VDX0 && VDY0 && VDZ0 && VDX1 && VDY1); + + const StackFrameContext *SFC = + Eng.getAnalysisDeclContextManager().getStackFrame(D); + + Loc LX0 = loc::MemRegionVal(MRMgr.getVarRegion(VDX0, SFC)); + Loc LY0 = loc::MemRegionVal(MRMgr.getVarRegion(VDY0, SFC)); + Loc LZ0 = loc::MemRegionVal(MRMgr.getVarRegion(VDZ0, SFC)); + Loc LX1 = loc::MemRegionVal(MRMgr.getVarRegion(VDX1, SFC)); + Loc LY1 = loc::MemRegionVal(MRMgr.getVarRegion(VDY1, SFC)); + + Store StInit = StMgr.getInitialStore(SFC).getStore(); + SVal Zero = SVB.makeZeroVal(ACtx.IntTy); + SVal One = SVB.makeIntVal(1, ACtx.IntTy); + SVal NarrowZero = SVB.makeZeroVal(ACtx.CharTy); + + // Bind(Zero) + Store StX0 = + StMgr.Bind(StInit, LX0, Zero).getStore(); + ASSERT_EQ(Zero, StMgr.getBinding(StX0, LX0, ACtx.IntTy)); + + // BindDefaultInitial(Zero) + Store StY0 = + StMgr.BindDefaultInitial(StInit, LY0.getAsRegion(), Zero).getStore(); + ASSERT_EQ(Zero, StMgr.getBinding(StY0, LY0, ACtx.IntTy)); + ASSERT_EQ(Zero, *StMgr.getDefaultBinding(StY0, LY0.getAsRegion())); + + // BindDefaultZero() + Store StZ0 = + StMgr.BindDefaultZero(StInit, LZ0.getAsRegion()).getStore(); + // BindDefaultZero wipes the region with '0 S8b', not with out Zero. + // Direct load, however, does give us back the object of the type + // that we specify for loading. + ASSERT_EQ(Zero, StMgr.getBinding(StZ0, LZ0, ACtx.IntTy)); + ASSERT_EQ(NarrowZero, *StMgr.getDefaultBinding(StZ0, LZ0.getAsRegion())); + + // Bind(One) + Store StX1 = + StMgr.Bind(StInit, LX1, One).getStore(); + ASSERT_EQ(One, StMgr.getBinding(StX1, LX1, ACtx.IntTy)); + + // BindDefaultInitial(One) + Store StY1 = + StMgr.BindDefaultInitial(StInit, LY1.getAsRegion(), One).getStore(); + ASSERT_EQ(One, StMgr.getBinding(StY1, LY1, ACtx.IntTy)); + ASSERT_EQ(One, *StMgr.getDefaultBinding(StY1, LY1.getAsRegion())); + } + +public: + VariableBindConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {} + ~VariableBindConsumer() override {} + + bool HandleTopLevelDecl(DeclGroupRef DG) override { + for (const auto *D : DG) + performTest(D); + return true; + } +}; + +class VariableBindAction : public ASTFrontendAction { +public: + VariableBindAction() {} + std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler, + StringRef File) override { + return llvm::make_unique<VariableBindConsumer>(Compiler); + } +}; + +// Test that marking s.x as live would also make s live. +TEST(Store, VariableBind) { + EXPECT_TRUE(tooling::runToolOnCode( + new VariableBindAction, "void foo() { int x0, y0, z0, x1, y1; }")); +} + +} // namespace +} // namespace ento +} // namespace clang Index: clang/unittests/StaticAnalyzer/CMakeLists.txt =================================================================== --- clang/unittests/StaticAnalyzer/CMakeLists.txt +++ clang/unittests/StaticAnalyzer/CMakeLists.txt @@ -4,6 +4,7 @@ add_clang_unittest(StaticAnalysisTests AnalyzerOptionsTest.cpp + StoreTest.cpp RegisterCustomCheckersTest.cpp SymbolReaperTest.cpp ) Index: clang/test/Analysis/string.c =================================================================== --- clang/test/Analysis/string.c +++ clang/test/Analysis/string.c @@ -1554,3 +1554,9 @@ // This should be true. clang_analyzer_eval(x == 0x101); // expected-warning{{UNKNOWN}} } + +void memset29_plain_int_zero() { + short x; + memset(&x, 0, sizeof(short)); + clang_analyzer_eval(x == 0); // expected-warning{{TRUE}} +} Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/RegionStore.cpp +++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -1927,7 +1927,10 @@ const VarRegion *R) { // Check if the region has a binding. - if (const Optional<SVal> &V = B.getDirectBinding(R)) + if (Optional<SVal> V = B.getDirectBinding(R)) + return *V; + + if (Optional<SVal> V = B.getDefaultBinding(R)) return *V; // Lazily derive a value for the VarRegion.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
