[llvm-objdump] Match GNU objdump on symbol types shown in disassembly output.

[ychen]

Summary

STT_OBJECT and STT_COMMON are dumped as data, not disassembled.

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

[MaskRay]

The test doesn't have a dynamic symbole table (.dynsym) so the source change seems irrelevant.
I think we have the current output without the source change.

See not_func is misaligned, so something goes wrong.

0000000000001001 both_dyn:
    1001: 90                            nop
    1002: 90                            nop

0000000000001003 not_func:
    1003:        90                              .

STT_SECTION exists primarily for relocations are they normally have STB_LOCAL binding.
None of ld.bfd/gold/lld places STB_LOCAL symbols in .dynsym (I am less confident with the statement regarding ld.bfd) so the updated test may not be used in practice.

[jhenderson]

In D62964#1533587, @MaskRay wrote:

The test doesn't have a dynamic symbole table (.dynsym) so the source change seems irrelevant.

@MaskRay, the test DOES have a dynamic symbol table. Line 61 in the updated test starts the DynamicSymbols list, which causes one to be implicitly generated.

I think the code change is mostly just to bring things into line with static symbol dumping, but I could see an argument for deleting the if statement entirely. In that case, I'd just confirm against GNU objdump behaviour to see if it really matters.

[ychen]

Thanks for the review, MaskRay.

See not_func is misaligned, so something goes wrong.

I think it might be better to take care of the formatting issue (dumpELFData emits extra tab) in a separate patch. And this one focuses on which symbols should be included in the disassembly output?

STT_SECTION exists primarily for relocations are they normally have STB_LOCAL binding.
None of ld.bfd/gold/lld places STB_LOCAL symbols in .dynsym (I am less confident with the statement regarding ld.bfd) so the updated test may not be used in practice.

@MaskRay @jhenderson @grimar @rupprecht I'm not sure if this has been discussed before, I think it comes down to whether we want llvm-objdump to be compatible with GNU objdump on:

1. well-formed object files (definitely yes)
2. well-formed object files with something not usually produced by static linker etc.. and not explicitly disallowed by ABI (like STT_SECTION symbol in .dynsym in this case)
3. ill-formed object files (maybe not?)

What's your thought?

deleting the if statement entirely

@jhenderson this seems to assume 1. The patch in its current shape assumes 2?

[jhenderson]

My personal thought is definitely 1), and no need for 3), as long as the error/output makes sense (note that if GNU produces something sensible and doesn't error for 3), we should do the same). I'm in two minds about 2), mostly because we can't be certain that every case we think is unusual applies to every use case out there for downstream users. Ultimately, I'm okay with 2) going either way. I'd plump for matching GNU, but if there's general opinions from others against it, I don't mind.

[ychen]

How about only addressing what PR41947 need (STT_OBJECT and STT_FUNC) in this patch and have a separate patch addressing matching with GNU on what other types of symbols should show up in output? Additionally, a patch for matching GNU on output format for these symbols got shown.

[jhenderson]

In D62964#1540406, @ychen wrote:

How about only addressing what PR41947 need (STT_OBJECT and STT_FUNC) in this patch and have a separate patch addressing matching with GNU on what other types of symbols should show up in output?

PR41947 only used a STT_OBJECT as an example, so we shouldn't just limit this patch to the two types. Note that STT_NOTYPE, as well as symbols with processor etc specific types, can also appear in dynamic symbol tables.

Additionally, a patch for matching GNU on output format for these symbols got shown.

Sorry, I'm not sure I understand what you mean.

[ychen]

• Match GNU objdump on symbol types shown in disassembly output.
• Update test

[jhenderson]

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

112

As noted in D63393, we shouldn't do this and should instead use 0x10 or whatever explicitly, since STT_LOOS etc are just range markers. Also, it's probably only necessary to test one value in each range.

[ychen]

• update test

[jhenderson]

Aside from one test comment, this looks good to me.

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

91

I'm not sure I follow what the purpose of this particular symbol is beyond what func_dyn gives us?

[ychen]

• update

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

91

I think it is not needed. It was in the test file but I'm not sure its purpose so I left it here.

Deleted.

[jhenderson]

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

63

I think you may have gone overboard with the symbol renaming. These first two are deliberately named so that they clearly clash/don't clash with a corresponding static symbol (only_[static|dyn] means only the static/dynamic symbol respectively is present at that address whilst the both_[static|dyn] means both symbols have that value).

86

This value is bad, because the test case was supposed to be for just a static symbol at this address. See the symbol name comments.

91

I don't think you need to rename this symbol from zero_sized.

[ychen]

• update test

[ychen]

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

63

Thank you. Now I see the point.

[ychen]

• update test

[jhenderson]

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

25–27

