<div dir="ltr">Thank you, Justin.<div class="gmail_extra"><br clear="all"><div><div class="gmail_signature">2015-12-03 5:37 GMT+06:00 Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span>:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Serge Pavlov <<a href="mailto:sepavloff@gmail.com">sepavloff@gmail.com</a>> writes:<br>
> sepavloff created this revision.<br>
> sepavloff added a reviewer: bogner.<br>
> sepavloff added subscribers: cfe-commits, silvas.<br>
><br>
> Constructors and destructors may be represented by several functions<br>
> in IR. Only the base structors correspond to source code, others<br>
> are small pieces of code and eventually call the base variant. In<br>
> this case instrumentation of non-base structors has little sense,<br>
> this fix remove it. Now profile data of a declaration correspond to<br>
> exactly one function in IR, it agrees with current logic of profile<br>
> data loading.<br>
><br>
> This change fixes PR24996.<br>
<br>
</span>This looks like the right thing to do. A couple of comments on the patch<br>
below.<br>
<span class=""><br>
><br>
> <a href="http://reviews.llvm.org/D15158" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15158</a><br>
><br>
> Files:<br>
> lib/CodeGen/CGBlocks.cpp<br>
> lib/CodeGen/CGObjC.cpp<br>
> lib/CodeGen/CGStmt.cpp<br>
> lib/CodeGen/CGStmtOpenMP.cpp<br>
> lib/CodeGen/CodeGenFunction.cpp<br>
> lib/CodeGen/CodeGenPGO.cpp<br>
> lib/CodeGen/CodeGenPGO.h<br>
> test/Profile/cxx-structors.cpp<br>
> test/Profile/cxx-virtual-destructor-calls.cpp<br>
><br>
</span>> Index: test/Profile/cxx-virtual-destructor-calls.cpp<br>
> ===================================================================<br>
> --- test/Profile/cxx-virtual-destructor-calls.cpp<br>
> +++ test/Profile/cxx-virtual-destructor-calls.cpp<br>
> @@ -13,18 +13,25 @@<br>
> virtual ~B();<br>
> };<br>
><br>
> -// Complete dtor<br>
> -// CHECK: @__llvm_profile_name__ZN1BD1Ev = private constant [9 x i8] c"_ZN1BD1Ev"<br>
> +// Base dtor<br>
> +// CHECK: @__llvm_profile_name__ZN1BD2Ev = private constant [9 x i8] c"_ZN1BD2Ev"<br>
><br>
> -// Deleting dtor<br>
> -// CHECK: @__llvm_profile_name__ZN1BD0Ev = private constant [9 x i8] c"_ZN1BD0Ev"<br>
> +// Complete dtor must not be instrumented<br>
> +// @__llvm_profile_name__ZN1BD1Ev = private constant [9 x i8] c"_ZN1BD1Ev"<br>
<br>
I guess these should be CHECK-NOT instead of just comments.<br></blockquote><div><br></div><div>Exactly. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
> -// Complete dtor counters and profile data<br>
> -// CHECK: @__llvm_profile_counters__ZN1BD1Ev = private global [1 x i64] zeroinitializer<br>
> -// CHECK: @__llvm_profile_data__ZN1BD1Ev =<br>
> +// Deleting dtor must not be instrumented<br>
> +// @__llvm_profile_name__ZN1BD0Ev = private constant [9 x i8] c"_ZN1BD0Ev"<br>
><br>
> -// Deleting dtor counters and profile data<br>
> -// CHECK: @__llvm_profile_counters__ZN1BD0Ev = private global [1 x i64] zeroinitializer<br>
> -// CHECK: @__llvm_profile_data__ZN1BD0Ev =<br>
> +// Base dtor counters and profile data<br>
> +// CHECK: @__llvm_profile_counters__ZN1BD2Ev = private global [1 x i64] zeroinitializer<br>
> +// CHECK: @__llvm_profile_data__ZN1BD2Ev =<br>
> +<br>
> +// Complete dtor counters and profile data must absent<br>
> +// @__llvm_profile_counters__ZN1BD1Ev = private global [1 x i64] zeroinitializer<br>
> +// @__llvm_profile_data__ZN1BD1Ev =<br>
> +<br>
> +// Deleting dtor counters and profile data must absent<br>
> +// @__llvm_profile_counters__ZN1BD0Ev = private global [1 x i64] zeroinitializer<br>
> +// @__llvm_profile_data__ZN1BD0Ev =<br>
><br>
> B::~B() { }<br>
> Index: test/Profile/cxx-structors.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/Profile/cxx-structors.cpp<br>
> @@ -0,0 +1,32 @@<br>
> +// Tests for instrumentation of C++ constructors and destructors.<br>
> +//<br>
> +// RUN: %clang_cc1 -triple x86_64-apple-macosx10.11.0 -x c++ %s -o - -emit-llvm -fprofile-instr-generate | FileCheck %s<br>
> +<br>
> +struct Foo {<br>
> + Foo() {}<br>
> + Foo(int) {}<br>
> + ~Foo() {}<br>
> +};<br>
> +<br>
> +struct Bar : public Foo {<br>
> + Bar() {}<br>
> + Bar(int x) : Foo(x) {}<br>
> + ~Bar();<br>
> +};<br>
> +<br>
> +Foo foo;<br>
> +Foo foo2(1);<br>
> +Bar bar;<br>
> +<br>
> +// Profile data for complete constructors and destructors must absent.<br>
> +<br>
> +// CHECK-NOT: @__llvm_profile_name__ZN3FooC1Ev<br>
> +// CHECK-NOT: @__llvm_profile_name__ZN3FooC1Ei<br>
> +// CHECK-NOT: @__llvm_profile_name__ZN3FooD1Ev<br>
> +// CHECK-NOT: @__llvm_profile_name__ZN3BarC1Ev<br>
> +// CHECK-NOT: @__llvm_profile_name__ZN3BarD1Ev<br>
> +// CHECK-NOT: @__llvm_profile_counters__ZN3FooD1Ev<br>
> +// CHECK-NOT: @__llvm_profile_data__ZN3FooD1Ev<br>
> +<br>
> +int main() {<br>
> +}<br>
> Index: lib/CodeGen/CodeGenPGO.h<br>
> ===================================================================<br>
> --- lib/CodeGen/CodeGenPGO.h<br>
> +++ lib/CodeGen/CodeGenPGO.h<br>
> @@ -78,13 +78,11 @@<br>
> setCurrentRegionCount(*Count);<br>
> }<br>
><br>
> - /// Check if we need to emit coverage mapping for a given declaration<br>
> - void checkGlobalDecl(GlobalDecl GD);<br>
> /// Assign counters to regions and configure them for PGO of a given<br>
> /// function. Does nothing if instrumentation is not enabled and either<br>
> /// generates global variables or associates PGO data with each of the<br>
> /// counters depending on whether we are generating or using instrumentation.<br>
> - void assignRegionCounters(const Decl *D, llvm::Function *Fn);<br>
> + void assignRegionCounters(GlobalDecl GD, llvm::Function *Fn);<br>
> /// Emit a coverage mapping range with a counter zero<br>
> /// for an unused declaration.<br>
> void emitEmptyCounterMapping(const Decl *D, StringRef FuncName,<br>
> Index: lib/CodeGen/CodeGenPGO.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CodeGenPGO.cpp<br>
> +++ lib/CodeGen/CodeGenPGO.cpp<br>
> @@ -602,27 +602,25 @@<br>
> return endian::read<uint64_t, little, unaligned>(Result);<br>
> }<br>
><br>
> -void CodeGenPGO::checkGlobalDecl(GlobalDecl GD) {<br>
> - // Make sure we only emit coverage mapping for one constructor/destructor.<br>
> - // Clang emits several functions for the constructor and the destructor of<br>
> - // a class. Every function is instrumented, but we only want to provide<br>
> - // coverage for one of them. Because of that we only emit the coverage mapping<br>
> - // for the base constructor/destructor.<br>
> - if ((isa<CXXConstructorDecl>(GD.getDecl()) &&<br>
> - GD.getCtorType() != Ctor_Base) ||<br>
> - (isa<CXXDestructorDecl>(GD.getDecl()) &&<br>
> - GD.getDtorType() != Dtor_Base)) {<br>
> - SkipCoverageMapping = true;<br>
> - }<br>
> -}<br>
> -<br>
> -void CodeGenPGO::assignRegionCounters(const Decl *D, llvm::Function *Fn) {<br>
> +void CodeGenPGO::assignRegionCounters(GlobalDecl GD, llvm::Function *Fn) {<br>
> + const Decl *D = GD.getDecl();<br>
> bool InstrumentRegions = CGM.getCodeGenOpts().ProfileInstrGenerate;<br>
> llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();<br>
> if (!InstrumentRegions && !PGOReader)<br>
> return;<br>
> if (D->isImplicit())<br>
> return;<br>
> + // Constructors and destructors may be represented by several functions in IR.<br>
> + // If so, instrument only base variant, others are implemented by delegation<br>
> + // to the base one, it would be counted twice otherwise.<br>
> + if (CGM.getTarget().getCXXABI().hasConstructorVariants() &&<br>
> + ((isa<CXXConstructorDecl>(GD.getDecl()) &&<br>
> + GD.getCtorType() != Ctor_Base) ||<br>
> + (isa<CXXDestructorDecl>(GD.getDecl()) &&<br>
> + GD.getDtorType() != Dtor_Base))) {<br>
> + SkipCoverageMapping = true;<br>
<br>
Is SkipCoverageMapping even necessary anymore? Since we return here, we<br>
shouldn't hit emitCounterRegionMapping at all, I think.<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + return;<br>
> + }<br>
> CGM.ClearUnusedCoverageMapping(D);<br>
> setFuncName(Fn);<br>
><br>
> Index: lib/CodeGen/CodeGenFunction.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CodeGenFunction.cpp<br>
> +++ lib/CodeGen/CodeGenFunction.cpp<br>
> @@ -946,8 +946,7 @@<br>
> StartFunction(GD, ResTy, Fn, FnInfo, Args, Loc, BodyRange.getBegin());<br>
><br>
> // Generate the body of the function.<br>
> - PGO.checkGlobalDecl(GD);<br>
> - PGO.assignRegionCounters(GD.getDecl(), CurFn);<br>
> + PGO.assignRegionCounters(GD, CurFn);<br>
> if (isa<CXXDestructorDecl>(FD))<br>
> EmitDestructorBody(Args);<br>
> else if (isa<CXXConstructorDecl>(FD))<br>
> Index: lib/CodeGen/CGStmtOpenMP.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CGStmtOpenMP.cpp<br>
> +++ lib/CodeGen/CGStmtOpenMP.cpp<br>
> @@ -141,7 +141,7 @@<br>
> ++Cnt, ++I;<br>
> }<br>
><br>
> - PGO.assignRegionCounters(CD, F);<br>
> + PGO.assignRegionCounters(GlobalDecl(CD), F);<br>
> CapturedStmtInfo->EmitBody(*this, CD->getBody());<br>
> FinishFunction(CD->getBodyRBrace());<br>
><br>
> Index: lib/CodeGen/CGStmt.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CGStmt.cpp<br>
> +++ lib/CodeGen/CGStmt.cpp<br>
> @@ -2167,7 +2167,7 @@<br>
> CXXThisValue = EmitLoadOfLValue(ThisLValue, Loc).getScalarVal();<br>
> }<br>
><br>
> - PGO.assignRegionCounters(CD, F);<br>
> + PGO.assignRegionCounters(GlobalDecl(CD), F);<br>
> CapturedStmtInfo->EmitBody(*this, CD->getBody());<br>
> FinishFunction(CD->getBodyRBrace());<br>
><br>
> Index: lib/CodeGen/CGObjC.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CGObjC.cpp<br>
> +++ lib/CodeGen/CGObjC.cpp<br>
> @@ -557,7 +557,7 @@<br>
> /// its pointer, name, and types registered in the class struture.<br>
> void CodeGenFunction::GenerateObjCMethod(const ObjCMethodDecl *OMD) {<br>
> StartObjCMethod(OMD, OMD->getClassInterface());<br>
> - PGO.assignRegionCounters(OMD, CurFn);<br>
> + PGO.assignRegionCounters(GlobalDecl(OMD), CurFn);<br>
> assert(isa<CompoundStmt>(OMD->getBody()));<br>
> incrementProfileCounter(OMD->getBody());<br>
> EmitCompoundStmtWithoutScope(*cast<CompoundStmt>(OMD->getBody()));<br>
> Index: lib/CodeGen/CGBlocks.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/CGBlocks.cpp<br>
> +++ lib/CodeGen/CGBlocks.cpp<br>
> @@ -1241,7 +1241,7 @@<br>
> if (IsLambdaConversionToBlock)<br>
> EmitLambdaBlockInvokeBody();<br>
> else {<br>
> - PGO.assignRegionCounters(blockDecl, fn);<br>
> + PGO.assignRegionCounters(GlobalDecl(blockDecl), fn);<br>
> incrementProfileCounter(blockDecl->getBody());<br>
> EmitStmt(blockDecl->getBody());<br>
> }<br>
</blockquote></div><br></div></div>