[ELF] Don't suggest an alternative spelling for a symbol in a discarded section

[MaskRay]

Summary

For undef-not-suggest.yaml, we currently make redundant alternative
spelling suggestions:

ld.lld: error: relocation refers to a discarded section: .text.foo
>>> defined in a.o
>>> section group signature: foo
>>> prevailing definition is in a.o
>>> referenced by a.o:(.rodata+0x0)
>>> did you mean:
>>> defined in: a.o

ld.lld: error: relocation refers to a symbol in a discarded section: foo
>>> defined in a.o
>>> section group signature: foo
>>> prevailing definition is in a.o
>>> referenced by a.o:(.rodata+0x8)
>>> did you mean: for
>>> defined in: a.o

[ruiu]

LGTM

[grimar]

lld/ELF/Relocations.cpp

725

I'd suggest to:

if (auto *file = dyn_cast_or_null<ObjFile<ELFT>>(sym.file))
...

lld/test/ELF/undef-not-suggest.s

1 ↗

(On Diff #234808)

I wonder if we should use yaml2obj (if possible) and make this test be not 'riscv' specific?

[MaskRay]

lld/test/ELF/undef-not-suggest.s

1 ↗

(On Diff #234808)

A yaml2obj test will degrade test coverage.

Symbols:
  - Name:    foo
    Section: .text.foo
  - Name:    for
    Section: .data
  - Name:    .text.foo
    Type:    STT_SECTION
    Section: .text.foo
### This symbol's name is ".data", instead of "" as produced by an assembler.
### We need a symbol with empty name for the `.quad .text.foo` test, to test that an empty name will not be suggested.
  - Name:    .data
    Type:    STT_SECTION
    Section: .data

[MaskRay]

Simplify

[grimar]

lld/test/ELF/undef-not-suggest.s

1 ↗

(On Diff #234808)

I think there is no problem to use an empty name for a symbol.
Am I missing something? The following seems just works.

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_DYN
  Machine: EM_RISCV
Sections:
  - Name: .data
    Type: SHT_PROGBITS
Symbols:
  - Name:    ""
    Type:    STT_SECTION
    Section: .data

[MaskRay]

lld/test/ELF/undef-not-suggest.s

1 ↗

(On Diff #234808)

If we make the name of the STT_SECTION symbol .text.foo empty

- Name:    ""
  Type:    STT_SECTION
  Section: .text.foo
- Name:    ""
  Type:    STT_SECTION
  Section: .data

Then we can't reference it in a relocation:

- Name: .rela.rodata               
  Type: SHT_RELA                   
  Link: .symtab                    
  Info: .rodata                    
  Relocations:                     
    - Offset: 0x0                  
      Symbol: .text.foo            
      Type:   R_X86_64_64          
    - Offset: 0x8                  
      Symbol: foo                  
      Type:   R_X86_64_64

yaml2obj: error: unknown symbol referenced: '.text.foo' by YAML section '.rela.rodata'

[grimar]

lld/test/ELF/undef-not-suggest.s

1 ↗

(On Diff #234808)

But can't you reference it by index?

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_DYN
  Machine: EM_X86_64
Sections:
- Name: .rodata             
  Type: SHT_PROGBITS
- Name: .text.foo               
  Type: SHT_PROGBITS
- Name: .rela.rodata               
  Type: SHT_RELA                   
  Link: .symtab                    
  Info: .rodata                  
  Relocations:                     
    - Offset: 0x0                  
      Symbol: 1            
      Type:   R_X86_64_64
Symbols:
- Name:    ""
  Type:    STT_SECTION
  Section: .text.foo

[MaskRay]

Use yaml2obj.

[grimar]

LGTM with a few nits/suggestions.

Perhaps the test name should be *.test instead of *.yaml. This is consistent with other our tests that use YAML (ELF\invalid\*).
(we have only one *.yaml test, gdb-index-invalid-section-index.yaml, seems it was named inconsistently)

lld/test/ELF/undef-not-suggest.yaml

34 ↗

(On Diff #235074)

"Check we don't suggest .data which has an empty name."
sounds like .data has an empty name, what can't be true.
Perhaps, check we don't suggest the section symbol for ".data", which ... ?

Also, .text.foo . -> ".text.foo". ? (to get rid of trailing space after the section name).

40 ↗

(On Diff #235074)

foo->"foo", for -> "for"? (because "Check we don't suggest for" looks like an incomplete sentence probably).

[MaskRay]

Improve comments

lld/ELF/Relocations.cpp

static bool canSuggestExternCForCXX(StringRef ref, StringRef def) {
   bool ret = ref == buf;
   free(buf);
   return ret;
 }
 
 // Suggest an alternative spelling of an "undefined symbol" diagnostic. Returns
 // the suggested symbol, which is either in the symbol table, or in the same
 // file of sym.
+template <class ELFT>
 static const Symbol *getAlternativeSpelling(const Undefined &sym,
                                             std::string &pre_hint,
                                             std::string &post_hint) {
-  // Build a map of local defined symbols.
   DenseMap<StringRef, const Symbol *> map;
-  if (sym.file && !isa<SharedFile>(sym.file)) {
+  if (auto *file = dyn_cast_or_null<ObjFile<ELFT>>(sym.file)) {
+    // If sym is a symbol defined in a discarded section, maybeReportDiscarded()
+    // will give an error. Don't suggest an alternative spelling.
+    if (file && sym.discardedSecIdx != 0 &&
+        file->getSections()[sym.discardedSecIdx] == &InputSection::discarded)
+      return nullptr;
+
+    // Build a map of local defined symbols.
     for (const Symbol *s : sym.file->getSymbols())
       if (s->isLocal() && s->isDefined())
         map.try_emplace(s->getName(), s);
   }
 
   auto suggest = [&](StringRef newName) -> const Symbol * {
     // If defined locally.
     if (const Symbol *s = map.lookup(newName))
static void reportUndefinedSymbol(const UndefinedDiag &undef,
   }
 
   if (i < undef.locs.size())
     msg += ("\n>>> referenced " + Twine(undef.locs.size() - i) + " more times")
                .str();
 
   if (correctSpelling) {
     std::string pre_hint = ": ", post_hint;
-    if (const Symbol *corrected =
-            getAlternativeSpelling(cast<Undefined>(sym), pre_hint, post_hint)) {
+    if (const Symbol *corrected = getAlternativeSpelling<ELFT>(
+            cast<Undefined>(sym), pre_hint, post_hint)) {
       msg += "\n>>> did you mean" + pre_hint + toString(*corrected) + post_hint;
       if (corrected->file)
         msg += "\n>>> defined in: " + toString(corrected->file);
     }
   }
 
   if (sym.getName().startswith("_ZTV"))
     msg += "\nthe vtable symbol may be undefined because the class is missing "

lld/test/ELF/undef-not-suggest.test

+## Check we don't suggest alternative spelling for relocations to symbols
+## defined in discarded sections.
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: not ld.lld %t.o %t.o -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK-NOT: did you mean:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: .group
+    Type: SHT_GROUP
+    Link: .symtab
+    Info: foo
+    Members:
+      - SectionOrType: GRP_COMDAT
+      - SectionOrType: .text.foo
+  - Name:  .text.foo
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_EXECINSTR, SHF_GROUP ]
+  - Name:  .rodata
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]
+  - Name: .rela.rodata
+    Type: SHT_RELA
+    Link: .symtab
+    Info: .rodata
+    Relocations:
+      ## Relocation which references ".text.foo". Check we don't suggest the
+      ## section symbol for ".data", which has an empty name.
+      - Offset: 0x0
+        ## .text.foo
+        Symbol: 1
+        Type:   R_X86_64_64
+      ## Relocation which references "foo". Check we don't suggest "for".
+      - Offset: 0x8
+        Symbol: foo
+        Type:   R_X86_64_64
+  - Name: .data
+    Type: SHT_PROGBITS
+Symbols:
+  - Name:    ""
+    Type:    STT_SECTION
+    Section: .text.foo
+  - Name:    ""
+    Type:    STT_SECTION
+    Section: .data
+  - Name:    foo
+    Section: .text.foo
+  - Name:    for
+    Section: .data