[PATCH] D53234: Don't remove COMDATs when internalizing global objects

Sat May 22 16:01:20 PDT 2021

MaskRay added subscribers: xur, MaskRay.
MaskRay added a comment.

This change makes sense to me.
PE-COFF's IMAGE_COMDAT_SELECT_ASSOCIATIVE and ELF's section group have the "if one member is retained, other members are retained as well" semantics.

In the current PGO section design, a pair of `__llvm_prf_cnts` and `__llvm_prf_data` make use of the semantics for garbage collection.
Noticed by @xur: if internalizer makes `__llvm_prf_data` internal, we lose the semantics: if the paired `__llvm_prf_cnts` is retained, the `__llvm_prf_data` should be retained as well.
This patch (keeping COMDAT) will bring back the desired semantics.

> This behavior can end up causing issues involving the BFD linker and MD_Associated metadata, since the removed COMDAT allows GlobalDCE to delete the target of the MD_Associated metadata. Keeping the COMDAT causes the MD_Associated-annotated object and its target to be kept or removed as a unit, which is the correct behavior.

This (https://github.com/Aaron1011/llvm_lto_ir/blob/master/bad_ir.ll) is stale with binutils 2.36.
LLD used to have similar issues. LLD now supports sh_link=0 and mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER input sections ( D72904 <https://reviews.llvm.org/D72904> D84001 <https://reviews.llvm.org/D84001>).
GNU ld had made similar changes (https://sourceware.org/bugzilla/show_bug.cgi?id=26256)

BTW: I cannot find a use case where `associated` should be used together with a COMDAT group.
With the COMDAT GC semantics, `associated` is not useful.
When used with functions (instead of variables), `associated` doesn't play well with inlining (D97430 <https://reviews.llvm.org/D97430>).

BTW: `comdat any` is suppored (in 2021) in ELF AsmPrinter, which uses a section group with flag 0.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53234/new/

https://reviews.llvm.org/D53234