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