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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Looks good to me, just some suggestions on the comments.



================
Comment at: lld/ELF/InputFiles.cpp:1160
         sym->replace(und);
-      else
+        // Prevent possibly internalizing the symbol.
+        sym->isUsedInRegularObj = true;
----------------
Would be good to include LTO in the comment to give some context. For example:
```
// Prevent LTO from internalizing the symbol in case there is a reference to this symbol from this file.
```


================
Comment at: lld/ELF/InputFiles.cpp:1161
+        // Prevent possibly internalizing the symbol.
+        sym->isUsedInRegularObj = true;
+      } else
----------------
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.


================
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 {
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114801