[Linker] Support weak symbols in nodeduplicate COMDAT group

[phosek]

Summary

When a nodeduplicate COMDAT group contains a weak symbol, choose
a non-weak symbol (or one of the weak ones) rather than reporting
an error. This should address issue PR51394.

With the current IR representation, a generic comdat nodeduplicate
semantics is not representable for LTO. In the linker, sections and
symbols are separate concepts. A dropped weak symbol does not force the
defining input section to be dropped as well (though it can be collected
by GC). In the IR, when a weak linkage symbol is dropped, its associate
section content is dropped as well.

For InstrProfiling, which is where ran into this issue in PR51394, the
deduplication semantic is a sufficient workaround.

[MaskRay]

This is a workaround instead of a proper fix. comdat nodeduplicate does not perform deduplication. LinkFromSrc is for deduplication, so not suitable.
All the members should be tested.
For InstrProfiling.cpp, I agree that the deduplication semantic is a sufficient workaround.

llvm/test/Linker/comdat4.ll

2 ↗

(On Diff #368573)

Use split-file to avoid an one-off Inputs/ file

[MaskRay]

The description should call out that with current IR, a generic comdat nodeduplicate for regular LTO is not representable.

In the linker, sections and symbols are separate concepts. A dropped weak symbol does not force the drop of the defining input section (though can be collected by GC).
While in the IR, when a weak linkage symbol is dropped, its associate section content is dropped as well.
profc and profd must be parallel, so we need to be careful.
For their regular LTO behavior, a comdat any happens to work because we don't really the need other members in the comdat whose leader is weak.

[phosek]

In D108689#2968019, @MaskRay wrote:

The description should call out that with current IR, a generic comdat nodeduplicate for regular LTO is not representable.

In the linker, sections and symbols are separate concepts. A dropped weak symbol does not force the drop of the defining input section (though can be collected by GC).
While in the IR, when a weak linkage symbol is dropped, its associate section content is dropped as well.
profc and profd must be parallel, so we need to be careful.
For their regular LTO behavior, a comdat any happens to work because we don't really the need other members in the comdat whose leader is weak.

@mcgrathr suggested using a COMDAT group anytime the instrumented function is weak, I can implement that solution instead if that's fine with you.

[MaskRay]

If you mean zero flag groups for export linkage functions and GRP_COMDAT groups for weak linkage functions.
If the symbol resolution says the strong symbol overrides the weak one, no rule says that the GRP_COMDAT group should be discarded because it is unclear it is a non-prevailing group.
For local linkage functions, we'd still use a zero flag group.

Symbol resolution is orthogonal to GRP_COMDAT selection.
Switching to GRP_COMDAT does not imply that the weak leader symbol makes the group discarded.
Therefore, I think switching the group flag doesn't help.
The fundamental issue is that LLVM IR is symbol oriented. It cannot encode the section oriented ELF group semantics.

Since GRP_COMDAT doesn't help, I think accepting the deficiency and using comdat any like semantics for regular LTO is fine.
We just need to be careful not to request too much from comdat nodeduplicate.

[MaskRay]

Err, this can be properly fixed by renaming to a private (ELF) internal (COFF) linkage symbol. Let me take a stab.

But this should be a safe choice for release/13.x

[MaskRay]

More complete (though still partial) fix: D108879

[phosek]

In D108689#2970956, @MaskRay wrote:

More complete (though still partial) fix: D108879

Do you prefer landing your change instead and abandoning this one?

[MaskRay]

In D108689#2970961, @phosek wrote:

In D108689#2970956, @MaskRay wrote:

More complete (though still partial) fix: D108879

Do you prefer landing your change instead and abandoning this one?

Landing just D108879 if no rush for release/13.x:) (which I assume yes because the issue is from a proprietary group which uses rolling updates instead of releases).

[phosek]

In D108689#2970982, @MaskRay wrote:

In D108689#2970961, @phosek wrote:

In D108689#2970956, @MaskRay wrote:

More complete (though still partial) fix: D108879

Do you prefer landing your change instead and abandoning this one?

