rnk created this revision. rnk added reviewers: xur, vsk. Herald added subscribers: Sanitizers, cfe-commits, hiraditya, eraman. Herald added projects: clang, Sanitizers, LLVM.
I hadn't realized that instrumentation runs before inlining, so we can't use the function as the comdat group. Doing so can create relocations against discarded sections when references to discarded __profc_ variables are inlined into functions outside the function's comdat group. In the future, perhaps we should consider standardizing the comdat group names that ELF and COFF use. It will save object file size, since __profv_$sym won't appear in the symbol table again. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D58737 Files: clang/test/Profile/cxx-templates.cpp compiler-rt/test/profile/coverage-inline.cpp llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp llvm/test/Instrumentation/InstrProfiling/PR23499.ll llvm/test/Instrumentation/InstrProfiling/comdat.ll
Index: llvm/test/Instrumentation/InstrProfiling/comdat.ll =================================================================== --- llvm/test/Instrumentation/InstrProfiling/comdat.ll +++ llvm/test/Instrumentation/InstrProfiling/comdat.ll @@ -17,8 +17,8 @@ ; ELF: @__profc_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat($__profv_foo_inline), align 8 ; ELF: @__profd_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($__profv_foo_inline), align 8 -; COFF: @__profc_foo_inline = internal global{{.*}}, section ".lprfc$M", comdat($foo_inline), align 8 -; COFF: @__profd_foo_inline = internal global{{.*}}, section ".lprfd$M", comdat($foo_inline), align 8 +; COFF: @__profc_foo_inline = linkonce_odr dso_local global{{.*}}, section ".lprfc$M", comdat, align 8 +; COFF: @__profd_foo_inline = internal global{{.*}}, section ".lprfd$M", comdat($__profc_foo_inline), align 8 define weak_odr void @foo_inline() comdat { call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0) ret void Index: llvm/test/Instrumentation/InstrProfiling/PR23499.ll =================================================================== --- llvm/test/Instrumentation/InstrProfiling/PR23499.ll +++ llvm/test/Instrumentation/InstrProfiling/PR23499.ll @@ -20,8 +20,8 @@ ; COFF-NOT: __profn__Z3barIvEvv -; COFF: @__profc__Z3barIvEvv = internal global [1 x i64] zeroinitializer, section "{{.*}}prfc$M", comdat($_Z3barIvEvv), align 8 -; COFF: @__profd__Z3barIvEvv = internal global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}prfd{{.*}}", comdat($_Z3barIvEvv), align 8 +; COFF: @__profc__Z3barIvEvv = linkonce_odr dso_local global [1 x i64] zeroinitializer, section "{{.*}}prfc$M", comdat, align 8 +; COFF: @__profd__Z3barIvEvv = internal global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}prfd{{.*}}", comdat($__profc__Z3barIvEvv), align 8 declare void @llvm.instrprof.increment(i8*, i64, i32, i32) #1 Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp =================================================================== --- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -743,25 +743,20 @@ // Move the name variable to the right section. Place them in a COMDAT group // if the associated function is a COMDAT. This will make sure that only one - // copy of counters of the COMDAT function will be emitted after linking. + // copy of counters of the COMDAT function will be emitted after linking. Keep + // in mind that this pass runs before the inliner, so we need to create a new + // comdat group for the counters and profiling data. If we use the comdat of + // the parent function, that will result in relocations against discarded + // sections. Comdat *Cmdt = nullptr; GlobalValue::LinkageTypes CounterLinkage = Linkage; if (needsComdatForCounter(*Fn, *M)) { if (TT.isOSBinFormatCOFF()) { - // There are two cases that need a comdat on COFF: - // 1. Functions that already have comdats (standard case) - // 2. available_externally functions (dllimport and C99 inline) - // In the first case, put all the data in the original function comdat. In - // the second case, create a new comdat group using the counter as the - // leader. It's linkage must be external, so use linkonce_odr linkage in - // that case. - if (Comdat *C = Fn->getComdat()) { - Cmdt = C; - } else { - Cmdt = M->getOrInsertComdat( - getVarName(Inc, getInstrProfCountersVarPrefix())); - CounterLinkage = GlobalValue::LinkOnceODRLinkage; - } + // For COFF, the comdat group name must be the name of a symbol in the + // group. Use the counter variable name. + Cmdt = M->getOrInsertComdat( + getVarName(Inc, getInstrProfCountersVarPrefix())); + CounterLinkage = GlobalValue::LinkOnceODRLinkage; } else { // For other platforms that use comdats (ELF), make a new comdat group for // all the profile data. It will be deduplicated within the current DSO. Index: compiler-rt/test/profile/coverage-inline.cpp =================================================================== --- compiler-rt/test/profile/coverage-inline.cpp +++ compiler-rt/test/profile/coverage-inline.cpp @@ -1,11 +1,18 @@ +// Test that the instrumentation puts the right linkage on the profile data for +// inline functions. // RUN: %clang_profgen -g -fcoverage-mapping -c -o %t1.o %s -DOBJECT_1 // RUN: %clang_profgen -g -fcoverage-mapping -c -o %t2.o %s // RUN: %clang_profgen -g -fcoverage-mapping %t1.o %t2.o -o %t.exe // RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.exe // RUN: llvm-profdata show %t.profraw -all-functions | FileCheck %s -// Test that the instrumentation puts the right linkage on the profile data for -// inline functions. +// Again, with optimizations and inlining. This tests that we use comdats +// correctly. +// RUN: %clang_profgen -O2 -g -fcoverage-mapping -c -o %t1.o %s -DOBJECT_1 +// RUN: %clang_profgen -O2 -g -fcoverage-mapping -c -o %t2.o %s +// RUN: %clang_profgen -g -fcoverage-mapping %t1.o %t2.o -o %t.exe +// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.exe +// RUN: llvm-profdata show %t.profraw -all-functions | FileCheck %s // CHECK: {{.*}}foo{{.*}}: // CHECK-NEXT: Hash: Index: clang/test/Profile/cxx-templates.cpp =================================================================== --- clang/test/Profile/cxx-templates.cpp +++ clang/test/Profile/cxx-templates.cpp @@ -10,8 +10,8 @@ // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE -check-prefix=ALL %s -// T0GEN: @[[T0C:__profc__Z4loopILj0EEvv]] = {{(linkonce_odr hidden|internal)}} global [2 x i64] zeroinitializer -// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = {{(linkonce_odr hidden|internal)}} global [2 x i64] zeroinitializer +// T0GEN: @[[T0C:__profc__Z4loopILj0EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer +// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer // T0GEN-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj0EEvv() // T0USE-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj0EEvv()
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits