[PATCH] D59649: [ELF] Improve error message for relocations to symbols defined in discarded sections

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Driver.cpp:1296
         replaceSymbol<Undefined>(S, nullptr, S->getName(), STB_WEAK, S->StOther,
-                                 S->Type);
+                                 S->Type, 0);
         S->Used = Used;
----------------
I would use something like `0 /*DiscardedSecIdx*/` because it is not clear what is `0`.
(here and below)


================
Comment at: ELF/Relocations.cpp:682
+  Elf_Shdr_Impl<ELFT> ELFSec = ObjSections[Sym.DiscardedSecIdx - 1];
+  if (ELFSec.sh_type == SHT_GROUP) {
+    StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
----------------
Early return?


================
Comment at: ELF/Relocations.cpp:684
+    StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
+    auto It = Symtab->ComdatGroups.find(CachedHashStringRef(Signature));
+    if (It != Symtab->ComdatGroups.end())
----------------
Use `lookup`?


================
Comment at: ELF/Relocations.cpp:691
+
+  return Msg;
+}
----------------
I.e. this code seems a bit better looking as:

```
  // If the discarded section is a COMDAT.
  Elf_Shdr_Impl<ELFT> ELFSec = ObjSections[Sym.DiscardedSecIdx - 1];
  if (ELFSec.sh_type != SHT_GROUP)
    return Msg;

  StringRef Signature = File->getShtGroupSignature(ObjSections, ELFSec);
  if (const InputFile *InFile =
          Symtab->ComdatGroups.lookup(CachedHashStringRef(Signature)))
    Msg += "\n>>> section group signature: " + Signature.str() +
           "\n>>> prevailing definition is in " + toString(InFile);
  return Msg;
```


================
Comment at: ELF/Relocations.cpp:707
 
-  std::string Msg = "undefined ";
-  if (Sym.Visibility == STV_INTERNAL)
-    Msg += "internal ";
-  else if (Sym.Visibility == STV_HIDDEN)
-    Msg += "hidden ";
-  else if (Sym.Visibility == STV_PROTECTED)
-    Msg += "protected ";
-  Msg += "symbol: " + toString(Sym) + "\n>>> referenced by ";
+  auto Visibility = [&]() -> StringRef {
+    switch (Sym.Visibility) {
----------------
If you'll do `-> std::string` here, then ...


================
Comment at: ELF/Relocations.cpp:723
+  if (Msg.empty())
+    Msg = ("undefined " + Visibility() + "symbol: " + toString(Sym)).str();
+
----------------
... you can remove `.str()` here.


================
Comment at: test/ELF/exclude-discarded-error2.s:9
+# in %t.o
+# CHECK-NOT: discarded section
+
----------------
What about testing the full error message explicitly here?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D59649