[PATCH] D114801: [ELF] Prevent internalizing used comdat symbol

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:1161
+        // Prevent possibly internalizing the symbol.
+        sym->isUsedInRegularObj = true;
+      } else
----------------
peter.smith wrote:
> Presumably we would only need to do this if there were a reference (relocation) from outside the group, i.e. if the caller to foo is getting discarded as well then it doesn't matter. However I don't think it is worth the extra complexity to check for this case.
I thought the same.

The fun fact is that moving the definition of `foo` in `bc.ll` before `declare void @bar()` makes the symbol `foo` at this point `Defined`, not `Lazy`, so `sym->resolve()` will be called instead of `replace()`, and the flag `isUsedInRegularObj` will be set.


================
Comment at: lld/test/ELF/lto/comdat-mixed-archive.test:45
+;; Note. "foo" is defined after declaring "bar" so that loading the regular
+;; object file "obj.o" is triggered before processing the definition of "foo".
+define linkonce_odr void @foo() comdat {
----------------
peter.smith wrote:
> IIUC we have the sequence:
> ```
> start.o refs baz
> bc.o fetched for baz
> Group foo selected from bc.o
> obj.o fetched for bar
> Group foo rejected from obj.o
> foo loses usedInRegularObj
> LTO internalizes foo
> reference to foo from bar is undefined.
> ```
> If that is the case may be worth adding
> ```
> ;; group foo in obj.o is rejected in favor of bc.o but we need to prevent LTO from
> ;; internalizing foo as there is still a reference from outside the group in obj.o. 
Your analysis is accurate. I'll add your suggested comment in the common description of the test because this `Note` is more situational.


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

https://reviews.llvm.org/D114801