[Coverage] Comdat section name should be same as the variable name in COFF format

[adamf]

Summary

COFF format requires to have Comdat section names same as the variable name.
For Instruction Profile Data Variable the Comdat name wasn't equal to the variable name.
It caused linker error like: "error LNK2005: ___profd_?compute@@YAHH@Z already defined in Source.obj" e.g. for functions included into multiple cpp files.

[vsk]

Could you update some tests in 'test/Instrumentation/InstrProfiling/' to reflect this change? PR23499.ll and linkage.ll are good candidates.

Also, do you know why the change in r256220 ([PGO] Fix another comdat related issue for COFF) was not enough to fix linkage issues when using COFF?

[davidxl]

This patch basically splits the comdat section for data, counters and value vars with COFF.

[rnk]

+1 to tests, it would help illustrate what's changing.

[adamf]

Fixed use after free from previous patch on non x86_64-pc-win32-coff.
Updated test according to the fix.

[adamf]

Could you update some tests in 'test/Instrumentation/InstrProfiling/' to reflect this change? PR23499.ll and linkage.ll are good candidates.

Thanks, I have updated this test. linkage.ll was broken because of bug in my fix (use after free in ternary operator).

+1 to tests, it would help illustrate what's changing.

I have added some additional checks in PR23499.ll to emphasize my change.

To avoid XY Problem I will start from test case to reproduce my initial problem. Based on this example you can easily reproduce the problem.
In attachment is source code, compilation command and result error (shortly also below).

LinkingError.zip2 KBDownload

Shortly how to reproduce this linker error:
I have 3 files

"Header.h"

#pragma once
inline int compute(int i){return i+1;}

"A.cpp"

#include "Header.h"
int main(){return compute(1);}

"B.cpp"

#include "Header.h"
int common(){return compute(2);}

Then I compiled it using command (on Windows):

clang++.exe -fprofile-instr-generate -fcoverage-mapping A.cpp B.cpp

Finally I received linker error message:

B-5be2e4.o : error LNK2005: ___profd_?compute@@YAHH@Z already defined in A-a141c9.o

Now I can describe details about my solution:

In above example the coverage data for function "compute" has linkage type "linkeonce_odr" for variables with prefix _profd_ and _profc_ ("_profc_?compute@@YAHH@Z" and "_profd_?compute@@YAHH@Z").
It is because these data are computed from header function file and are duplicated in A.obj and B.obj so linker has to merge these symbols.
But I have observed if the COMDAT section name is different to the variable name then the linker still fails to merge these symbols.
In the above example the COMDAT section name is "_profc_?compute@@YAHH@Z" and it is equal to variable with prefix __profc_. So the linker complains only about variable with prefix _profd_ .
So my fix creates COMDAT section name equals to the variable name in case of triple: x86_64-pc-win32-coff

Also, do you know why the change in r256220 ([PGO] Fix another comdat related issue for COFF) was not enough to fix linkage issues when using COFF?

COMDAT section generated by the function "getOrCreateProfileComdat" is added to 3 variables: CounterPtr, ValuesVar and Data.
The COMDAT name was always the same (for each variable) based on return value from function: "getInstrProfCountersVarPrefix" (after r260117), after r256220 it was "getInstrProfNameVarPrefix".
Before my patch the linker properly merged symbols for variables with prefix profc_ but not for profd_. After patch r256220, but before r260117 I suppose the problem was with both (profc_ and profd_).

[rnk]

I suspect that the appropriate fix is to give the __profd variables internal linkage, not to change the comdat name. The linker will still discard duplicate profc and profd sections with this change.

Actually, I'm surprised that the profiling data isn't comdat associated with the function itself. Is it possible for instrprofiling to be different across different TUs, or to link PGO instrumented code with non-PGO instrumented code? If that's the case, we probably want to make all profiling metadata associated with the function itself.

[davidxl]

lib/Transforms/Instrumentation/InstrProfiling.cpp

373

Please also fix the stale comment.

[adamf]

I suspect that the appropriate fix is to give the __profd variables internal linkage, not to change the comdat name. The linker will still discard duplicate profc and profd sections with this change.

I have made few tests and changing to InternalLinkage seems also to work. The patch is in attachment.

InstrProfiling.patch1 KBDownload

.
This patch affects all formats, not only COFF.
With InternalLinkage we should change both _ _profd and _ _profc. (If we change only _ _ profd then counters in the coverage report are not properly interpreted).
Another advantage of using InternalLinkage is that we can check the counters for each translation unit.

Because fixing this problem starts to affect more amount of code and requires wider knowledge I am afraid that I won't be able to provide fully valuable fix.

[adamf]

Alternative solution with Internal Linkage for COFF format.

[davidxl]

lgtm

[rnk]

