This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Dec 19 2019, 4:43 PM.

Details

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

Event Timeline

MaskRay created this revision.Dec 19 2019, 4:43 PM
ruiu accepted this revision.Dec 19 2019, 9:45 PM

LGTM

This revision is now accepted and ready to land.Dec 19 2019, 9:45 PM
grimar added inline comments.Dec 20 2019, 1:21 AM
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
2

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

MaskRay marked 2 inline comments as done.Dec 20 2019, 10:11 AM
MaskRay added inline comments.
lld/test/ELF/undef-not-suggest.s
2

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
grimar added inline comments.Dec 21 2019, 1:21 AM
lld/test/ELF/undef-not-suggest.s
2

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 marked an inline comment as done.Dec 21 2019, 9:27 AM
MaskRay added inline comments.
lld/test/ELF/undef-not-suggest.s
2

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 added inline comments.Dec 22 2019, 11:38 AM
lld/test/ELF/undef-not-suggest.s
2

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 updated this revision to Diff 235074.Dec 22 2019, 11:50 AM
MaskRay edited the summary of this revision. (Show Details)

Use yaml2obj.

grimar accepted this revision.Dec 23 2019, 12:56 AM

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 updated this revision to Diff 235151.Dec 23 2019, 9:07 AM
MaskRay marked 2 inline comments as done.

Improve comments

This revision was automatically updated to reflect the committed changes.