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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:1232
+  IRSymtabFile symtabFile = check(readIRSymtab(mb));
+  for (unsigned moduleIndex = 0; moduleIndex != symtabFile.Mods.size();
+       ++moduleIndex) {
----------------
range-based for


================
Comment at: lld/ELF/InputFiles.cpp:1236
+         symtabFile.TheReader.module_symbols(moduleIndex)) {
+        if (sym.getName().equals(symName))
+          return sym.isGlobal() && !sym.isUndefined() && !sym.isCommon();
----------------
`if` is misaligned. equals -> `==`


================
Comment at: lld/ELF/InputFiles.cpp:1244
+template <class ELFT>
+bool isUncommonDef(MemoryBufferRef mb, StringRef symName,
+                          StringRef archiveName) {
----------------
static


================
Comment at: lld/ELF/InputFiles.cpp:1251
+    Expected<StringRef> name = sym.getName(stringtable);
+    if (name && name.get().equals(symName))
+      return sym.isDefined() && !sym.isCommon();
----------------
range based for


================
Comment at: lld/ELF/Symbols.cpp:702
+  // should be fetched as the cannonical definition instead.
+  if (isCommon() && isObject()) {
+    if (auto *laSym = dyn_cast<LazyArchive>(&other)) {
----------------
Delete `isObject()`

A common symbol is a subset of an object symbol. (If it doesn't, it is an error)


================
Comment at: lld/test/ELF/Inputs/blockdata.ll:1
+target datalayout = "e-m:e-i64:64-n32:64"
+target triple = "powerpc64le-unknown-linux-gnu"
----------------
Use `RUN: split-file` for short files.

Replace double array with a simple `i32`. 


================
Comment at: lld/test/ELF/Inputs/commonblock.ll:6
+
+define dso_local double @foo(i32 signext %i) local_unnamed_addr {
+entry:
----------------
Keeping one function is sufficient to demonstrate the effects.


================
Comment at: lld/test/ELF/common-archive-lookup.s:66
+.Lgep_foo:
+  addis 2, 12, .TOC.-.Lgep_foo at ha
+  addi  2, 2, .TOC.-.Lgep_foo at l
----------------
The test is much more complex than it needs to be. You just need one reference. All the setup (TOC, addi, .localentry) and teardown (blr) instructions should be deleted not to harm readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86142