Landing just D108879 if no rush for release/13.x:) (which I assume yes because the issue is from a proprietary group which uses rolling updates instead of releases).

I think we need to cherry pick this into 13.x. I can land this one and prepare a cherry-pick for 13.x, and we can then land D108879 on top as a more complete solution, is that OK with you?

[MaskRay]

It is ok. You will need to rebase anyway ... because I've changed the tests and some LinkFromSrc.

[bd1976llvm]

Thanks for the timely fix :)

I noticed that Comdat::SelectionKind::NoDeduplicate is also used for the code/data added when using sanitizers. Can I get your opinion on whether this fix is sufficient for that case as well?

I looked at D108879 but that is only required when the initializers of variables in the discarded comdat are important and I don't *think* that's the case when using sanitizers - but I'm not very familiar with the area.

Thanks in advance!

llvm/lib/Linker/LinkModules.cpp

return emitError("Linking COMDATs named '" + ComdatName +
                      "': invalid selection kinds!");
   }
 
   switch (Result) {
   case Comdat::SelectionKind::Any:
     // Go with Dst.
     From = LinkFrom::Dst;
     break;
-  case Comdat::SelectionKind::NoDeduplicate:
+  case Comdat::SelectionKind::NoDeduplicate: {
+    const GlobalVariable *DstGV;
+    const GlobalVariable *SrcGV;
+    if (getComdatLeader(DstM, ComdatName, DstGV) ||
+        getComdatLeader(*SrcM, ComdatName, SrcGV))
+      return true;
+
+    if (SrcGV->isWeakForLinker()) {
+      // Go with Dst.
+      From = LinkFrom::Dst;
+    } else if (DstGV->isWeakForLinker()) {
+      // Go with Src.
+      From = LinkFrom::Src;
+    } else {
       return emitError("Linking COMDATs named '" + ComdatName +
                        "': nodeduplicate has been violated!");
+    }
+    break;
+  }
   case Comdat::SelectionKind::ExactMatch:
   case Comdat::SelectionKind::Largest:
   case Comdat::SelectionKind::SameSize: {
     const GlobalVariable *DstGV;
     const GlobalVariable *SrcGV;
     if (getComdatLeader(DstM, ComdatName, DstGV) ||
         getComdatLeader(*SrcM, ComdatName, SrcGV))
       return true;

llvm/test/Linker/comdat-nodeduplicate.ll

 ; RUN: rm -rf %t && split-file %s %t
 ; RUN: not llvm-link -S %t/1.ll %t/1-aux.ll 2>&1 | FileCheck %s
 
 ; CHECK: error: Linking COMDATs named 'foo': nodeduplicate has been violated!
 
-; RUN: not llvm-link -S %t/2.ll %t/2-aux.ll 2>&1 | FileCheck %s --check-prefix=CHECK2
-; RUN: not llvm-link -S %t/2-aux.ll %t/2.ll 2>&1 | FileCheck %s --check-prefix=CHECK2
+; RUN: llvm-link -S %t/2.ll %t/2-aux.ll | FileCheck %s --check-prefix=CHECK2
+; RUN: llvm-link -S %t/2-aux.ll %t/2.ll | FileCheck %s --check-prefix=CHECK2
 
-; CHECK2: error: Linking COMDATs named 'foo'
+; CHECK2-DAG: @foo = global i64 2, section "data", comdat, align 8
+; CHECK2-DAG: @bar = weak global i64 0, section "cnts", comdat($foo)
+; CHECK2-DAG: @qux = weak_odr global i64 4, comdat($foo)
 
 ; RUN: not llvm-link -S %t/non-var.ll %t/non-var.ll 2>&1 | FileCheck %s --check-prefix=NONVAR
 
-; NONVAR: error: Linking COMDATs named 'foo': nodeduplicate has been violated!
+; NONVAR: error: Linking COMDATs named 'foo': GlobalVariable required for data dependent selection!
 
 ;--- 1.ll
 $foo = comdat nodeduplicate
 @foo = global i64 43, comdat($foo)
 
 ;--- 1-aux.ll
 $foo = comdat nodeduplicate
 @foo = global i64 43, comdat($foo)