[PATCH] D86142: [LLD] Search archives for non-tentative defintions.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

We usually use x86 for generic feature tests (sorry). I am fine with ppc but the test is probably worth a comment that some FORTRAN libraries on ppc require this behavior (i.e. if you no longer require this behavior and if we need some symbol table refactoring where this behavior gets in the way, we should have the right to revisit the decision. SHN_COMMON will get more and more obsoleted and hope we don't need this particular behavior in the future ;-) )

Gold behaves like LLD and internally we have much Fortran code (x86/ppc) which does not tickle this property:)

https://sourceware.org/pipermail/binutils/2020-August/112926.html

> COMMON is also one of those dark corners with multiple implementations, so I'm not at all surprised to learn that some linkers handle it differently. Most code doesn't tickle those cases, so these differences generally go unnoticed.

In any case I have to do a large run internally because we still use -fcommon  :)



================
Comment at: lld/ELF/InputFiles.cpp:1243
+       symtabFile.TheReader.symbols()) {
+    if (sym.getName() == symName)
+      return sym.isGlobal() && !sym.isUndefined() && !sym.isCommon();
----------------
`sym.isGlobal()` should be checked along with the name, because a local symbol should be ignored.


================
Comment at: lld/ELF/InputFiles.cpp:1257
+    Expected<StringRef> name = sym.getName(stringtable);
+    if (name && (name.get() == symName))
+      return sym.isDefined() && !sym.isCommon();
----------------
Nit: parens around == are unneeded


================
Comment at: lld/ELF/InputFiles.cpp:1263
+
+bool isUncommonDef(MemoryBufferRef mb, StringRef symName,
+                   StringRef archiveName) {
----------------
static


================
Comment at: lld/ELF/Symbols.cpp:693
 
+template <class LazyT> void replaceCommon(Symbol &oldSym, const LazyT &newSym) {
+  backwardReferences.erase(&oldSym);
----------------
static


================
Comment at: lld/test/ELF/common-archive-lookup.s:1
+# REQUIRES: ppc
+
----------------
We usually use x86 for generic features (sorry). I am fine with ppc but this is probably worth a comment that some FORTRAN libraries on ppc require this behavior (i.e. if you no longer require this behavior and if we need some symbol table refactoring where this behavior gets in the way, we should have the right to revisit the decision. SHN_COMMON will get more and more obsoleted and hope we don't need this particular behavior in the future ;-) )

Gold behaves like LLD and internally we have much Fortran code (x86/ppc) which does not tickle this property:)

https://sourceware.org/pipermail/binutils/2020-August/112926.html

> COMMON is also one of those dark corners with multiple implementations, so I'm not at all surprised to learn that some linkers handle it differently. Most code doesn't tickle those cases, so these differences generally go unnoticed.

In any case I have to do a large run internally because we still use -fcommon  :)


================
Comment at: lld/test/ELF/common-archive-lookup.s:60
+
+# Ensure we have used the initialized definition of 'block' instead of a common definition.
+# TEST1-LABEL:  Disassembly of section .data:
----------------
For newer tests we use `## ` for non-RUN-non-CHECK comments.


================
Comment at: lld/test/ELF/common-archive-lookup.s:94
+  .type block, at object
+  .comm block,40,8
+
----------------
.comm changes st_type, so `.type` is redundant.


================
Comment at: lld/test/ELF/common-archive-lookup.s:134
+block:
+        .quad   0x400921fb54442eea              # double 3.1415926535900001
+        .space  32
----------------
Don't mix different indentations. 2 may be sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86142