[AIX] Change the linkage of profiling counter/data to be private

[jsji]

Summary

We generate symbols like profc/profd for each function, and put them into csects.
When there are weak functions, we generate weak symbols for the functions as well,
with ELF (and some others), linker (binder) will discard and only keep one copy of the weak symbols.

However, on AIX, the current binder can NOT discard the weak symbols if we put all of them into the same csect,
as binder can NOT discard a subset of a csect.

This creates a unique challenge for using those symbols to calculate some relative offsets.

This patch changed the linkage of profc/profd symbols to be private, so that all the profc/profd for each weak symbol will be *local* to objects, and all kept in the csect, so we won't have problem. Although only one of the counters will be used, all the pointer in the profd is correct.

The downside is that we won't be able to discard the duplicated counters and profile data,
but those can not be discarded even if we keep the weak linkage,
due to the binder limitation of not discarding a subsect of the csect either .

[jsji]

Restore the limitation of sample profiling.

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

869

Then you can just keep the existing weak symbols.

The weak symbols will not be selected due to linker semantics.

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

978

This place is for binary formats which can properly GC metadata sections. If XCOFF can't, it shouldn't be added here.

[jsji]

Address comments.

[jsji]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

869

Not sure what you meant?

[jsji]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

869

To be clear, keeping the weak linkage is causing issues in our test, and we investigate the problem , together with AIX binder owner, and it is the limitation in binder support that we can't ensure the relocation getting resolved to the intended weak symbol. So this is what we can do right now for enabling PGO.

We may be able to lift it this limitation later if binder get updated and do what we want, but this is limitation for now.

[MaskRay]

we can NOT guarantee that the relocations get resolved to the intended weak symbol, so we can not ensure the correctness of the relative CounterPtr, so we have to use private linkage for counter and data symbols.

In other binary formats the first weak definition is selected and other weak definitions are discarded.
Do you mean that AIX ld doesn't pick the first weak definition?

[jsji]

In other binary formats the first weak definition is selected and other weak definitions are discarded.
Do you mean that AIX ld doesn't pick the first weak definition?

No. I think this is exactly what is causing problem here.

eg: if we have two weak symbols （weak_func）, and we generate the weak counter/data as usual:

__llvm_prf_cnts:

__profc_weak_func(1):

__profc_weak_func(2):

__llvm_prf_data:

__profd_weak func(1):

Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer

__profd_weak_func(2):

Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer

The relative pointer calculation in one of the __profd_weak_fun will be wrong after linking.

[MaskRay]

In D110422#3021283, @jsji wrote:

In other binary formats the first weak definition is selected and other weak definitions are discarded.
Do you mean that AIX ld doesn't pick the first weak definition?

No. I think this is exactly what is causing problem here.

eg: if we have two weak symbols （weak_func）, and we generate the weak counter/data as usual:

__llvm_prf_cnts:

__profc_weak_func(1):

__profc_weak_func(2):

__llvm_prf_data:

__profd_weak func(1):

Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer

__profd_weak_func(2):

Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer

The relative pointer calculation in one of the __profd_weak_fun will be wrong after linking.

Can you elaborate the Pos_Rel __profc_weak_func notation?

Note: In other binary formats' scheme, it doesn't matter whether non-first weak definitions are discarded or not.
After linking, the non-first weak definitions will be unreachable anyway, so their internal references don't really matter.

[jsji]

Pos_Rel profc_weak_func notation is just the normal relocation to symbol profc_weak_func.

The relocation here is to calculate the relative pointer (offset) to profcweak_func from profdweak__func.

In example above, the offsets value after binding will be the same for both instances, as the symbol profcweak__func will be the one that is chosen by binder. Let us say it is the value of first instances.

Then when we calculate the profc_weak_func pointer for 2nd instances, we get garbage value.

[MaskRay]

The description is still unclear.