This looks wrong, and I'm surprised this test isn't failing now. As things stand 0x1002 isn't being checked for at all in dynamic output, but there's no reason it shouldn't be there.

74

You can reduce the diff by making this symbol the STT_FUNC one and the func symbol the STT_NOTYPE one.

[ychen]

• update

[ychen]

• update

[ychen]

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

25–27

both_dyn is STT_OBJECT, dumped as data in one line until the next symbol regardless of its size.

[jhenderson]

LGTM, with the suggested change, but please wait for the other reviewers to confirm too.

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

25–27

Ah, okay. Probably worth making both_dyn the STT_NOTYPE symbol then and making what is currently notype the STT_OBJECT one. That'll be less confusing.

[ychen]

• update

[ychen]

Hi @MaskRay, does this version look good to you?

llvm/test/tools/llvm-objdump/X86/elf-disassemble-dynamic-symbols.test

 ## Show the disassembly behaviour when dynamic symbols are present.
 # RUN: yaml2obj %s -o %t
 
 ## Case 1: Both static and dynamic symbols are present. Only static
 ## symbols are displayed.
 # RUN: llvm-objdump -d %t | FileCheck %s --check-prefixes=CHECK,STATIC
 
 ## Case 2: Only the dynamic symbols are present. These should be displayed, if
 ## they meet the necessary criteria.
 # RUN: llvm-objcopy --strip-all %t %t2
 # RUN: llvm-objdump -d %t2 | FileCheck %s --check-prefixes=CHECK,DYN
 
 # CHECK:       Disassembly of section .text:
 # CHECK-EMPTY:
 # STATIC-NEXT: 0000000000001000 .text:
-# DYN-NEXT:    0000000000001000 only_dyn:
+# DYN-NEXT:    0000000000001000 notype_dyn:
 # CHECK-NEXT:      1000:
 # CHECK-EMPTY:
 # STATIC-NEXT: 0000000000001001 both_static:
-# DYN-NEXT:    0000000000001001 both_dyn:
+# DYN-NEXT:    0000000000001001 object_dyn:
 # CHECK-NEXT:      1001:
-# STATIC-EMPTY:
+# CHECK-EMPTY:
 # STATIC-NEXT: 0000000000001002 only_static:
+# DYN-NEXT:    0000000000001002 func_dyn:
 # CHECK-NEXT:      1002:
+# DYN-EMPTY:
+# DYN-NEXT:    0000000000001003 both_func_dyn:
 # CHECK-NEXT:      1003:
+# DYN-EMPTY:
+# DYN-NEXT:    0000000000001004 zero_sized_func_dyn:
 # CHECK-NEXT:      1004:
-# CHECK-NEXT:      1005:{{.*}}
+# DYN-EMPTY:
+# DYN-NEXT:    0000000000001005 common_dyn:
+# CHECK-NEXT:      1005:
+# DYN-EMPTY:
+# DYN-NEXT:    0000000000001006 loos_dyn:
+# CHECK-NEXT:      1006:
+# DYN-EMPTY:
+# DYN-NEXT:    0000000000001007 loproc_dyn:
+# CHECK-NEXT:      1007:
+# CHECK-NEXT:      1008:
+# CHECK-NEXT:      1009:
+# CHECK-NEXT:      100a:
+# CHECK-NEXT:      100b:{{.*}}
 # CHECK-NOT:   {{.}}
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:
   - Name:    .text
     Type:    SHT_PROGBITS
     Flags:   [SHF_ALLOC, SHF_EXECINSTR]
     Address: 0x1000
-    Content: 909090909090
+    Content: 909090909090909090909090
 ProgramHeaders:
   - Type: PT_LOAD
     VAddr: 0x1000
     Sections:
       - Section: .text
 Symbols:
   - Name:    both_static
     Value:   0x1001
     Section: .text
     Binding: STB_GLOBAL
   - Name:    only_static
     Value:   0x1002
     Section: .text
     Binding: STB_GLOBAL
 DynamicSymbols:
-  - Name:    only_dyn
+  - Name:    notype_dyn
     Value:   0x1000
     Section: .text
     Size:    1
-    Type:    STT_FUNC
+    Type:    STT_NOTYPE
     Binding: STB_GLOBAL
-  - Name:    both_dyn
+  - Name:    object_dyn
     Value:   0x1001
     Section: .text
+    Type:    STT_OBJECT
+    Binding: STB_GLOBAL
+  - Name:    func_dyn
+    Value:   0x1002
+    Section: .text
     Size:    1
     Type:    STT_FUNC
     Binding: STB_GLOBAL
