r356222 - [analyzer] Support C++17 aggregates with bases without constructors.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 19 10:24:19 PDT 2019
Hi,
I'll try to fix this ASAP!
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
>
More information about the cfe-commits
mailing list