Say a.o has a weak foo, __profc_foo, __profd_foo, b.o has a weak foo, __profc_foo, __profd_foo.
The linker picks the definitions from a.o. In the PGO implementation, it doesn't whether the non-discarded b.o __profd_foo has garbage value.

[jsji]

In D110422#3021535, @MaskRay wrote:

The description is still unclear.

Say a.o has a weak foo, __profc_foo, __profd_foo, b.o has a weak foo, __profc_foo, __profd_foo.
The linker picks the definitions from a.o. In the PGO implementation, it doesn't whether the non-discarded b.o __profd_foo has garbage value.

We have 3 sets weak symbols here: weak_func, profc_weak_foo, profd_weak_func. We can't ensure that binder always choose 3 of them from same object.

[MaskRay]

In D110422#3021572, @jsji wrote:

In D110422#3021535, @MaskRay wrote:

The description is still unclear.

Say a.o has a weak foo, __profc_foo, __profd_foo, b.o has a weak foo, __profc_foo, __profd_foo.
The linker picks the definitions from a.o. In the PGO implementation, it doesn't whether the non-discarded b.o __profd_foo has garbage value.

We have 3 sets weak symbols here: weak_func, profc_weak_foo, profd_weak_func. We can't ensure that binder always choose 3 of them from same object.

Oh, this is a serious enough bug. I am not sure working around the bug by switching to PrivateLinkage will help.
The values will be wrong as well.

[jsji]

We have verified all the profile counters in SPEC , all are OK.

[jsji]

Using the PrivateLinkage, all the profc/profd for each weak symbol will be *local* to objects, and all kept in the csect,
so we won't have problem. Although only one of the counters will be used, all the pointer in the profd is correct.

The downside is that we won't be able to discard the duplicated counters and profile data,
but those can not be discarded even if we keep the weak linkage,
due to the binder limitation of not discarding only part of the csect either .

[jsji]

@MaskRay Do you have further comments or alternative solutions? Thanks.

[MaskRay]

The description isn't clear why the previous weak symbol usage does not work. Have you verified that it does not work?

[jsji]

In D110422#3025133, @MaskRay wrote:

The description isn't clear why the previous weak symbol usage does not work. Have you verified that it does not work?

Yes, we started with the weak symbols for profc/profd first, and tested it. We met problems, and we spent time investigate why they are not working.

I have update the description to include most of the details. Let me know if it is still unclear.

[MaskRay]

Sorry, I still don't understand this response:

We have 3 sets weak symbols here: weak_func, profc_weak_foo, profd_weak_func. We can't ensure that binder always choose 3 of them from same object.

So this part of the description isn't clear to me:

However, on AIX, the current binder can NOT discard the weak symbols if we put all of them into the same csect, as binder can NOT discard only part of a csect.

This creates a unique challenge for using those symbols to calculate some relative offset.

Say a.o has a weak foo, __profc_foo, __profd_foo. The __profd_foo references the __profc_foo

b.o has another set of weak foo, __profc_foo, __profd_foo. The __profd_foo references the __profc_foo.

What's the linker (called binder?) behavior? "We can't ensure that binder always choose 3 of them from same object."?
But is that a problem? For linkonce_odr functions, they should have identical semantics in a.o and b.o.
(There can be ODR issues if you use advanced PGO techniques like value profiling, but I assume that it does not work and will be difficult to make work and more importantly, it is also of lower value anyway.)

So the linker picking the a.o foo and the b.o __profc_foo/__profd_foo isn't a poblem.

I understand that PrivateLinkage can avoid the issues but you need to justify the complexity in InstrProfiling.cpp with the value.
Currently I don't think using weak symbol is a big problem, especially if the AIX linker will be fixed anyway in the future.

I expect that the description includes the justification.

[jsji]

OK, I may not describe the example clearly.

Let me use example code with offsets and llvm internal calculations as the example, so that you might be clearer.