-  ## The rest of the dynamic symbols won't be used for various reasons.
-  ## FIXME: the first two symbols here should be dumped.
-  ##        See https://bugs.llvm.org/show_bug.cgi?id=41947
-  - Name:    not_func
+  - Name:    both_func_dyn
     Value:   0x1003
     Section: .text
-    Type:    STT_OBJECT
+    Size:    1
+    Type:    STT_FUNC
     Binding: STB_GLOBAL
-  - Name:    zero_sized
+  - Name:    zero_sized_func_dyn
     Value:   0x1004
     Section: .text
     Type:    STT_FUNC
     Binding: STB_GLOBAL
-  - Name:    '' # No name
+  - Name:    common_dyn
     Value:   0x1005
     Section: .text
     Size:    1
+    Type:    STT_COMMON
+    Binding: STB_GLOBAL
+  - Name:    loos_dyn
+    Value:   0x1006
+    Section: .text
+    Size:    1
+    Type:    10
+    Binding: STB_GLOBAL
+  - Name:    loproc_dyn
+    Value:   0x1007
+    Section: .text
+    Size:    1
+    Type:    13
+    Binding: STB_GLOBAL
+  ## The rest of the dynamic symbols won't be used for various reasons.
+  - Name:    section_dyn
+    Value:   0x1008
+    Section: .text
+    Size:    1
+    Type:    STT_SECTION
+    Binding: STB_GLOBAL
+  - Name:    '' # No name
+    Value:   0x1009
+    Section: .text
+    Size:    1
     Type:    STT_FUNC
     Binding: STB_GLOBAL
   - Name:    absolute
-    Value:   0x1005
+    Value:   0x100a
     Index:   SHN_ABS
     Size:    1
     Type:    STT_FUNC
     Binding: STB_GLOBAL
   - Name:    undefined
-    Value:   0x1005
+    Value:   0x100b
     Index:   SHN_UNDEF
     Size:    1
     Type:    STT_FUNC
     Binding: STB_GLOBAL

llvm/tools/llvm-objdump/llvm-objdump.cpp

static uint8_t getElfSymbolType(const ObjectFile *Obj, const SymbolRef &Sym) {
   llvm_unreachable("Unsupported binary format");
 }
 
 template <class ELFT> static void
 addDynamicElfSymbols(const ELFObjectFile<ELFT> *Obj,
                      std::map<SectionRef, SectionSymbolsTy> &AllSymbols) {
   for (auto Symbol : Obj->getDynamicSymbolIterators()) {
     uint8_t SymbolType = Symbol.getELFType();
-    if (SymbolType != ELF::STT_FUNC || Symbol.getSize() == 0)
+    if (SymbolType == ELF::STT_SECTION)
       continue;
 
     uint64_t Address = unwrapOrError(Symbol.getAddress(), Obj->getFileName());
+    // ELFSymbolRef::getAddress() returns size instead of value for common
+    // symbols which is not desirable for disassembly output. Overriding.
+    if (SymbolType == ELF::STT_COMMON)
+      Address = Obj->getSymbol(Symbol.getRawDataRefImpl())->st_value;
+
     StringRef Name = unwrapOrError(Symbol.getName(), Obj->getFileName());
     if (Name.empty())
       continue;
 
     section_iterator SecI =
         unwrapOrError(Symbol.getSection(), Obj->getFileName());
     if (SecI == Obj->section_end())
       continue;
#endif
       DisAsm->onSymbolStart(SymbolName, Size, Bytes.slice(Start, End - Start),
                             SectionAddr + Start, DebugOut, CommentStream);
       Start += Size;
 
       Index = Start;
       if (SectionAddr < StartAddress)
         Index = std::max<uint64_t>(Index, StartAddress - SectionAddr);
 
-      // If there is a data symbol inside an ELF text section and we are
+      // If there is a data/common symbol inside an ELF text section and we are
       // only disassembling text (applicable all architectures), we are in a
       // situation where we must print the data and not disassemble it.
-      if (Obj->isELF() && std::get<2>(Symbols[SI]) == ELF::STT_OBJECT &&
-          !DisassembleAll && Section.isText()) {
+      if (Obj->isELF() && !DisassembleAll && Section.isText()) {
+        uint8_t SymTy = std::get<2>(Symbols[SI]);
+        if (SymTy == ELF::STT_OBJECT || SymTy == ELF::STT_COMMON) {
           dumpELFData(SectionAddr, Index, End, Bytes);
           Index = End;
         }
+      }
 
       bool CheckARMELFData = isArmElf(Obj) &&
                              std::get<2>(Symbols[SI]) != ELF::STT_OBJECT &&
                              !DisassembleAll;
       while (Index < End) {
         // AArch64 ELF binaries can interleave data and text in the same
         // section. We rely on the markers introduced to understand what we
         // need to dump. If the data marker is within a function, it is