[PATCH] D80765: [ELF] Handle bitcode comdat groups separately to deduplicate thinlto comdat sections

[Christy Lee via Phabricator via llvm-commits]

christylee marked 2 inline comments as done and an inline comment as not done.
christylee added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:611
+      // Else, check all comdat groups.
+      bool isNew = isLTOOutput
+                       ? symtab->ltoOutputComdatGroups
----------------
christylee wrote:
> MaskRay wrote:
> > christylee wrote:
> > > @MaskRay 
> > > >  Since .debug_types (notably, a non-SHF_ALLOC section) is the only COMDAT rule this patch will discard, how about special casing .debug_types (i.e. if isLTOOutput && the group is related to .debug_types)?
> > > 
> > > At the time we run this check, not all InputSections have been initialized, so a the group might be related to an uninitialized section.  Since it might be uninitialized, what's the best way to check if that section is non-SHF_ALLOC?
> > This is difficult. An alternative is to use ltoOutputComdatGroups but assert that no collision happens
> > 
> > ```
> > if (isLTOOutput) {
> >   CachedHashStringRef ref(signature);
> >   isNew = symtab->ltoOutputComdatGroups.try_emplace(ref, this).second);
> >   if (symtab->comdatGroups.coumt(ref))
> >     errorOrWarn(toString(this) + ": LTO output should not emit a section group whose signature collides with a non-LTO group signature");
> > }
> > ```
> When we link something with thinlto, we first process the comdat groups when they are in bitcode form.  We add that to `symtab->comdatGroups` since we want `symtab->comdatGroups` to contain all possible comdatGroups.  After compiling the bitcode to object files, we process comdat groups again.  At this point, if they are lto output, then we need to check whether we've seen it from other lto outputs.  In other words, if something is in `symtab->ltoOutputComdatGroups`, then it must also be in `symtab->comdatGroups`, but we need the two distinct maps to ensure there are no duplicate symbols.
@MaskRay , can we ship this as is?


================
Comment at: lld/test/ELF/lto/debug-types-deduplication.ll:37
+!5 = !DISubroutineType(types: !{!6})
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = !DILocalVariable(name: "f1", scope: !4, file: !1,type: !8)
----------------
MaskRay wrote:
> Can you drop some unneeded metadata nodes here? 
I've tried dropping the metadata but this is as small as I can get it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80765