Fix Bug 41353 - unique symbols printed as D instead of u

[mmpozulp]

Summary

https://bugs.llvm.org/show_bug.cgi?id=41353

I'm new to LLVM and C++ so please do not hesitate to iterate with me on this fix.

[mmpozulp]

Protect dyn_cast with isELF() check

[rupprecht]

Adding zbrid since she has context already. I'll take a look in a bit too. Thanks for the patch!

[rupprecht]

(Also sorry for the delay; I was out last week and am doing a very slow job of un-burying myself from what's piled up)

[rupprecht]

Looks like support was recently added in rL359380, though without any tests, and was accidentally reverted.

The approach there (checking SymI->getOther() >> 4 == ELF::STB_GNU_UNIQUE) is a bit opaque, but I think with a comment might look better than the dyn_cast chain. Can you see if that works?

Also +grimar who has been adding GNU_UNIQUE support elsewhere

llvm/test/tools/llvm-nm/X86/Inputs/unique.s

1 ↗

(On Diff #196713)

For simple cases like this, you can embed the assembly directly into the test file. See llvm-nm/X86/radix.s for an example test that does this.

llvm/test/tools/llvm-nm/X86/unique.test

1 ↗

(On Diff #196713)

Instead of clang, use llvm-mc -filetype=obj -triple=x86_64-pc-linux to directly handle the assembly

Actually, yaml2obj might be even better, now that it can handle unique symbols.

[grimar]

In D61117#1488853, @rupprecht wrote:

Looks like support was recently added in rL359380, though without any tests, and was accidentally reverted.

I think it just had a bug. Binding is st_info >> 4, not st_other >> 4:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/BinaryFormat/ELF.h#L995

[mmpozulp]

Incorporate feedback from @rupprecht

[mmpozulp]

Delete unnecessary modification to ELFTypes.h

[grimar]

llvm/test/tools/llvm-nm/X86/unique.test

1 ↗

(On Diff #198353)

I would write in plain English what this test checks (instead of adding a link).
This is s more common way to describe what is going on in test and much more convenient for readers.

1 ↗

(On Diff #196713)

+1 to use yaml2obj.

llvm/tools/llvm-nm/llvm-nm.cpp

932 ↗

(On Diff #198353)

Since getSymbolNMTypeChar starts from comment "// OK, this is ELF",
and has ELFObjectFileBase argument (i.e. it is called only for ELF)
I suggest to simplify and inline this method instead.
Something like:

case (ELF::SHF_TLS | ELF::SHF_ALLOC | ELF::SHF_WRITE):
case (ELF::SHF_ALLOC | ELF::SHF_WRITE):
  return ELFSymbolRef(Sym).getBinding() == ELF::STB_GNU_UNIQUE ? 'u' : 'd';
...
if ((Symflags & object::SymbolRef::SF_Global) && 
  ELFSymbolRef(Sym).getBinding() != ELF::STB_GNU_UNIQUE)
    Ret = toupper(Ret);

[mmpozulp]

Inline the check for gnu unique symbol as per suggestion by @grimar. Use yaml2obj instead of llvm-mc in unique.test.

[grimar]

llvm/test/tools/llvm-nm/X86/unique.test

13 ↗

(On Diff #198526)

The header you wrote looks nice for me personally, but the problem that it is inconsistent
with other tests (I do not think we usually write how the llvm-mc was called for the very trivial cases like this).

Also, the problem is that your YAML has more sections than needed. (And removing the sections would need to
update your comment about how the YAML was produced, what is probably does not make value here since it
is a very simple case which just does not need llvm-mc I think).

So I think you can just use:

## Check that we print 'u' for unique symbols
# RUN: yaml2obj < %s | llvm-nm - | FileCheck %s

--- !ELF
FileHeader:      
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_REL
  Machine:      EM_X86_64
Sections:        
  - Name:            .data
    Type:            SHT_PROGBITS
    Flags:           [ SHF_WRITE, SHF_ALLOC ]
    AddressAlign:    0x0000000000000001
    Content:         '00'
Symbols:         
  - Name:            foo
    Type:            STT_OBJECT
    Section:         .data
    Binding:         STB_GNU_UNIQUE
...

# CHECK: 0000000000000000 u foo

Or even may be:

## Check that we print 'u' for unique symbols
# RUN: yaml2obj < %s | llvm-nm - | FileCheck %s

--- !ELF
FileHeader:      
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_REL
  Machine:      EM_X86_64
Symbols:         
  - Name:          foo
    Binding:        STB_GNU_UNIQUE
...

# CHECK: 0000000000000000 u foo

[grimar]

llvm/tools/llvm-nm/llvm-nm.cpp

1177 ↗

(On Diff #198526)

A bit hard to read these conditions.

What do you think about the following?

if (!(Symflags & object::SymbolRef::SF_Global))
    return Ret;

if (Obj.isELF() && ELFSymbolRef(*I).getBinding() == ELF::STB_GNU_UNIQUE)
    return Ret;

return toupper(Ret);

[mmpozulp]

Incorporate feedback by @grimar

[mmpozulp]

llvm/test/tools/llvm-nm/X86/unique.test

13 ↗

(On Diff #198526)

Good suggestion! The smallest yaml file that I could achieve is

--- !ELF
FileHeader:      
  Class:           ELFCLASS64
  Data:            ELFDATA2LSB
  Type:            ET_REL
  Machine:         EM_X86_64
Sections:        
  - Name:            .data
    Type:            SHT_PROGBITS
    Flags:           [ SHF_WRITE, SHF_ALLOC ]
Symbols:         
  - Name:            foo
    Section:         .data
    Binding:         STB_GNU_UNIQUE

If I delete the .data section I get 'U' since the symbol is now undefined. I also noticed that if I delete

Flags:           [ SHF_WRITE, SHF_ALLOC ]

nm reports 'u' but llvm-nm reports 'n'. If I also delete

Binding:         STB_GNU_UNIQUE

nm reports 'd' but llvm-nm reports 'n'. I assume that these situations don't arise in reality so we don't care.

[MaskRay]

llvm/tools/llvm-nm/llvm-nm.cpp

946 ↗

(On Diff #199317)

You have to rebase. This switch has been deleted

[jhenderson]

llvm/test/tools/llvm-nm/X86/unique.test

6 ↗

(On Diff #199317)

Nit: there's no need to add large numbers of spaces between the tag and value in the YAML. Just add enough so that all values in a block line up:

Class:   ELFCLASS64
Data:    ELFDATA2LSB
Type:    ET_REL
Machine: EM_X86_64

[mmpozulp]

Rebase and fix yaml spaces. Thanks @MaskRay and @jhenderson :)

[jhenderson]

llvm/test/tools/llvm-nm/X86/unique.test

1 ↗

(On Diff #199527)

Nit: missing trailing full stop.

13 ↗

(On Diff #199527)

If the flags aren't relevant to the resolution of the symbol, I suggest getting rid of them entirely.

llvm/tools/llvm-nm/llvm-nm.cpp

911–912 ↗

(On Diff #199527)

I don't know how realistic this is, but is this the right order of precedence for unique symbols, compared with GNU nm? In other words, do executable and NOBITS sections containing these symbols result in 't'/'b'?

[mmpozulp]

Incorporate feedback by @jhenderson.

[mmpozulp]

llvm/tools/llvm-nm/llvm-nm.cpp

911–912 ↗

(On Diff #199527)

STB_GNU_UNIQUE precedence is high. I tested 5 different cases with GNU nm version 2.27-34.base.el7 to make sure llvm-nm agrees (see below), and added those tests to this patch. What do you think, @jhenderson?

--- !ELF
FileHeader:    
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
Sections:    
  - Name:  .data
    Type:  XXX
Symbols:    
  - Name:    foo 
    Section: .data
    Binding: STB_GNU_UNIQUE

XXX             nm  llvm-nm
SHT_PROGBITS    u   u
SHT_NOBITS      u   u

--- !ELF
FileHeader:    
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
Sections:    
  - Name:  .data
    Type:  SHT_PROGBITS
    Flags: [YYY]
Symbols:    
  - Name:    foo 
    Section: .data
    Binding: STB_GNU_UNIQUE

YYY                     nm  llvm-nm
SHF_EXECINSTR           u   u
SHF_ALLOC               u   u
SHF_ALLOC, SHF_WRITE    u   u

[jhenderson]

I think you can afford to have all the test cases for unique symbols in one test file. I'd also name the sections and symbols to illustrate the cases, rather than calling them all .data/foo.

[mmpozulp]

Squish tests together into one file and use more descriptive section/symbol names. Thanks @jhenderson :)

[MaskRay]

llvm/tools/llvm-nm/llvm-nm.cpp

911–912 ↗

(On Diff #199527)

You may move the 'u' check before if (SecI != Obj.section_end()) {

See
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=9d6d4be89d12747a92629ed1bde1d423e2831de1;f=bfd/syms.c#l699

The if (SecI != Obj.section_end()) { blocks roughly corresponds to decode_section_type (symbol->section);.

[MaskRay]

BTW, I think STB_GNU_UNIQUE is a misfeature (no clear specification, only (complex) implementation is in glibc, interaction with STB_WEAK/STB_GLOBAL is unclear..).. this feature will highlight the problem :)

[mmpozulp]

In D61117#1507698, @MaskRay wrote:

BTW, I think STB_GNU_UNIQUE is a misfeature (no clear specification, only (complex) implementation is in glibc, interaction with STB_WEAK/STB_GLOBAL is unclear..).. this feature will highlight the problem :)

@MaskRay, it sounds like you know a lot more than I do about this feature :). Do you know what purpose is served by "unique" global symbols? man nm shows

"u" The symbol is a unique global symbol.  This is a GNU extension to the
    standard set of ELF symbol bindings.  For such a symbol the dynamic
    linker will make sure that in the entire process there is just one
    symbol with this name and type in use.

but without an example I don't understand why this feature is useful. If you, @MaskRay, don't know of an example, then @rupprecht, who asked for this feature, or other knowledgeable people like @grimar and @jhenderson may know of one. Thanks :)

[jhenderson]

I'd like @MaskRay's comment to be addressed, but otherwise, this looks good to me.

[mmpozulp]

Move the 'u' check before if (SecI != Obj.section_end()), thanks @MaskRay.

[jhenderson]

llvm/test/tools/llvm-nm/X86/unique.test

39 ↗

(On Diff #200402)

Could you add a unique symbol without a section to show it gets 'u', please?

[mmpozulp]

llvm/test/tools/llvm-nm/X86/unique.test

39 ↗

(On Diff #200402)

Is that the behavior that we want for llvm-nm? If I run GNU nm (version 2.27-34.base.el7) it reports 'U' instead of 'u' for a unique symbol without a section

Symbols:
  - Name:    nosection
    Binding: STB_GNU_UNIQUE

U nosection

[grimar]

llvm/test/tools/llvm-nm/X86/unique.test

39 ↗

(On Diff #200402)

I can confirm. Symbol is undefined, but unique and GNU nm shows 'U' in this case:

Symbol table '.symtab' contains 2 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 NOTYPE  UNIQUE DEFAULT  UND foo

[jhenderson]

llvm/test/tools/llvm-nm/X86/unique.test

39 ↗

(On Diff #200402)

Right, sorry. I based my comment on reading the change below (and I might have misread it).

I think we should have a test case for whatever the behaviour is.

[rupprecht]

LGTM, pending the additional test case James requested

[mmpozulp]

Update test to make sure that we print 'U' for a unique symbol without a section. Thanks, @jhenderson, for suggesting this.

[rupprecht]

llvm/test/tools/llvm-nm/X86/unique.test

3 ↗

(On Diff #200823)

nit: < is not necessary for yaml2obj. Just yaml2obj %s is fine (and more common).

[mmpozulp]

Use yaml2obj %s instead of yaml2obj < %s in test, thanks @rupprecht.

[jhenderson]

LGTM. Do you need help committing this?

[mmpozulp]

In D61117#1513247, @jhenderson wrote:

LGTM. Do you need help committing this?

Yes, please. Thanks!

[rupprecht]

Committed as r361595. Thanks!

llvm/trunk/include/llvm/Object/ELFObjectFile.h

class ELFObjectFileBase : public ObjectFile {
   friend class ELFSectionRef;
   friend class ELFSymbolRef;
 
 protected:
   ELFObjectFileBase(unsigned int Type, MemoryBufferRef Source);
 
   virtual uint16_t getEMachine() const = 0;
   virtual uint64_t getSymbolSize(DataRefImpl Symb) const = 0;
+  virtual uint8_t getSymbolBinding(DataRefImpl Symb) const = 0;
   virtual uint8_t getSymbolOther(DataRefImpl Symb) const = 0;
   virtual uint8_t getSymbolELFType(DataRefImpl Symb) const = 0;
 
   virtual uint32_t getSectionType(DataRefImpl Sec) const = 0;
   virtual uint64_t getSectionFlags(DataRefImpl Sec) const = 0;
   virtual uint64_t getSectionOffset(DataRefImpl Sec) const = 0;
 
   virtual Expected<int64_t> getRelocationAddend(DataRefImpl Rel) const = 0;
public:
   const ELFObjectFileBase *getObject() const {
     return cast<ELFObjectFileBase>(BasicSymbolRef::getObject());
   }
 
   uint64_t getSize() const {
     return getObject()->getSymbolSize(getRawDataRefImpl());
   }
 
+  uint8_t getBinding() const {
+    return getObject()->getSymbolBinding(getRawDataRefImpl());
+  }
+
   uint8_t getOther() const {
     return getObject()->getSymbolOther(getRawDataRefImpl());
   }
 
   uint8_t getELFType() const {
     return getObject()->getSymbolELFType(getRawDataRefImpl());
   }
 
protected:
 
   void moveSymbolNext(DataRefImpl &Symb) const override;
   Expected<StringRef> getSymbolName(DataRefImpl Symb) const override;
   Expected<uint64_t> getSymbolAddress(DataRefImpl Symb) const override;
   uint64_t getSymbolValueImpl(DataRefImpl Symb) const override;
   uint32_t getSymbolAlignment(DataRefImpl Symb) const override;
   uint64_t getCommonSymbolSizeImpl(DataRefImpl Symb) const override;
   uint32_t getSymbolFlags(DataRefImpl Symb) const override;
+  uint8_t getSymbolBinding(DataRefImpl Symb) const override;
   uint8_t getSymbolOther(DataRefImpl Symb) const override;
   uint8_t getSymbolELFType(DataRefImpl Symb) const override;
   Expected<SymbolRef::Type> getSymbolType(DataRefImpl Symb) const override;
   Expected<section_iterator> getSymbolSection(const Elf_Sym *Symb,
                                               const Elf_Shdr *SymTab) const;
   Expected<section_iterator> getSymbolSection(DataRefImpl Symb) const override;
 
   void moveSectionNext(DataRefImpl &Sec) const override;

 }
 
 template <class ELFT>
 uint64_t ELFObjectFile<ELFT>::getCommonSymbolSizeImpl(DataRefImpl Symb) const {
   return getSymbol(Symb)->st_size;
 }
 
 template <class ELFT>
+uint8_t ELFObjectFile<ELFT>::getSymbolBinding(DataRefImpl Symb) const {
+  return getSymbol(Symb)->getBinding();
+}
+
+template <class ELFT>
 uint8_t ELFObjectFile<ELFT>::getSymbolOther(DataRefImpl Symb) const {
   return getSymbol(Symb)->st_other;
 }
 
 template <class ELFT>
 uint8_t ELFObjectFile<ELFT>::getSymbolELFType(DataRefImpl Symb) const {
   return getSymbol(Symb)->getType();
 }

llvm/trunk/test/tools/llvm-nm/X86/unique.test

+## Check that we print 'u' for unique symbols
+## and 'U' for a unique symbol without a section.
+# RUN: yaml2obj %s | llvm-nm - | FileCheck %s
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:  .nobits
+    Type:  SHT_NOBITS
+  - Name:  .progbits
+    Type:  SHT_PROGBITS
+  - Name:  .progbits_alloc
+    Type:  SHT_PROGBITS
+    Flags: [SHF_ALLOC]
+  - Name:  .progbits_alloc_write
+    Type:  SHT_PROGBITS
+    Flags: [SHF_ALLOC, SHF_WRITE]
+  - Name:  .progbits_execinstr
+    Type:  SHT_PROGBITS
+    Flags: [SHF_EXECINSTR]
+Symbols:
+  - Name:    nosection
+    Binding: STB_GNU_UNIQUE
+  - Name:    nobits
+    Section: .nobits
+    Binding: STB_GNU_UNIQUE
+  - Name:    progbits
+    Section: .progbits
+    Binding: STB_GNU_UNIQUE
+  - Name:    progbits_alloc
+    Section: .progbits_alloc
+    Binding: STB_GNU_UNIQUE
+  - Name:    progbits_alloc_write
+    Section: .progbits_alloc_write
+    Binding: STB_GNU_UNIQUE
+  - Name:    progbits_execinstr
+    Section: .progbits_execinstr
+    Binding: STB_GNU_UNIQUE
+...
+
+# CHECK: 0000000000000000 u nobits
+# CHECK:                  U nosection
+# CHECK: 0000000000000000 u progbits
+# CHECK: 0000000000000000 u progbits_alloc
+# CHECK: 0000000000000000 u progbits_alloc_write
+# CHECK: 0000000000000000 u progbits_execinstr

llvm/trunk/tools/llvm-nm/llvm-nm.cpp

static char getSymbolNMTypeChar(ELFObjectFileBase &Obj,
   elf_symbol_iterator SymI(I);
 
   Expected<elf_section_iterator> SecIOrErr = SymI->getSection();
   if (!SecIOrErr) {
     consumeError(SecIOrErr.takeError());
     return '?';
   }
 
+  if (SymI->getBinding() == ELF::STB_GNU_UNIQUE)
+    return 'u';
+
   elf_section_iterator SecI = *SecIOrErr;
   if (SecI != Obj.section_end()) {
     uint32_t Type = SecI->getType();
     uint64_t Flags = SecI->getFlags();
     if (Flags & ELF::SHF_EXECINSTR)
       return 't';
     if (Type == ELF::SHT_NOBITS)
       return 'b';
else if (COFFImportFile *COFFImport = dyn_cast<COFFImportFile>(&Obj))
     Ret = getSymbolNMTypeChar(*COFFImport);
   else if (MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj))
     Ret = getSymbolNMTypeChar(*MachO, I);
   else if (WasmObjectFile *Wasm = dyn_cast<WasmObjectFile>(&Obj))
     Ret = getSymbolNMTypeChar(*Wasm, I);
   else
     Ret = getSymbolNMTypeChar(cast<ELFObjectFileBase>(Obj), I);
 
-  if (Symflags & object::SymbolRef::SF_Global)
-    Ret = toupper(Ret);
+  if (!(Symflags & object::SymbolRef::SF_Global))
+    return Ret;
 
+  if (Obj.isELF() && ELFSymbolRef(*I).getBinding() == ELF::STB_GNU_UNIQUE)
     return Ret;
+
+  return toupper(Ret);
 }
 
 // getNsectForSegSect() is used to implement the Mach-O "-s segname sectname"
 // option to dump only those symbols from that section in a Mach-O file.
 // It is called once for each Mach-O file from dumpSymbolNamesFromObject()
 // to get the section number for that named section from the command line
 // arguments. It returns the section number for that section in the Mach-O
 // file or zero it is not present.