Page MenuHomePhabricator

[Internalize] Rename instead of removal if a to-be-internalized comdat has more than one member
ClosedPublic

Authored by MaskRay on May 24 2021, 1:17 PM.

Details

Summary

Beside the comdat any deduplication feature, instrumentations use comdat to
establish dependencies among a group of sections, to prevent section based
linker garbage collection from discarding some members without discarding all.
LangRef acknowledges this usage with the following wording:

All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key.

On ELF, for PGO instrumentation, a __llvm_prf_cnts section and its associated
__llvm_prf_data section are placed in the same GRP_COMDAT group. A
__llvm_prf_data is usually not referenced and expects the liveness of its
associated __llvm_prf_cnts to retain it.

The setComdat(nullptr) code (added by D10679) in InternalizePass can break the
use case (a __llvm_prf_data may be dropped with its associated __llvm_prf_cnts retained).
The main goal of this patch is to fix the dependency relationship.

I think it makes sense for InternalizePass to internalize a comdat and thus
suppress the deduplication feature, e.g. a relocatable link of a regular LTO can
create an object file affected by InternalizePass.
If a non-internal comdat in a.o is prevailed by an internal comdat in b.o, the a.o references to the comdat definitions will be non-resolvable (references cannot bind to STB_LOCAL definitions in b.o)

hide deduplication so that the result object file cannot collide with a
subsequent link with a comdat with the same signature but non-internal.
The approach adopted by this patch tries to keep this semantics working.

On PE-COFF, for a non-external selection symbol, deduplication is naturally
suppressed with link.exe and lld-link. However, this is fuzzy on ELF and I tend
to believe the spec creator has not thought about this use case (see D102973).

GNU ld and gold are still using the "signature is name based" interpretation.
So even if D102973 for ld.lld is accepted, for portability, a better approach is
to rename the comdat. A comdat with one single member is the common case,
leaving the comdat can waste (sizeof(Elf64_Shdr)+4*2) bytes, so we optimize by
deleting the comdat; otherwise we rename the comdat.

Diff Detail

Event Timeline

MaskRay created this revision.May 24 2021, 1:17 PM
MaskRay requested review of this revision.May 24 2021, 1:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2021, 1:17 PM
MaskRay added a reviewer: xur.May 24 2021, 1:17 PM
MaskRay edited the summary of this revision. (Show Details)May 24 2021, 1:19 PM

This seems like a reasonable approach to me. Couple minor comments below. Hoping others can take a look in case I am missing anything.

llvm/include/llvm/Transforms/IPO/Internalize.h
37

Struct fields could use some descriptive comments.

llvm/test/Transforms/Internalize/comdat-empty-moduleid.ll
5

Presumably this doesn't hurt in practice because all of the internal symbols get removed during optimization?

MaskRay updated this revision to Diff 347493.May 24 2021, 2:04 PM
MaskRay marked 2 inline comments as done.

Add comments

llvm/test/Transforms/Internalize/comdat-empty-moduleid.ll
5

Confirmed that GlobalOpt will discard all the internal symbols.

opt -internalize a.ll | opt -O2 -S

xur added a comment.May 24 2021, 2:45 PM

I can understand the single member comdat part -- it should be safe to internalize it without break anything. I'm not sure I follow the rename part. Why can't the linker handle the duplicated comdat? Is deduplication should be done by linker?

I can understand the single member comdat part -- it should be safe to internalize it without break anything. I'm not sure I follow the rename part. Why can't the linker handle the duplicated comdat? Is deduplication should be done by linker?

On PE-COFF, a comdat with a non-external selection symbol does not deduplicate.

On ELF, a comdat with a STB_LOCAL signature symbol still deduplicates with all of ld.lld/GNU ld/gold. If a non-internal comdat in a.o is prevailed by an internal comdat in b.o, the a.o references to the comdat definitions will be un-resolvable (references cannot bind to STB_LOCAL definitions in b.o)

MaskRay edited the summary of this revision. (Show Details)May 24 2021, 4:32 PM
This revision is now accepted and ready to land.May 25 2021, 10:55 AM

Thanks! I'll push this in the afternoon if I have seen no objection.

This seems to be causing some build breakage for us when building with thinLTO+PGO: https://github.com/ClangBuiltLinux/linux/issues/1388.

ld.lld: error: drivers/gpu/drm/amd/amdgpu/amdgpu.lto.o: SHT_SYMTAB_SHNDX has 79582 entries, but the symbol table associated has 79583

Should "the symbol table associated" also have some renaming done, rather than removal?

This seems to be causing some build breakage for us when building with thinLTO+PGO: https://github.com/ClangBuiltLinux/linux/issues/1388.

ld.lld: error: drivers/gpu/drm/amd/amdgpu/amdgpu.lto.o: SHT_SYMTAB_SHNDX has 79582 entries, but the symbol table associated has 79583

Should "the symbol table associated" also have some renaming done, rather than removal?

The diagnostic is about size(.symtab) and size(.symtab_shndx) mismatch, unrelated to what the patch did.

I've commented on https://github.com/ClangBuiltLinux/linux/issues/1388