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

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 19 11:00:45 PDT 2019


On Tue, Mar 19, 2019 at 6:24 PM Artem Dergachev <noqnoqneo at gmail.com> wrote:

> Hi,
>
> I'll try to fix this ASAP!
>

Thanks!

> 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?


No, but this particular example comes from C++11 code that just has subtly
different semantics in C++17. I guess, just adding -std=c++17 on existing
code (LLVM, for example ;) could help uncover some of the issues. I
believe, we can also continue providing reports in case CSA fails
assertions on our code.


> 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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190319/6a2f59a4/attachment-0001.html>


More information about the cfe-commits mailing list