Let us say we have 2 objects , which both have weak function foo (and some non-weak functions).

[2054]  m   0x110000ec0     .data     1  unamex                    __llvm_prf_cnts
[2056]  m   0x110000ef8     .data     1    weak                    __profc__foo
[1389]  m   0x110001540     .data     1  unamex                    __llvm_prf_cnts
[1391]  m   0x1100015a8     .data     1    weak                    __profc__foo                <====== chosen by binder

[2290]  m   0x110001ca8     .data     1  unamex                    __llvm_prf_data
[2292]  m   0x110001ce0     .data     1    weak                    __profd__foo
[1633]  m   0x110003678     .data     1  unamex                    __llvm_prf_data
[1635]  m   0x1100036b0     .data     1    weak                    __profd__foo                <=======  chosen by binder

In binding, binder chose 0x1100015a8 for __profc__foo, and 0x1100036b0 for __profd__foo.
(Not the 1st in the csect, but 1st seen by binder)

At the beginning CountersDelta is 0xfffffffffffff218. ( 0x110000ec0 [2054] - 0x110001ca8 [2290] ) .

The first record is for non-weak function, so we are OK.

Then we move forward to read foo counters,
CountersDelta is then updated to 0xfffffffffffff1e0 ( 0xfffffffffffff218 - sizeof(Data))
CounterPtr is now 0x1100015a8 - 0x1100036b0 = 0xffffffffffffdef8

CountersOffset is now 0x1ffffffffffffda3 ( CounterPtr - CountersDelta / sizeof(unit64_6))

CountersOffset > MaxNumCounters !

If we have more weak symbols, the symbols chosen by binder may be interleaving in the csects,
we won't be able to calculate the CountersOffset correctly for all of them.

If we use private linkage, we will use 0x11000ef8 and 0x110001ce0 to calculate the counterPtr, then we get everything as expected:

CounterPtr: 0xfffffffffffff218
CountersDelta: 0xfffffffffffff218
CountersOffset: 0x0

[MaskRay]

However, on AIX, the current binder can NOT discard the weak symbols if we put all of them into the same csect, as binder can NOT discard only part of a csect.

"as binder can NOT discard only part of a csect." this part caused confusion to me...

CountersOffset is now 0x1ffffffffffffda3 ( CounterPtr - CountersDelta / sizeof(unit64_6))

CountersOffset > MaxNumCounters !

It is true that there are two pairs of weak profc/profd, but the first profc has a zero value, so I assume that it is fine?

The second pair of profc/profd has correct value.

I haven't verified but it is possible that PGO picks the first pair (zero value) and have an incorrect counter for foo.

---

Does your example imply that the weak symbol __profc_foo has 2 definitions with conflicting values?
I presume it can cause more fallout.
Any idea whether the linker bug will be fixed?

[jsji]

"as binder can NOT discard only part of a csect." this part caused confusion to me...

OK, yeah, csect is special to XCOFF, so this is not the same concept of sections in ELF.

I haven't verified but it is possible that PGO picks the first pair (zero value) and have an incorrect counter for foo.

As I mentioned before, we verified (with llvm-profdata show --all-functions --counts) on all SPEC benchmarks that all the counters are good with private linkage.

---

Does your example imply that the weak symbol __profc_foo has 2 definitions with conflicting values?

No, they are identical weak functions. Those offsets are actually from real example using std::stringbuf,
the foo is actually _ZNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEC2Ej.

Any idea whether the linker bug will be fixed?

Unfortunately, I just double confirmed with AIX linker (binder) owner,
there is NO plan in near future of lifting the limitation that binder can NOT discard only part of a csect.

[MaskRay]

In D110422#3025911, @jsji wrote:

"as binder can NOT discard only part of a csect." this part caused confusion to me...

OK, yeah, csect is special to XCOFF, so this is not the same concept of sections in ELF.

"as binder can NOT discard a subset of a csect."?

