r356222 - [analyzer] Support C++17 aggregates with bases without constructors.

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 18 17:14:15 PDT 2019


On Fri, Mar 15, 2019 at 1:21 AM Artem Dergachev via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Author: dergachev
> Date: Thu Mar 14 17:22:59 2019
> New Revision: 356222
>
> URL: http://llvm.org/viewvc/llvm-project?rev=356222&view=rev
> Log:
> [analyzer] Support C++17 aggregates with bases without constructors.
>
> RegionStore now knows how to bind a nonloc::CompoundVal that represents the
> value of an aggregate initializer when it has its initial segment of
> sub-values
> correspond to base classes.
>
> Additionally, fixes the crash from pr40022.
>
> Differential Revision: https://reviews.llvm.org/D59054
>
> Modified:
>     cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>     cfe/trunk/test/Analysis/array-struct-region.cpp
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=356222&r1=356221&r2=356222&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Thu Mar 14 17:22:59
> 2019
> @@ -2334,12 +2334,57 @@ RegionBindingsRef RegionStoreManager::bi
>    if (V.isUnknown() || !V.getAs<nonloc::CompoundVal>())
>      return bindAggregate(B, R, UnknownVal());
>
> +  // The raw CompoundVal is essentially a symbolic InitListExpr: an
> (immutable)
> +  // list of other values. It appears pretty much only when there's an
> actual
> +  // initializer list expression in the program, and the analyzer tries to
> +  // unwrap it as soon as possible.
> +  // This code is where such unwrap happens: when the compound value is
> put into
> +  // the object that it was supposed to initialize (it's an *initializer*
> list,
> +  // after all), instead of binding the whole value to the whole object,
> we bind
> +  // sub-values to sub-objects. Sub-values may themselves be compound
> values,
> +  // and in this case the procedure becomes recursive.
> +  // FIXME: The annoying part about compound values is that they don't
> carry
> +  // any sort of information about which value corresponds to which
> sub-object.
> +  // It's simply a list of values in the middle of nowhere; we expect to
> match
> +  // them to sub-objects, essentially, "by index": first value binds to
> +  // the first field, second value binds to the second field, etc.
> +  // It would have been much safer to organize non-lazy compound values as
> +  // a mapping from fields/bases to values.
>    const nonloc::CompoundVal& CV = V.castAs<nonloc::CompoundVal>();
>    nonloc::CompoundVal::iterator VI = CV.begin(), VE = CV.end();
>
> -  RecordDecl::field_iterator FI, FE;
>    RegionBindingsRef NewB(B);
>
> +  // In C++17 aggregates may have base classes, handle those as well.
> +  // They appear before fields in the initializer list / compound value.
> +  if (const auto *CRD = dyn_cast<CXXRecordDecl>(RD)) {
> +    assert(CRD->isAggregate() &&
> +           "Non-aggregates are constructed with a constructor!");


Now we see this assertion being triggered on a substantial number of files
in our codebase:
llvm/tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2362 in (anonymous
namespace)::RegionBindingsRef (anonymous
namespace)::RegionStoreManager::bindStruct(RegionBindingsConstRef, const
clang::ento::TypedValueRegion *, clang::ento::SVal): CRD->isAggregate() &&
"Non-aggregates are constructed with a constructor!"
Stack trace:
    @     0x5596b00a84e6         96  __assert_fail
    @     0x5596b6e6ea14        304  (anonymous
namespace)::RegionStoreManager::bindStruct()
    @     0x5596afb30228        128  (anonymous
namespace)::RegionStoreManager::Bind()
    @     0x5596af822abf        128  clang::ento::ProgramState::bindLoc()
    @     0x5596b6e2b657        112
clang::ento::ExprEngine::processPointerEscapedOnBind()
    @     0x5596b907f65f        512  clang::ento::ExprEngine::evalBind()
    @     0x5596b6e30ea7        560
clang::ento::ExprEngine::VisitDeclStmt()
    @     0x5596b8da1fe2        752  clang::ento::ExprEngine::Visit()
    @     0x5596b8d9cb2f        400  clang::ento::ExprEngine::ProcessStmt()
    @     0x5596af78431e        112
clang::ento::ExprEngine::processCFGElement()
    @     0x5596af6578e0         48
clang::ento::CoreEngine::HandlePostStmt()
    @     0x5596b03b151b        272
clang::ento::CoreEngine::ExecuteWorkList()
    @     0x5596af6f8efe       1248  (anonymous
namespace)::AnalysisConsumer::HandleCode()
    @     0x5596b6c54a77        448  (anonymous
namespace)::AnalysisConsumer::HandleTranslationUnit()
    @     0x5596b706f08c         48
clang::MultiplexConsumer::HandleTranslationUnit()
    @     0x5596aff72e24        144  clang::ParseAST()
    @     0x5596b7053bc3         48  clang::FrontendAction::Execute()
    @     0x5596b7002ba0        160
clang::CompilerInstance::ExecuteAction()
    @     0x5596b6f91a61        464
clang::tooling::FrontendActionFactory::runInvocation()
    @     0x5596b6f917ea         80
clang::tooling::ToolInvocation::runInvocation()
    @     0x5596afe70af6       2352  clang::tooling::ToolInvocation::run()

Trying to get an isolated test case.



+
> +    for (const auto &B : CRD->bases()) {
> +      // (Multiple inheritance is fine though.)
> +      assert(!B.isVirtual() && "Aggregates cannot have virtual base
> classes!");
> +
> +      if (VI == VE)
> +        break;
> +
> +      QualType BTy = B.getType();
> +      assert(BTy->isStructureOrClassType() && "Base classes must be
> classes!");
> +
> +      const CXXRecordDecl *BRD = BTy->getAsCXXRecordDecl();
> +      assert(BRD && "Base classes must be C++ classes!");
> +
> +      const CXXBaseObjectRegion *BR =
> +          MRMgr.getCXXBaseObjectRegion(BRD, R, /*IsVirtual=*/false);
> +
> +      NewB = bindStruct(NewB, BR, *VI);
> +
> +      ++VI;
> +    }
> +  }
> +
> +  RecordDecl::field_iterator FI, FE;
> +
>    for (FI = RD->field_begin(), FE = RD->field_end(); FI != FE; ++FI) {
>
>      if (VI == VE)
>
> Modified: cfe/trunk/test/Analysis/array-struct-region.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/array-struct-region.cpp?rev=356222&r1=356221&r2=356222&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Analysis/array-struct-region.cpp (original)
> +++ cfe/trunk/test/Analysis/array-struct-region.cpp Thu Mar 14 17:22:59
> 2019
> @@ -1,7 +1,21 @@
> -// RUN: %clang_analyze_cc1
> -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c %s
> -// RUN: %clang_analyze_cc1
> -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -x c++
> -analyzer-config c++-inlining=constructors %s
> -// RUN: %clang_analyze_cc1
> -analyzer-checker=core,alpha.core,debug.ExprInspection -DINLINE -verify -x
> c %s
> -// RUN: %clang_analyze_cc1
> -analyzer-checker=core,alpha.core,debug.ExprInspection -DINLINE -verify -x
> c++ -analyzer-config c++-inlining=constructors %s
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
> +// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
> +// RUN:                    -x c %s
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
> +// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
> +// RUN:                    -x c++ -std=c++14 %s
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
> +// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
> +// RUN:                    -x c++ -std=c++17 %s
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
> +// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
> +// RUN:                    -DINLINE -x c %s
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
> +// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
> +// RUN:                    -DINLINE -x c++ -std=c++14 %s
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
> +// RUN:                    -analyzer-checker=debug.ExprInspection -verify\
> +// RUN:                    -DINLINE -x c++ -std=c++17 %s
>
>  void clang_analyzer_eval(int);
>
> @@ -196,4 +210,49 @@ namespace EmptyClass {
>    }
>  }
>
> +#if __cplusplus >= 201703L
> +namespace aggregate_inheritance_cxx17 {
> +struct A {
> +  int x;
> +};
> +
> +struct B {
> +  int y;
> +};
> +
> +struct C: B {
> +  int z;
> +};
> +
> +struct D: A, C {
> +  int w;
> +};
> +
> +void foo() {
> +  D d{1, 2, 3, 4};
> +  clang_analyzer_eval(d.x == 1); // expected-warning{{TRUE}}
> +  clang_analyzer_eval(d.y == 2); // expected-warning{{TRUE}}
> +  clang_analyzer_eval(d.z == 3); // expected-warning{{TRUE}}
> +  clang_analyzer_eval(d.w == 4); // expected-warning{{TRUE}}
> +}
> +} // namespace aggregate_inheritance_cxx17
> +#endif
> +
> +namespace flex_array_inheritance_cxx17 {
> +struct A {
> +  int flexible_array[];
> +};
> +
> +struct B {
> +  long cookie;
> +};
> +
> +struct C : B {
> +  A a;
> +};
> +
> +void foo() {
> +  C c{}; // no-crash
> +}
> +} // namespace flex_array_inheritance_cxx17
>  #endif
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190319/8d2e818c/attachment-0001.html>


More information about the cfe-commits mailing list