<div dir="ltr"><div dir="ltr">On Tue, Mar 19, 2019 at 6:24 PM Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com">noqnoqneo@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
I'll try to fix this ASAP!<br></blockquote><div><br></div><div>Thanks! </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It sounds as if i don't have nearly enough C++17 codebases for testing, <br>
are there any obvious open-source projects that you could recommend?</blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I'd <br>
love to try to reincarnate <br>
<a href="http://green.lab.llvm.org/green/view/Experimental/job/StaticAnalyzerBenchmarks/" rel="noreferrer" target="_blank">http://green.lab.llvm.org/green/view/Experimental/job/StaticAnalyzerBenchmarks/</a> <br>
with those.<br>
<br>
On 3/19/19 9:37 AM, Alexander Kornienko wrote:<br>
> Filed this as <a href="https://bugs.llvm.org/show_bug.cgi?id=41142" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=41142</a>. Any hope <br>
> for a prompt fix here?<br>
><br>
> On Tue, Mar 19, 2019 at 5:34 PM Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a> <br>
> <mailto:<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>>> wrote:<br>
><br>
> A reduced test case:<br>
> $ cat test-RegionStoreManager__bindStruct.cc<br>
> struct a {};<br>
> class b : a {};<br>
> b c() { b d{c()}; }<br>
> $ ./clang-tidy -checks="-*,clang-analyzer*"<br>
> test-RegionStoreManager__bindStruct.cc -- -std=c++17<br>
> assert.h assertion failed at<br>
> tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2362 in<br>
> (anonymous namespace)::RegionBindingsRef (anonym<br>
> ous<br>
> namespace)::RegionStoreManager::bindStruct(RegionBindingsConstRef,<br>
> const clang::ento::TypedValueRegion *, clang::ento::SVal):<br>
> CRD->isAggregate() && "Non-aggregates are constructed with a<br>
> constructor!"<br>
> @ 0x559908170326 __assert_fail<br>
> @ 0x5599068d4854 (anonymous<br>
> namespace)::RegionStoreManager::bindStruct()<br>
> @ 0x5599068c93c8 (anonymous<br>
> namespace)::RegionStoreManager::Bind()<br>
> @ 0x5599068b409f clang::ento::ProgramState::bindLoc()<br>
> @ 0x559906865935<br>
> clang::ento::ExprEngine::processPointerEscapedOnBind()<br>
> @ 0x55990685d4b3 clang::ento::ExprEngine::evalBind()<br>
> @ 0x559906872a43 clang::ento::ExprEngine::VisitDeclStmt()<br>
> @ 0x55990685c16f clang::ento::ExprEngine::Visit()<br>
> @ 0x559906858b1f clang::ento::ExprEngine::ProcessStmt()<br>
> @ 0x559906858808 clang::ento::ExprEngine::processCFGElement()<br>
> @ 0x55990684cb65 clang::ento::CoreEngine::HandlePostStmt()<br>
> @ 0x55990684bf5c clang::ento::CoreEngine::ExecuteWorkList()<br>
> @ 0x5599065b635b (anonymous<br>
> namespace)::AnalysisConsumer::HandleCode()<br>
> @ 0x5599065a0135 (anonymous<br>
> namespace)::AnalysisConsumer::HandleTranslationUnit()<br>
> @ 0x559906bb7cbc<br>
> clang::MultiplexConsumer::HandleTranslationUnit()<br>
> @ 0x559906d226d4 clang::ParseAST()<br>
> @ 0x559906b98a83 clang::FrontendAction::Execute()<br>
> @ 0x559906b31cd1 clang::CompilerInstance::ExecuteAction()<br>
> @ 0x559906a9cf61<br>
> clang::tooling::FrontendActionFactory::runInvocation()<br>
> @ 0x55990620cc07<br>
> clang::tidy::runClangTidy()::ActionFactory::runInvocation()<br>
> @ 0x559906a9ccca<br>
> clang::tooling::ToolInvocation::runInvocation()<br>
> @ 0x559906a9c646 clang::tooling::ToolInvocation::run()<br>
> @ 0x559906a9ef22 clang::tooling::ClangTool::run()<br>
> @ 0x559906207ecf clang::tidy::runClangTidy()<br>
> @ 0x559902d47c45 main<br>
><br>
> On Tue, Mar 19, 2019 at 1:14 AM Alexander Kornienko<br>
> <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a> <mailto:<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>>> wrote:<br>
><br>
> On Fri, Mar 15, 2019 at 1:21 AM Artem Dergachev via<br>
> cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <mailto:<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>>> wrote:<br>
><br>
> Author: dergachev<br>
> Date: Thu Mar 14 17:22:59 2019<br>
> New Revision: 356222<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=356222&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=356222&view=rev</a><br>
> Log:<br>
> [analyzer] Support C++17 aggregates with bases without<br>
> constructors.<br>
><br>
> RegionStore now knows how to bind a nonloc::CompoundVal<br>
> that represents the<br>
> value of an aggregate initializer when it has its initial<br>
> segment of sub-values<br>
> correspond to base classes.<br>
><br>
> Additionally, fixes the crash from pr40022.<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D59054" rel="noreferrer" target="_blank">https://reviews.llvm.org/D59054</a><br>
><br>
> Modified:<br>
> cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp<br>
> cfe/trunk/test/Analysis/array-struct-region.cpp<br>
><br>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=356222&r1=356221&r2=356222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=356222&r1=356221&r2=356222&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp<br>
> (original)<br>
> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Thu<br>
> Mar 14 17:22:59 2019<br>
> @@ -2334,12 +2334,57 @@ RegionBindingsRef<br>
> RegionStoreManager::bi<br>
> if (V.isUnknown() || !V.getAs<nonloc::CompoundVal>())<br>
> return bindAggregate(B, R, UnknownVal());<br>
><br>
> + // The raw CompoundVal is essentially a symbolic<br>
> InitListExpr: an (immutable)<br>
> + // list of other values. It appears pretty much only<br>
> when there's an actual<br>
> + // initializer list expression in the program, and the<br>
> analyzer tries to<br>
> + // unwrap it as soon as possible.<br>
> + // This code is where such unwrap happens: when the<br>
> compound value is put into<br>
> + // the object that it was supposed to initialize (it's<br>
> an *initializer* list,<br>
> + // after all), instead of binding the whole value to<br>
> the whole object, we bind<br>
> + // sub-values to sub-objects. Sub-values may themselves<br>
> be compound values,<br>
> + // and in this case the procedure becomes recursive.<br>
> + // FIXME: The annoying part about compound values is<br>
> that they don't carry<br>
> + // any sort of information about which value<br>
> corresponds to which sub-object.<br>
> + // It's simply a list of values in the middle of<br>
> nowhere; we expect to match<br>
> + // them to sub-objects, essentially, "by index": first<br>
> value binds to<br>
> + // the first field, second value binds to the second<br>
> field, etc.<br>
> + // It would have been much safer to organize non-lazy<br>
> compound values as<br>
> + // a mapping from fields/bases to values.<br>
> const nonloc::CompoundVal& CV =<br>
> V.castAs<nonloc::CompoundVal>();<br>
> nonloc::CompoundVal::iterator VI = CV.begin(), VE =<br>
> CV.end();<br>
><br>
> - RecordDecl::field_iterator FI, FE;<br>
> RegionBindingsRef NewB(B);<br>
><br>
> + // In C++17 aggregates may have base classes, handle<br>
> those as well.<br>
> + // They appear before fields in the initializer list /<br>
> compound value.<br>
> + if (const auto *CRD = dyn_cast<CXXRecordDecl>(RD)) {<br>
> + assert(CRD->isAggregate() &&<br>
> + "Non-aggregates are constructed with a<br>
> constructor!");<br>
><br>
><br>
> Now we see this assertion being triggered on a substantial<br>
> number of files in our codebase:<br>
> llvm/tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2362<br>
> in (anonymous namespace)::RegionBindingsRef (anonymous<br>
> namespace)::RegionStoreManager::bindStruct(RegionBindingsConstRef,<br>
> const clang::ento::TypedValueRegion *, clang::ento::SVal):<br>
> CRD->isAggregate() && "Non-aggregates are constructed with a<br>
> constructor!"<br>
> Stack trace:<br>
> @ 0x5596b00a84e6 96 __assert_fail<br>
> @ 0x5596b6e6ea14 304 (anonymous<br>
> namespace)::RegionStoreManager::bindStruct()<br>
> @ 0x5596afb30228 128 (anonymous<br>
> namespace)::RegionStoreManager::Bind()<br>
> @ 0x5596af822abf 128<br>
> clang::ento::ProgramState::bindLoc()<br>
> @ 0x5596b6e2b657 112<br>
> clang::ento::ExprEngine::processPointerEscapedOnBind()<br>
> @ 0x5596b907f65f 512<br>
> clang::ento::ExprEngine::evalBind()<br>
> @ 0x5596b6e30ea7 560<br>
> clang::ento::ExprEngine::VisitDeclStmt()<br>
> @ 0x5596b8da1fe2 752<br>
> clang::ento::ExprEngine::Visit()<br>
> @ 0x5596b8d9cb2f 400<br>
> clang::ento::ExprEngine::ProcessStmt()<br>
> @ 0x5596af78431e 112<br>
> clang::ento::ExprEngine::processCFGElement()<br>
> @ 0x5596af6578e0 48<br>
> clang::ento::CoreEngine::HandlePostStmt()<br>
> @ 0x5596b03b151b 272<br>
> clang::ento::CoreEngine::ExecuteWorkList()<br>
> @ 0x5596af6f8efe 1248 (anonymous<br>
> namespace)::AnalysisConsumer::HandleCode()<br>
> @ 0x5596b6c54a77 448 (anonymous<br>
> namespace)::AnalysisConsumer::HandleTranslationUnit()<br>
> @ 0x5596b706f08c 48<br>
> clang::MultiplexConsumer::HandleTranslationUnit()<br>
> @ 0x5596aff72e24 144 clang::ParseAST()<br>
> @ 0x5596b7053bc3 48<br>
> clang::FrontendAction::Execute()<br>
> @ 0x5596b7002ba0 160<br>
> clang::CompilerInstance::ExecuteAction()<br>
> @ 0x5596b6f91a61 464<br>
> clang::tooling::FrontendActionFactory::runInvocation()<br>
> @ 0x5596b6f917ea 80<br>
> clang::tooling::ToolInvocation::runInvocation()<br>
> @ 0x5596afe70af6 2352<br>
> clang::tooling::ToolInvocation::run()<br>
><br>
> Trying to get an isolated test case.<br>
><br>
> +<br>
> + for (const auto &B : CRD->bases()) {<br>
> + // (Multiple inheritance is fine though.)<br>
> + assert(!B.isVirtual() && "Aggregates cannot have<br>
> virtual base classes!");<br>
> +<br>
> + if (VI == VE)<br>
> + break;<br>
> +<br>
> + QualType BTy = B.getType();<br>
> + assert(BTy->isStructureOrClassType() && "Base<br>
> classes must be classes!");<br>
> +<br>
> + const CXXRecordDecl *BRD = BTy->getAsCXXRecordDecl();<br>
> + assert(BRD && "Base classes must be C++ classes!");<br>
> +<br>
> + const CXXBaseObjectRegion *BR =<br>
> + MRMgr.getCXXBaseObjectRegion(BRD, R,<br>
> /*IsVirtual=*/false);<br>
> +<br>
> + NewB = bindStruct(NewB, BR, *VI);<br>
> +<br>
> + ++VI;<br>
> + }<br>
> + }<br>
> +<br>
> + RecordDecl::field_iterator FI, FE;<br>
> +<br>
> for (FI = RD->field_begin(), FE = RD->field_end(); FI<br>
> != FE; ++FI) {<br>
><br>
> if (VI == VE)<br>
><br>
> Modified: cfe/trunk/test/Analysis/array-struct-region.cpp<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/array-struct-region.cpp?rev=356222&r1=356221&r2=356222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/array-struct-region.cpp?rev=356222&r1=356221&r2=356222&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Analysis/array-struct-region.cpp (original)<br>
> +++ cfe/trunk/test/Analysis/array-struct-region.cpp Thu<br>
> Mar 14 17:22:59 2019<br>
> @@ -1,7 +1,21 @@<br>
> -// RUN: %clang_analyze_cc1<br>
> -analyzer-checker=core,alpha.core,debug.ExprInspection<br>
> -verify -x c %s<br>
> -// RUN: %clang_analyze_cc1<br>
> -analyzer-checker=core,alpha.core,debug.ExprInspection<br>
> -verify -x c++ -analyzer-config c++-inlining=constructors %s<br>
> -// RUN: %clang_analyze_cc1<br>
> -analyzer-checker=core,alpha.core,debug.ExprInspection<br>
> -DINLINE -verify -x c %s<br>
> -// RUN: %clang_analyze_cc1<br>
> -analyzer-checker=core,alpha.core,debug.ExprInspection<br>
> -DINLINE -verify -x c++ -analyzer-config<br>
> c++-inlining=constructors %s<br>
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\<br>
> +// RUN: -analyzer-checker=debug.ExprInspection -verify\<br>
> +// RUN: -x c %s<br>
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\<br>
> +// RUN: -analyzer-checker=debug.ExprInspection -verify\<br>
> +// RUN: -x c++ -std=c++14 %s<br>
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\<br>
> +// RUN: -analyzer-checker=debug.ExprInspection -verify\<br>
> +// RUN: -x c++ -std=c++17 %s<br>
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\<br>
> +// RUN: -analyzer-checker=debug.ExprInspection -verify\<br>
> +// RUN: -DINLINE -x c %s<br>
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\<br>
> +// RUN: -analyzer-checker=debug.ExprInspection -verify\<br>
> +// RUN: -DINLINE -x c++ -std=c++14 %s<br>
> +// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\<br>
> +// RUN: -analyzer-checker=debug.ExprInspection -verify\<br>
> +// RUN: -DINLINE -x c++ -std=c++17 %s<br>
><br>
> void clang_analyzer_eval(int);<br>
><br>
> @@ -196,4 +210,49 @@ namespace EmptyClass {<br>
> }<br>
> }<br>
><br>
> +#if __cplusplus >= 201703L<br>
> +namespace aggregate_inheritance_cxx17 {<br>
> +struct A {<br>
> + int x;<br>
> +};<br>
> +<br>
> +struct B {<br>
> + int y;<br>
> +};<br>
> +<br>
> +struct C: B {<br>
> + int z;<br>
> +};<br>
> +<br>
> +struct D: A, C {<br>
> + int w;<br>
> +};<br>
> +<br>
> +void foo() {<br>
> + D d{1, 2, 3, 4};<br>
> + clang_analyzer_eval(d.x == 1); // expected-warning{{TRUE}}<br>
> + clang_analyzer_eval(d.y == 2); // expected-warning{{TRUE}}<br>
> + clang_analyzer_eval(d.z == 3); // expected-warning{{TRUE}}<br>
> + clang_analyzer_eval(d.w == 4); // expected-warning{{TRUE}}<br>
> +}<br>
> +} // namespace aggregate_inheritance_cxx17<br>
> +#endif<br>
> +<br>
> +namespace flex_array_inheritance_cxx17 {<br>
> +struct A {<br>
> + int flexible_array[];<br>
> +};<br>
> +<br>
> +struct B {<br>
> + long cookie;<br>
> +};<br>
> +<br>
> +struct C : B {<br>
> + A a;<br>
> +};<br>
> +<br>
> +void foo() {<br>
> + C c{}; // no-crash<br>
> +}<br>
> +} // namespace flex_array_inheritance_cxx17<br>
> #endif<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a> <mailto:<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
><br>
<br>
</blockquote></div></div>