I haven't verified but it is possible that PGO picks the first pair (zero value) and have an incorrect counter for foo.

As I mentioned before, we verified (with llvm-profdata show --all-functions --counts) on all SPEC benchmarks that all the counters are good with private linkage.

---

Does your example imply that the weak symbol __profc_foo has 2 definitions with conflicting values?

No, they are identical weak functions. Those offsets are actually from real example using std::stringbuf,
the foo is actually _ZNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEC2Ej.

Any idea whether the linker bug will be fixed?

Unfortunately, I just double confirmed with AIX linker (binder) owner,
there is NO plan in near future of lifting the limitation that binder can NOT discard only part of a csect.

The linker (binder) doesn't have to discard a subset of csect, but it should ensure there are not two non-local symbols with the same name.
The latter is what I request.

Without section deduplication we just end up with PDP-11 a.out extended with .weak directive, it isn't too bad. Mach-O uses this model as well.
(PE-COFF/ELF properly deduplicates in the absence of section based GC.)
Having two non-local symbols could cause serious symbol resolution problems.

[jsji]

"as binder can NOT discard a subset of a csect."?

Thanks, this does looks better!

The linker (binder) doesn't have to discard a subset of csect, but it should ensure there are not two non-local symbols with the same name.
The latter is what I request.

Without section deduplication we just end up with PDP-11 a.out extended with .weak directive, it isn't too bad. Mach-O uses this model as well.
(PE-COFF/ELF properly deduplicates in the absence of section based GC.)
Having two non-local symbols could cause serious symbol resolution problems.

That I can definitely forward to AIX binder team, but we can't expect they implement it right away for us.

But yeah, once they can fix it, we can update to revert back to weak linkages.

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

865

"Due to the current limitation of binder,"

Replace "current" with "as of version XX or date"

[jsji]

Address comments.

[MaskRay]

I still think the description can be rephrased to be shorter and clearer, and am not very sure this workaround should be added.
But I will take a vacation and be back after one week and don't want to appear that I am blocking this change.