Are we sure we want it like this? Doesn't this retain instrprof data structures from both A.obj and B.obj for inline functions present in both objs? Won't that be a problem?

From @davidxl :

It was comdat associated with the function before -- but it was wrong. The problem is that different comdat functions may have different CFGs (due to different early inlining), leading to incompatible profile counter data. Note that a comdat function can be later inlined, so using a shared copy of profile counter will be bad (out of bound access, not to mention profile use phase can not use it).

I agree, if instrprof runs before inlining, then we don't want these to be in a comdat group with the function. It's not safe for other functions to reference the data after inlining.

However, if instrprof runs after "early inlining", which I didn't think we had any of, then how have we solved the problem of different comdat functions having different CFGs at instrumentation time?

lib/Transforms/Instrumentation/InstrProfiling.cpp

   if (!needsComdatForCounter(F, M))
     return nullptr;
 
-  // COFF format requires a COMDAT section to have a key symbol with the same
-  // name. The linker targeting COFF also requires that the COMDAT
-  // a section is associated to must precede the associating section. For this
-  // reason, we must choose the counter var's name as the name of the comdat.
-  StringRef ComdatPrefix = (Triple(M.getTargetTriple()).isOSBinFormatCOFF()
-                                ? getInstrProfCountersVarPrefix()
-                                : getInstrProfComdatPrefix());
+  // In COFF format we use Internal Linkage.
+  if (Triple(M.getTargetTriple()).isOSBinFormatCOFF())
+    return nullptr;
+
+  StringRef ComdatPrefix = getInstrProfComdatPrefix();
   return M.getOrInsertComdat(StringRef(getVarName(Inc, ComdatPrefix)));
 }
 

   // only one copy of counters of the COMDAT function will be emitted after
   // linking.
   Function *Fn = Inc->getParent()->getParent();
-  Comdat *ProfileVarsComdat = nullptr;
-  ProfileVarsComdat = getOrCreateProfileComdat(*M, *Fn, Inc);
-
+  Comdat *ProfileVarsComdat = getOrCreateProfileComdat(*M, *Fn, Inc);
+  GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
+  GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
+  if (Triple(M->getTargetTriple()).isOSBinFormatCOFF()) {
+    Linkage = GlobalValue::InternalLinkage;
+    Visibility = GlobalValue::DefaultVisibility;
+  }
   uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
   LLVMContext &Ctx = M->getContext();
   ArrayType *CounterTy = ArrayType::get(Type::getInt64Ty(Ctx), NumCounters);

 
   // Create the counters variable.
   auto *CounterPtr =
-      new GlobalVariable(*M, CounterTy, false, NamePtr->getLinkage(),
+      new GlobalVariable(*M, CounterTy, false, Linkage,
                          Constant::getNullValue(CounterTy),
                          getVarName(Inc, getInstrProfCountersVarPrefix()));
-  CounterPtr->setVisibility(NamePtr->getVisibility());
+  CounterPtr->setVisibility(Visibility);
   CounterPtr->setSection(
       getInstrProfSectionName(IPSK_cnts, TT.getObjectFormat()));
   CounterPtr->setAlignment(8);

       ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
 
       auto *ValuesVar =
-          new GlobalVariable(*M, ValuesTy, false, NamePtr->getLinkage(),
+          new GlobalVariable(*M, ValuesTy, false, Linkage,
                              Constant::getNullValue(ValuesTy),
                              getVarName(Inc, getInstrProfValuesVarPrefix()));
-      ValuesVar->setVisibility(NamePtr->getVisibility());
+      ValuesVar->setVisibility(Visibility);
       ValuesVar->setSection(
           getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
       ValuesVar->setAlignment(8);

 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Init,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
-  auto *Data = new GlobalVariable(*M, DataTy, false, NamePtr->getLinkage(),
+  auto *Data = new GlobalVariable(*M, DataTy, false, Linkage,
                                   ConstantStruct::get(DataTy, DataVals),
                                   getVarName(Inc, getInstrProfDataVarPrefix()));
-  Data->setVisibility(NamePtr->getVisibility());
+  Data->setVisibility(Visibility);
   Data->setSection(getInstrProfSectionName(IPSK_data, TT.getObjectFormat()));
   Data->setAlignment(INSTR_PROF_DATA_ALIGNMENT);
   Data->setComdat(ProfileVarsComdat);

test/Instrumentation/InstrProfiling/PR23499.ll

 
 
 ; COFF-NOT: __profn__Z3barIvEvv
-; COFF: @__profc__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}prfc", comdat, align 8
-; COFF: @__profd__Z3barIvEvv = linkonce_odr hidden 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
+; COFF: @__profc__Z3barIvEvv = internal global [1 x i64] zeroinitializer, section "{{.*}}prfc", 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{{.*}}", align 8
 
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32) #1