[PATCH] D120626: [ELF] Move section assignment from initializeSymbols to postParse

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D120626#3352236 <https://reviews.llvm.org/D120626#3352236>, @peter.smith wrote:

> Not supporting `comdat-binding: %t/w.o %t/g.o leads to an undesired undefined symbol. This is not ideal but acceptable because valid input cannot form this case.` makes me a bit nervous. In a clang/gcc world this is not a problem, but there may be third party compilers that differ. For example arm's legacy compiler armcc from Arm Compiler 5 uses STB_GLOBAL for definitions in COMDAT groups, whereas GCC, at least used to, uses STB_WEAK. It is possible that this is illegal, but I can't find in the spec where it says that.
>
> Perhaps it could be accounted for in resolve as I think we'd be passing a global with the discarded section. Perhaps we could prefer the existing symbol in that case?

Avoiding accessing `sections[*]` is what the patch strives to do: untangle section initialization and symbol initialization/resolution as much as possible.
I have a plan to merge section initialization and `initializeLocalSymbols` into `initSectionsAndLocalSyms`, which parallelizes section initialization.
In the long term, this is a necessary step to achieve parallel input file parsing (https://discourse.llvm.org/t/parallel-input-file-parsing/60164) (and perhaps important to keep lld relevant with the emerging of mold.)

Keeping `if (sec == &InputSection::discarded) ` will address the `%t/w.o %t/g.o` dilemma, but that probably will not fly.
Personally I'd like to say such a dilemma is unsupported. If we have to, it doesn't seem like there is a good way supporting it...



================
Comment at: lld/ELF/InputFiles.cpp:1162
+      if (sym.traced) {
+        printTraceSymbol(Undefined{this, sym.getName(), sym.binding,
+                                   sym.stOther, sym.type, secIdx},
----------------
peter.smith wrote:
> If this is happening in parallel, is there a chance of interleaved output? For example a traced symbol in multiple groups? I guess that would need something like driver->duplicates?
> 
Yes. If the determinism is desired, `elf::printTraceSymbol(` needs to some refactoring: the `message` call needs to be replaced with something like `driver->duplicates`.

Placing `duplicates` in `LinkerDriver` is a bit arbitrary. I wonder whether we should use `ctx->duplicates`.


================
Comment at: lld/ELF/Symbols.cpp:543
     return;
-  const Defined *d = cast<Defined>(&sym);
+  // With concurrency, sym might be Defined in postParse but later got changed
+  // to Undefined.
----------------
peter.smith wrote:
> I'm trying to think how this can happen? Would this be a case where there is a definition of symbol `S` outside of a group and then an existing weak `S` from prevailing group and a global `S` from a discarded group.
> 
> May be worth expanding on the case in the comment as it isn't easy (or at least I wasn't able to easily work it out) from the code.
Updated the comment (the original one was a bit wrong). It's related to test/ELF/comdat-linkonce.s


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120626