[InstrProfiling] Delete linkage/visibility toggling for Windows

[MaskRay]

Summary

The linkage/visibility of __profn_* variables are derived
from the profiled functions.

extern_weak => linkonce
available_externally => linkonce_odr
internal => private
extern => private
_ => unchanged

The linkage/visibility of __profc_*/__profd_* variables are derived from
__profn_* with linkage/visibility wrestling for Windows.

The changes can be folded to the following without changing semantics.

if (TT.isOSBinFormatCOFF() && !NeedComdat) {
  Linkage = GlobalValue::InternalLinkage;
  Visibility = GlobalValue::DefaultVisibility;
}

That said, I think we can just delete the code block.

An extern/internal function will now use private __profc_*/__profd_*
variables, instead of internal ones. This saves some symbol table entries.

A non-comdat {linkonce,weak}_odr function will now use hidden external
__profc_*/__profd_* variables instead of internal ones. There is potential
object file size increase because such symbols need /INCLUDE: directives.
However such non-comdat functions are rare (note that non-comdat weak
definitions don't prevent duplicate definition error).

[rnk]

The main impact of this change seems to be that PGO data for non-comdat code is now private instead of internal. That's a good change: PGO data has really high object file size overhead, and LLD already discards PGO symbol names from the PDB because they are so large.

Non-comdat weak code mainly comes from uses of __attribute__((weak)), and this change will make corresponding PGO data weak as well. LLVM's implementation of weak linkage on COFF is really convoluted, so I'm not sure this is correct. Consider adding an integration test case for __attribute__((weak)) to the compiler-rt profile test suite to see if this works.

[MaskRay]

rebase (after some pre-committed test improvement)
add a attribute((weak)) test

[MaskRay]

improve test/profile tests

they are unrelated to this patch so i'll pre-commit them if this is accepted.

[MaskRay]

compiler-rt/test/profile/Windows/coverage-linkage-lld.cpp

10 ↗

(On Diff #348673)

An issue unrelated to this patch:

discarded* in coverage-linkage.cpp are discarded.

However, the weak symbol weak has weird mangling .weak.?weak@@YAXXZ.default.?discarded0@@YAXXZ (how does it contain discarded0 in its name)? @mstorsjo

[mstorsjo]

compiler-rt/test/profile/Windows/coverage-linkage-lld.cpp

10 ↗

(On Diff #348673)

That's because COFF weak symbols need to point at a different, non-weak global symbol. If you have multiple object files providing a weak symbol (which is meant to be allowed, at least in mingw environments), the default symbols that they point at would collide. Therefore, the default symbols get an extra string to make them unique - it tries to add the name of a non-comdat extern symbol defined in the same object file. As long as that symbol isn't multiply defined (which would cause conflicts in itself), that should make the defaults for the weak symbols unique too.

See D75989 for this odd design (which mimics what GCC/binutils do for that case).

[MaskRay]

compiler-rt/test/profile/Windows/coverage-linkage-lld.cpp

10 ↗

(On Diff #348673)

Thanks for the explanation!

[MaskRay]

Add coverage-weak-lld.cpp runtime tests - they add coverage. This patch doesn't change the behavior so I will commit the tests separately.

[MaskRay]

improve coverage-weak-lld.cpp

[MaskRay]

compiler-rt/test/profile/Windows/coverage-weak-lld.cpp

63

@rnk The behavior change of this patch is here: previously this is 0 (similar to the PROFILE1 case). It is now 2 (like ELF) because the counter is weak and resolves to the %t0.o copy.

I think this doesn't matter because (1) the new behavior matches ELF (2) weak overriding weak is a mingw extension, not supported by link.exe.

[rnk]

lgtm, thanks

[vsk]

Thanks for adding these excellent tests. Lgtm as well.

compiler-rt/test/profile/Linux/coverage-linkage.cpp

39 ↗

(On Diff #349133)

Consider leaving a brief note explaining how __attribute__((noinline)) inline affects linkonce_odr -- this may look confusing to some readers.

[MaskRay]

rename inline_ to linkonce_odr - this should be clearer about its linkage.

compiler-rt/test/profile/Windows/coverage-weak-lld.cpp

 // PROFILE1:      ?weak@@YAXXZ:
 // PROFILE1-NEXT:    Hash:
 // PROFILE1-NEXT:    Counters: 1
 // PROFILE1-NEXT:    Function count: 2
 
 // CHECK2: weak
 // CHECK2: weak
 
+/// __profc__Z4weakv in %t2.o is weak and resolves to the value of %t0.o's copy.
+/// Without GC it takes a duplicate entry.
 // PROFILE2:      ?weak@@YAXXZ:
 // PROFILE2-NEXT:    Hash:
 // PROFILE2-NEXT:    Counters: 1
 // PROFILE2-NEXT:    Function count: 2
 // PROFILE2:      ?weak@@YAXXZ:
 // PROFILE2-NEXT:    Hash:
 // PROFILE2-NEXT:    Counters: 1
-// PROFILE2-NEXT:    Function count: 0
+// PROFILE2-NEXT:    Function count: 2
 
 #ifdef OBJ_1
 #include <stdio.h>
 
 void weak() { puts("strong"); }
 void foo() { weak(); }
 
 #elif defined(OBJ_2)

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   auto It = ProfileDataMap.find(NamePtr);
   PerFunctionProfileData PD;
   if (It != ProfileDataMap.end()) {
     if (It->second.RegionCounters)
       return It->second.RegionCounters;
     PD = It->second;
   }
 
-  // Match the linkage and visibility of the name global. COFF supports using
-  // comdats with internal symbols, so do that if we can.
+  // Match the linkage and visibility of the name global.
   Function *Fn = Inc->getParent()->getParent();
   GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
-  if (TT.isOSBinFormatCOFF()) {
-    Linkage = GlobalValue::InternalLinkage;
-    Visibility = GlobalValue::DefaultVisibility;
-  }
 
   // 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. Keep
   // in mind that this pass may run 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.
-  bool NeedComdat = needsComdatForCounter(*Fn, *M);
-  if (NeedComdat) {
-    if (TT.isOSBinFormatCOFF()) {
-      // For COFF, put the counters, data, and values each into their own
-      // comdats. We can't use a group because the Visual C++ linker will
-      // report duplicate symbol errors if there are multiple external symbols
-      // with the same name marked IMAGE_COMDAT_SELECT_ASSOCIATIVE.
-      Linkage = GlobalValue::LinkOnceODRLinkage;
-      Visibility = GlobalValue::HiddenVisibility;
-    }
-  }
+  //
+  //
+  // For ELF, when not using COMDAT, put counters, data and values into a
+  // noduplicates COMDAT which is lowered to a zero-flag section group. This
+  // allows -z start-stop-gc to discard the entire group when the function is
+  // discarded.
+  bool NeedComdat = needsComdatForCounter(*Fn, *M);
   std::string DataVarName = getVarName(Inc, getInstrProfDataVarPrefix());
   auto MaybeSetComdat = [=](GlobalVariable *GV) {
-    // For ELF, when not using COMDAT, put counters, data and values into
-    // a noduplicates COMDAT which is lowered to a zero-flag section group.
-    // This allows linker GC to discard the entire group when the function
-    // is discarded.
     bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
     if (UseComdat) {
       auto GroupName = TT.isOSBinFormatCOFF() ? GV->getName() : DataVarName;
       Comdat *C = M->getOrInsertComdat(GroupName);
       if (!NeedComdat)
         C->setSelectionKind(Comdat::NoDuplicates);
       GV->setComdat(C);
     }

llvm/test/Instrumentation/InstrProfiling/linkage.ll

 @"__profn_linkage.ll:foo_internal" = private constant [23 x i8] c"linkage.ll:foo_internal"
 @__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"
 @__profn_foo_extern = linkonce_odr hidden constant [10 x i8] c"foo_extern"
 
 ; ELF: @__profc_foo = private global {{.*}} section "__llvm_prf_cnts", comdat($__profd_foo), align 8
 ; ELF: @__profd_foo = private global {{.*}} section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_foo = private global
 ; MACHO: @__profd_foo = private global
-; COFF: @__profc_foo = internal global
+; COFF: @__profc_foo = private global
 ; COFF-NOT: comdat
-; COFF: @__profd_foo = internal global
+; COFF: @__profd_foo = private global
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF: @__profc_foo_weak = weak hidden global {{.*}} section "__llvm_prf_cnts", comdat($__profd_foo_weak), align 8
 ; ELF: @__profd_foo_weak = weak hidden global {{.*}} section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_foo_weak = weak hidden global
 ; MACHO: @__profd_foo_weak = weak hidden global
-; COFF: @__profc_foo_weak = internal global
-; COFF: @__profd_foo_weak = internal global
+; COFF: @__profc_foo_weak = weak hidden global
+; COFF: @__profd_foo_weak = weak hidden global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF: @"__profc_linkage.ll:foo_internal" = private global {{.*}} section "__llvm_prf_cnts", comdat($"__profd_linkage.ll:foo_internal"), align 8
 ; ELF: @"__profd_linkage.ll:foo_internal" = private global {{.*}} section "__llvm_prf_data", comdat, align 8
 ; MACHO: @"__profc_linkage.ll:foo_internal" = private global
 ; MACHO: @"__profd_linkage.ll:foo_internal" = private global
-; COFF: @"__profc_linkage.ll:foo_internal" = internal global
-; COFF: @"__profd_linkage.ll:foo_internal" = internal global
+; COFF: @"__profc_linkage.ll:foo_internal" = private global
+; COFF: @"__profd_linkage.ll:foo_internal" = private global
 define internal void @foo_internal() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([23 x i8], [23 x i8]* @"__profn_linkage.ll:foo_internal", i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF: @__profc_foo_inline = linkonce_odr hidden global {{.*}} section "__llvm_prf_cnts", comdat($__profd_foo_inline), align 8
 ; ELF: @__profd_foo_inline = linkonce_odr hidden global {{.*}} section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_foo_inline = linkonce_odr hidden global
 ; MACHO: @__profd_foo_inline = linkonce_odr hidden global
-; COFF: @__profc_foo_inline = internal global{{.*}} section ".lprfc$M", align 8
-; COFF: @__profd_foo_inline = internal global{{.*}} section ".lprfd$M", align 8
+; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
+; COFF: @__profd_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfd$M", align 8
 define linkonce_odr void @foo_inline() {
   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
 }
 
 ; ELF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat($__profd_foo_extern), align 8
 ; ELF: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_foo_extern = linkonce_odr hidden global

llvm/test/Instrumentation/InstrProfiling/platform.ll

 
 @__profn_foo = private constant [3 x i8] c"foo"
 ; MACHO-NOT: __profn_foo
 ; ELF-NOT: __profn_foo
 ; WINDOWS-NOT: __profn_foo
 
 ; MACHO: @__profc_foo = private global [1 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
 ; ELF: @__profc_foo = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profd_foo), align 8
-; WINDOWS: @__profc_foo = internal global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
+; WINDOWS: @__profc_foo = private global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
 
 ; MACHO: @__profd_foo = private {{.*}}, section "__DATA,__llvm_prf_data,regular,live_support", align 8
 ; ELF: @__profd_foo = private {{.*}}, section "__llvm_prf_data", comdat, align 8
-; WINDOWS: @__profd_foo = internal global {{.*}}, section ".lprfd$M", align 8
+; WINDOWS: @__profd_foo = private global {{.*}}, section ".lprfd$M", align 8
 
 ; ELF: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names", align 1
 ; WINDOWS: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}lprfn$M", align 1
 
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }

llvm/test/Instrumentation/InstrProfiling/profiling.ll

 ; CHECK-NOT: __profn_bar
 @__profn_baz = private constant [3 x i8] c"baz"
 ; CHECK-NOT: __profn_baz
 
 ; ELF:   @__profc_foo = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profd_foo), align 8
 ; ELF:   @__profd_foo = private {{.*}}, section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_foo = private global [1 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
 ; MACHO: @__profd_foo = private {{.*}}, section "__DATA,__llvm_prf_data,regular,live_support", align 8
-; WIN:   @__profc_foo = internal global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
-; WIN:   @__profd_foo = internal {{.*}}, section ".lprfd$M", align 8
+; WIN:   @__profc_foo = private global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
+; WIN:   @__profd_foo = private {{.*}}, section ".lprfd$M", align 8
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF:   @__profc_bar = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profd_bar), align 8
 ; ELF:   @__profd_bar = private {{.*}}, section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_bar = private global [1 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
 ; MACHO: @__profd_bar = private {{.*}}, section "__DATA,__llvm_prf_data,regular,live_support", align 8
-; WIN:   @__profc_bar = internal global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
-; WIN:   @__profd_bar = internal {{.*}}, section ".lprfd$M", align 8
+; WIN:   @__profc_bar = private global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
+; WIN:   @__profd_bar = private {{.*}}, section ".lprfd$M", align 8
 define void @bar() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_bar, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF:   @__profc_baz = private global [3 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profd_baz), align 8
 ; ELF:   @__profd_baz = private {{.*}}, section "__llvm_prf_data", comdat, align 8
 ; MACHO: @__profc_baz = private global [3 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
 ; MACHO: @__profd_baz = private {{.*}}, section "__DATA,__llvm_prf_data,regular,live_support", align 8
-; WIN:   @__profc_baz = internal global [3 x i64] zeroinitializer, section ".lprfc$M", align 8
-; WIN:   @__profd_baz = internal {{.*}}, section ".lprfd$M", align 8
+; WIN:   @__profc_baz = private global [3 x i64] zeroinitializer, section ".lprfc$M", align 8
+; WIN:   @__profd_baz = private {{.*}}, section ".lprfd$M", align 8
 define void @baz() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_baz, i32 0, i32 0), i64 0, i32 3, i32 0)
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_baz, i32 0, i32 0), i64 0, i32 3, i32 1)
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_baz, i32 0, i32 0), i64 0, i32 3, i32 2)
   ret void
 }
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)