So LGTM once you decrease the number of if (TT.isOSBinFormatXCOFF()) { from two to one.
If you track where Linkage and Visibility are updated it should be clear one place is not needed.

Also ideally the driver change should be in a separate patch.

[jsji]

Split the Driver changes to another patch, also fixed the duplicate code.

[jsji]

Remove empty line.

[jsji]

@MaskRay Thank you so much for your review and discussion!

clang/test/Profile/cxx-templates.cpp

 // Tests for instrumentation of templated code. Each instantiation of a template
 // should be instrumented separately.
 
 // RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument=clang > %tgen
 // RUN: FileCheck --input-file=%tgen -check-prefix=T0GEN -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tgen -check-prefix=T100GEN -check-prefix=ALL %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-templates.proftext -o %t.profdata
 // RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata > %tuse
 // 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|dso_local)}} global [2 x i64] zeroinitializer
-// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer
+// The linkage can be target dependent, so accept all linkage here,
+// the linkage tests for different target are in llvm/test/Instrumentation/InstrProfiling/profiling.ll
+// T0GEN: @[[T0C:__profc__Z4loopILj0EEvv]] = {{.*}} global [2 x i64] zeroinitializer
+// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = {{.*}} global [2 x i64] zeroinitializer
 
 // T0GEN-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj0EEvv()
 // T0USE-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj0EEvv()
 // T100GEN-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj100EEvv()
 // T100USE-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj100EEvv()
 template <unsigned N> void loop() {
   // ALL-NOT: ret
   // T0GEN: store {{.*}} @[[T0C]], i64 0, i64 0

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

if (It != ProfileDataMap.end()) {
     PD = It->second;
   }
 
   // Match the linkage and visibility of the name global.
   Function *Fn = Inc->getParent()->getParent();
   GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
 
+  // Due to the limitation of binder as of 2021/09/28, the duplicate weak
+  // symbols in the same csect won't be discarded. When there are duplicate weak
+  // symbols, we can NOT guarantee that the relocations get resolved to the
+  // intended weak symbol, so we can not ensure the correctness of the relative
+  // CounterPtr, so we have to use private linkage for counter and data symbols.
+  if (TT.isOSBinFormatXCOFF()) {
+    Linkage = GlobalValue::PrivateLinkage;
+    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.
   //
#include "llvm/ProfileData/InstrProfData.inc"
   //
   // On COFF, a comdat leader cannot be local so we require DataReferencedByCode
   // to be false.
   //
   // If profd is in a deduplicate comdat, NS==0 with a hash suffix guarantees
   // that other copies must have the same CFG and cannot have value profiling.
   // If no hash suffix, other profd copies may be referenced by code.
   if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) &&
       (TT.isOSBinFormatELF() ||
        (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
     Linkage = GlobalValue::PrivateLinkage;
     Visibility = GlobalValue::DefaultVisibility;
   }
   auto *Data =
       new GlobalVariable(*M, DataTy, false, Linkage, nullptr, DataVarName);
   // Reference the counter variable with a label difference (link-time
   // constant).

llvm/test/Instrumentation/InstrProfiling/profiling.ll

 }
 
 ; ELF: @__profc_foo_weak = weak hidden global{{.*}}section "__llvm_prf_cnts", comdat, align 8
 ; ELF: @__profd_foo_weak = private global{{.*}}section "__llvm_prf_data", comdat($__profc_foo_weak)
 ; MACHO: @__profc_foo_weak = weak hidden global
 ; MACHO: @__profd_foo_weak = weak hidden global
 ; COFF: @__profc_foo_weak = weak hidden global
 ; COFF: @__profd_foo_weak = private global
-; XCOFF: @__profc_foo_weak = weak hidden global
-; XCOFF: @__profd_foo_weak = weak hidden global
+; XCOFF: @__profc_foo_weak = private global
+; XCOFF: @__profd_foo_weak = private 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, align 8
 ; ELF: @"__profd_linkage.ll:foo_internal" = private global{{.*}}section "__llvm_prf_data", comdat($"__profc_linkage.ll:foo_internal"), align 8
 ; MACHO: @"__profc_linkage.ll:foo_internal" = private global
 ; MACHO: @"__profd_linkage.ll:foo_internal" = private global
 ; COFF: @"__profc_linkage.ll:foo_internal" = private global
 ; COFF: @"__profd_linkage.ll:foo_internal" = private global
 ; XCOFF: @"__profc_linkage.ll:foo_internal" = private global
 ; XCOFF: @"__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, align 8
 ; ELF: @__profd_foo_inline = private global{{.*}}section "__llvm_prf_data", comdat($__profc_foo_inline), align 8
 ; MACHO: @__profc_foo_inline = linkonce_odr hidden global
 ; MACHO: @__profd_foo_inline = linkonce_odr hidden global
 ; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
 ; COFF: @__profd_foo_inline = private global{{.*}} section ".lprfd$M", align 8
-; XCOFF: @__profc_foo_inline = linkonce_odr hidden global
-; XCOFF: @__profd_foo_inline = linkonce_odr hidden global
+; XCOFF: @__profc_foo_inline = private global
+; XCOFF: @__profd_foo_inline = private global
 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, align 8
 ; ELF: @__profd_foo_extern = private global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
 ; MACHO: @__profc_foo_extern = linkonce_odr hidden global
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
 ; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
 ; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
-; XCOFF: @__profc_foo_extern = linkonce_odr hidden global
-; XCOFF: @__profd_foo_extern = linkonce_odr hidden global
+; XCOFF: @__profc_foo_extern = private global
+; XCOFF: @__profd_foo_extern = private global
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 ; ELF:         @llvm.compiler.used = appending global {{.*}} @__profd_foo {{.*}}