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

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 19 11:10:29 PDT 2019


On Tue, 19 Mar 2019 at 10:24, Artem Dergachev via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
>
> Hi,
>
> I'll try to fix this ASAP!

It sounds like there might be a missing check for
InitListExpr::isTransparent somewhere. (A transparent InitListExpr
should be treated as equivalent to its one and only subexpression.)
Either that, or the static analyzer isn't aware that an object of
class type can be initialized directly from a function call, not via a
constructor.

> It sounds as if i don't have nearly enough C++17 codebases for testing,
> are there any obvious open-source projects that you could recommend? I'd
> love to try to reincarnate
> http://green.lab.llvm.org/green/view/Experimental/job/StaticAnalyzerBenchmarks/
> with those.
>
> On 3/19/19 9:37 AM, Alexander Kornienko wrote:
> > Filed this as https://bugs.llvm.org/show_bug.cgi?id=41142. Any hope
> > for a prompt fix here?
> >
> > On Tue, Mar 19, 2019 at 5:34 PM Alexander Kornienko <alexfh at google.com
> > <mailto:alexfh at google.com>> wrote:
> >
> >     A reduced test case:
> >     $ cat test-RegionStoreManager__bindStruct.cc
> >     struct a {};
> >     class b : a {};
> >     b c() { b d{c()}; }
> >     $ ./clang-tidy -checks="-*,clang-analyzer*"
> >     test-RegionStoreManager__bindStruct.cc -- -std=c++17
> >     assert.h assertion failed at
> >     tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2362 in
> >     (anonymous namespace)::RegionBindingsRef (anonym
> >     ous
> >     namespace)::RegionStoreManager::bindStruct(RegionBindingsConstRef,
> >     const clang::ento::TypedValueRegion *, clang::ento::SVal):
> >     CRD->isAggregate() && "Non-aggregates are constructed with a
> >     constructor!"
> >         @     0x559908170326  __assert_fail
> >         @     0x5599068d4854  (anonymous
> >     namespace)::RegionStoreManager::bindStruct()
> >         @     0x5599068c93c8  (anonymous
> >     namespace)::RegionStoreManager::Bind()
> >         @     0x5599068b409f clang::ento::ProgramState::bindLoc()
> >         @     0x559906865935
> >     clang::ento::ExprEngine::processPointerEscapedOnBind()
> >         @     0x55990685d4b3 clang::ento::ExprEngine::evalBind()
> >         @     0x559906872a43 clang::ento::ExprEngine::VisitDeclStmt()
> >         @     0x55990685c16f clang::ento::ExprEngine::Visit()
> >         @     0x559906858b1f clang::ento::ExprEngine::ProcessStmt()
> >         @     0x559906858808 clang::ento::ExprEngine::processCFGElement()
> >         @     0x55990684cb65 clang::ento::CoreEngine::HandlePostStmt()
> >         @     0x55990684bf5c clang::ento::CoreEngine::ExecuteWorkList()
> >         @     0x5599065b635b  (anonymous
> >     namespace)::AnalysisConsumer::HandleCode()
> >         @     0x5599065a0135  (anonymous
> >     namespace)::AnalysisConsumer::HandleTranslationUnit()
> >         @     0x559906bb7cbc
> >     clang::MultiplexConsumer::HandleTranslationUnit()
> >         @     0x559906d226d4  clang::ParseAST()
> >         @     0x559906b98a83 clang::FrontendAction::Execute()
> >         @     0x559906b31cd1 clang::CompilerInstance::ExecuteAction()
> >         @     0x559906a9cf61
> >     clang::tooling::FrontendActionFactory::runInvocation()
> >         @     0x55990620cc07
> >     clang::tidy::runClangTidy()::ActionFactory::runInvocation()
> >         @     0x559906a9ccca
> >     clang::tooling::ToolInvocation::runInvocation()
> >         @     0x559906a9c646 clang::tooling::ToolInvocation::run()
> >         @     0x559906a9ef22 clang::tooling::ClangTool::run()
> >         @     0x559906207ecf  clang::tidy::runClangTidy()
> >         @     0x559902d47c45  main
> >
> >     On Tue, Mar 19, 2019 at 1:14 AM Alexander Kornienko
> >     <alexfh at google.com <mailto:alexfh at google.com>> wrote:
> >
> >         On Fri, Mar 15, 2019 at 1:21 AM Artem Dergachev via
> >         cfe-commits <cfe-commits at lists.llvm.org
> >         <mailto: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 <mailto:cfe-commits at lists.llvm.org>
> >             https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


More information about the cfe-commits mailing list