Print reasonable representations of type names in llvm-nm, readelf and readobj.

[Sunil_Srivastava]

Summary

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

For type values that do not have proper names, print reasonable representation in llvm-nm, llvm-readobj and llvm-readelf, matching GNU tools.

[Sunil_Srivastava]

This patch makes no executable code changes, just the ElfSymbolTypes table.

[jhenderson]

I've added a few more reviewers who I know have been working on the GNU binutils recently.

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

When looking at this offline, I missed this. It isn't great that we have IFUNC printed here, because not all ABIs support such symbol types and it actually makes us less compatible with GNU nm. I think we should change the table to print "<OS specific>: 10" here, and put a (hopefully) temporary patch in llvm-readobj to convert that particular case to IFUNC, pending the resolution of https://bugs.llvm.org/show_bug.cgi?id=42686.

[jhenderson]

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

Sorry, got mixed up. This comment should probably be on the readelf test, not the llvm-nm test. Please double-check versus GNU readelf though!

[jhenderson]

lib/Object/ELFObjectFile.cpp

46 ↗

(On Diff #212652)

This should probably be moved so that it's in numerical order, i.e. between 9 and 11.

[grimar]

Well, my main concern is about ElfSymbolTypes array values representation.
I am not sure how my suggestion would affect the output, btw...

include/llvm/Object/ELFObjectFile.h

45 ↗

(On Diff #212652)

not relative to this patch, but I wonder why this is not a vector. (I do not like having a NumElfSymbolTypes).

lib/Object/ELFObjectFile.cpp

54 ↗

(On Diff #212652)

It looks a bit confusing to me. When I see "Unknown 7" it looks like its a 7th Unknown.
What do you think about hte following format?

{"Unknown(7)", "<unknown>(7)", 7},
{"Unknown(8)", "<unknown>(8)", 8},
{"Unknown(9)", "<unknown>(9)", 9},
{"OS Specific(11)", "<OS specific>(11)", 11},
{"OS Specific(12)", "<OS specific>(12)", 12},
{"Proc Specific(13)", "<processor specific>(13)", 13},
{"Proc Specific(14)", "<processor specific>(14)", 14},
{"Proc Specific(15)", "<processor specific>(15)", 15}

[grimar]

I tested the output with the change I suggested and it was:

Name                  Value           Class        Type         Size
 Line  Section
os_specific_10       |                |   U  |             IFUNC|     |     |*UND*
os_specific_11       |                |   U  | <OS specific>(11)|     |     |*UND*
os_specific_12       |                |   U  | <OS specific>(12)|     |     |*UND*
proc_specific_13     |                |   U  |<processor specific>(13)|     |     |*UND*
proc_specific_14     |                |   U  |<processor specific>(14)|     |     |*UND*
proc_specific_15     |                |   U  |<processor specific>(15)|     |     |*UND*
symbol_common        |                |   U  |            COMMON|     |     |*COM*
symbol_file          |                |   U  |              FILE|     |     |*UND*
symbol_func          |                |   U  |              FUNC|     |     |*UND*
symbol_ifunc         |                |   U  |             IFUNC|     |     |*UND*
symbol_notype        |                |   U  |            NOTYPE|     |     |*UND*
symbol_obj           |                |   U  |            OBJECT|     |     |*UND*
symbol_tls           |                |   U  |               TLS|     |     |*UND*
unknown_7            |                |   U  |      <unknown>(7)|     |     |*UND*
unknown_8            |                |   U  |      <unknown>(8)|     |     |*UND*
unknown_9            |                |   U  |      <unknown>(9)|

Looks good to me, though I do not really mind to keep the original output probably.
Lets see what others think.

[jhenderson]

The output for llvm-readelf should be identical to what GNU readelf prints, which is what the patch proposes. I'm happy with other options for the LLVM output. @grimar's suggestion here sounds good to me, though I'd suggest adding a space: Unknown (7).

[Sunil_Srivastava]

include/llvm/Object/ELFObjectFile.h

45 ↗

(On Diff #212652)

This goes back to an already submitted patch https://reviews.llvm.org/rL355742, where NumElfSymbolTypes was introduced. The table ElfSymbolTypes was moved from ELFDumper.cpp to ELFObjectFIle.cpp because it was also needed in llvm-nm.cpp

So, ElfSymbolTypes table is initialized in ELFObjectFile.cpp but it needs to be traversed in llvm-nm.cpp (function getELFTypeName) and ELFDumper.cpp, which needs to know the size. So the size needs to be somewhere visible to all of them. ELFObjectFile.h is that common file.

But perhaps it can be made a Vector. Current value, 16, is the maximum value, BTW because the type field is a 4 bit field. We could perhaps remove the named constant and just use hardcoded 16. All those will be separate issues though, that can be takes as a cleanup patch.

lib/Object/ELFObjectFile.cpp

54 ↗

(On Diff #212652)

I am open to any format for the first column; whatever the consensus is.

The second column comes from what the GNU nm prints, so that has to remain "<unknown>: 7", even with the inconsistent styles between 7, 11 and 13.

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

OK. Will update the proposal after checking GNU readelf.

[Sunil_Srivastava]

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

To clarify:

For type 10,

• GNU readelf prints IFUNC. So does llvm-nm with this proposal.
• GNU "nm --format=sysv" prints "<OS specific>: 10". This proposal prints IFUNC.

So you want llvm-nm to print "<OS specific>: 10" , at least for now. Right ? That will change llvm-nm test.

Looking at the code of both, it is easier to leave the ElfSymbolTypes table with IFUNC and make a special case in llvm-nm.cpp.

Will wait for confirmation before making change.

[jhenderson]

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

Having thought about it, here are my simple rules:

1. Places in llvm-nm and llvm-readelf that already print IFUNC should continue to do this for this patch (but we should discuss changing it to "<OS specific>: 10" later).
2. Places in llvm-nm and llvm-readelf that do not currently print IFUNC should continue to not print IFUNC.

In other words, the only behaviour change in llvm-nm and llvm-readelf should be to make things printing just "7" or whatever print <OS specific>/<unknown> etc. Everything else should be deferred to a later change.

[Sunil_Srivastava]

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

Hi James,

I agree with these "simple rules".

I believe the current proposal meets these rules. It does not change any IFUNC lines in tests, other than white-space changes, so it preserves the IFUNC behavior. It just fills missing entries.

Any change in the existing behavior should be a separate issue.

[MaskRay]

test/tools/llvm-nm/format-sysv-type.test

44 ↗

(On Diff #212652)

readelf prints IFUNC if e_ident[EI_OSABI] is ELFOSABI_NONE, ELFOSABI_GNU, or ELFOSABI_FREEBSD (Linux and FreeBSD are the two OSes I know that adopted IFUNC). Interestingly nm doesn't print IFUNC.

I am on the fence if it is necessary to make our IFUNC logic so complex for llvm-readelf/llvm-readobj. For this change, we should stick with the "simple rules".

[jhenderson]

I believe the current proposal meets these rules. It does not change any IFUNC lines in tests, other than white-space changes, so it preserves the IFUNC behavior. It just fills missing entries.

Okay. Please make the changes suggested inline re. ordering and formatting of the output ("Unknown (7)" or possibly "Unknown (0x7)" seems reasonable to me).

[Sunil_Srivastava]

Changed the table order and, typename style from "Unknown 7" to "Unknown(7)".

[jhenderson]

lib/Object/ELFObjectFile.cpp

54 ↗

(On Diff #212652)

I forgot this, but as demonstrated by your test, a typical symbol type output for a symbol in LLVM output style is "Section (0x3)". I don't think we want to repeat the number, and I therefore think simply printing "Unknown"/"OS Specific" etc in the first column is sufficient - the rest of the code then will append the "(0x7)" or whatever. In other words, the entries should be 3 "Unknown" strings, 2 "OS Specific" and 3 "Proc Specific".

[Sunil_Srivastava]

Removed number from the first column of the table, as suggested by James Henderson.

[Sunil_Srivastava]

lib/Object/ELFObjectFile.cpp

54 ↗

(On Diff #212652)

In other words, the entries should be 3 "Unknown" strings, 2 "OS Specific"
and 3 "Proc Specific".

OK. Done.

[jhenderson]

LGTM.

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

 #include "llvm/Support/MemoryBuffer.h"
 #include <cassert>
 #include <cstdint>
 #include <system_error>
 
 namespace llvm {
 namespace object {
 
-constexpr int NumElfSymbolTypes = 8;
+constexpr int NumElfSymbolTypes = 16;
 extern const llvm::EnumEntry<unsigned> ElfSymbolTypes[NumElfSymbolTypes];
 
 class elf_symbol_iterator;
 
 class ELFObjectFileBase : public ObjectFile {
   friend class ELFRelocationRef;
   friend class ELFSectionRef;
   friend class ELFSymbolRef;

llvm/trunk/lib/Object/ELFObjectFile.cpp

 const EnumEntry<unsigned> llvm::object::ElfSymbolTypes[NumElfSymbolTypes] = {
     {"None", "NOTYPE", ELF::STT_NOTYPE},
     {"Object", "OBJECT", ELF::STT_OBJECT},
     {"Function", "FUNC", ELF::STT_FUNC},
     {"Section", "SECTION", ELF::STT_SECTION},
     {"File", "FILE", ELF::STT_FILE},
     {"Common", "COMMON", ELF::STT_COMMON},
     {"TLS", "TLS", ELF::STT_TLS},
-    {"GNU_IFunc", "IFUNC", ELF::STT_GNU_IFUNC}};
+    {"Unknown", "<unknown>: 7", 7},
+    {"Unknown", "<unknown>: 8", 8},
+    {"Unknown", "<unknown>: 9", 9},
+    {"GNU_IFunc", "IFUNC", ELF::STT_GNU_IFUNC},
+    {"OS Specific", "<OS specific>: 11", 11},
+    {"OS Specific", "<OS specific>: 12", 12},
+    {"Proc Specific", "<processor specific>: 13", 13},
+    {"Proc Specific", "<processor specific>: 14", 14},
+    {"Proc Specific", "<processor specific>: 15", 15}
+};
 
 ELFObjectFileBase::ELFObjectFileBase(unsigned int Type, MemoryBufferRef Source)
     : ObjectFile(Type, Source) {}
 
 template <class ELFT>
 static Expected<std::unique_ptr<ELFObjectFile<ELFT>>>
 createPtr(MemoryBufferRef Object) {
   auto Ret = ELFObjectFile<ELFT>::create(Object);

llvm/trunk/test/tools/llvm-nm/format-sysv-type.test

Symbols:
   - Name: symbol_tls
     Type: STT_TLS
   - Name: symbol_file
     Type: STT_FILE
   - Name: symbol_obj
     Type: STT_OBJECT
   - Name: symbol_func
     Type: STT_FUNC
+  - Name: unknown_7
+    Type: 7
+  - Name: unknown_8
+    Type: 8
+  - Name: unknown_9
+    Type: 9
   - Name: symbol_ifunc
     Type: STT_GNU_IFUNC
-  - Name: os_specific
+  - Name: os_specific_10
+    Type: 10
+  - Name: os_specific_11
     Type: 11
-  - Name: proc_specific
+  - Name: os_specific_12
+    Type: 12
+  - Name: proc_specific_13
     Type: 13
-  - Name: unknown
-    Type: 7
-
-## FIXME: Should not print blank types for unknown, OS-specific and processor-
-##        specific types. See https://bugs.llvm.org/show_bug.cgi?id=41713.
+  - Name: proc_specific_14
+    Type: 14
+  - Name: proc_specific_15
+    Type: 15
 
-# CHECK:      os_specific   {{.*}}|                  |                |     |*UND*
-# CHECK-NEXT: proc_specific {{.*}}|                  |                |     |*UND*
-# CHECK-NEXT: symbol_common {{.*}}|            COMMON|                |     |*COM*
-# CHECK-NEXT: symbol_file   {{.*}}|              FILE|                |     |*UND*
-# CHECK-NEXT: symbol_func   {{.*}}|              FUNC|                |     |*UND*
-# CHECK-NEXT: symbol_ifunc  {{.*}}|             IFUNC|                |     |*UND*
-# CHECK-NEXT: symbol_notype {{.*}}|            NOTYPE|                |     |*UND*
-# CHECK-NEXT: symbol_obj    {{.*}}|            OBJECT|                |     |*UND*
-# CHECK-NEXT: symbol_tls    {{.*}}|               TLS|                |     |*UND*
-# CHECK-NEXT: unknown       {{.*}}|                  |                |     |*UND*
+# CHECK:      os_specific_10      |                |   U  |             IFUNC|                |     |*UND*
+# CHECK-NEXT: os_specific_11      |                |   U  | <OS specific>: 11|                |     |*UND*
+# CHECK-NEXT: os_specific_12      |                |   U  | <OS specific>: 12|                |     |*UND*
+# CHECK-NEXT: proc_specific_13    |                |   U  |<processor specific>: 13|                |     |*UND*
+# CHECK-NEXT: proc_specific_14    |                |   U  |<processor specific>: 14|                |     |*UND*
+# CHECK-NEXT: proc_specific_15    |                |   U  |<processor specific>: 15|                |     |*UND*
+# CHECK-NEXT: symbol_common       |                |   U  |            COMMON|                |     |*COM*
+# CHECK-NEXT: symbol_file         |                |   U  |              FILE|                |     |*UND*
+# CHECK-NEXT: symbol_func         |                |   U  |              FUNC|                |     |*UND*
+# CHECK-NEXT: symbol_ifunc        |                |   U  |             IFUNC|                |     |*UND*
+# CHECK-NEXT: symbol_notype       |                |   U  |            NOTYPE|                |     |*UND*
+# CHECK-NEXT: symbol_obj          |                |   U  |            OBJECT|                |     |*UND*
+# CHECK-NEXT: symbol_tls          |                |   U  |               TLS|                |     |*UND*
+# CHECK-NEXT: unknown_7           |                |   U  |      <unknown>: 7|                |     |*UND*
+# CHECK-NEXT: unknown_8           |                |   U  |      <unknown>: 8|                |     |*UND*
+# CHECK-NEXT: unknown_9           |                |   U  |      <unknown>: 9|                |     |*UND*

llvm/trunk/test/tools/llvm-readobj/elf-symbol-types.test

 # LLVM: Name: file
 # LLVM: Type: File
 # LLVM: Name: common
 # LLVM: Type: Common
 # LLVM: Name: tls
 # LLVM: Type: TLS
 # LLVM: Name: gnu_ifunc
 # LLVM: Type: GNU_IFunc
-# LLVM: Name: os_specific
-# LLVM: Type: 0xB
-# LLVM: Name: proc_specific
-# LLVM: Type: 0xD
-# LLVM: Name: unknown
-# LLVM: Type: 0x7
+
+# LLVM: Name: os_specific_11
+# LLVM: Type: OS Specific (0xB)
+# LLVM: Name: os_specific_12
+# LLVM: Type: OS Specific (0xC)
+
+# LLVM: Name: proc_specific_13
+# LLVM: Type: Proc Specific (0xD)
+# LLVM: Name: proc_specific_14
+# LLVM: Type: Proc Specific (0xE)
+# LLVM: Name: proc_specific_15
+# LLVM: Type: Proc Specific (0xF)
+
+# LLVM: Name: unknown_7
+# LLVM: Type: Unknown (0x7)
+# LLVM: Name: unknown_8
+# LLVM: Type: Unknown (0x8)
+# LLVM: Name: unknown_9
+# LLVM: Type: Unknown (0x9)
 
 # GNU: NOTYPE  {{.*}} notype
 # GNU: OBJECT  {{.*}} object
 # GNU: FUNC    {{.*}} func
 # GNU: SECTION {{.*}} .text
 # GNU: FILE    {{.*}} file
 # GNU: COMMON  {{.*}} common
 # GNU: TLS     {{.*}} tls
 # GNU: IFUNC   {{.*}} gnu_ifunc
-# GNU: b       {{.*}} os_specific
-# GNU: d       {{.*}} proc_specific
-# GNU: 7       {{.*}} unknown
+# GNU: <OS specific>: 11       {{.*}} os_specific_11
+# GNU: <OS specific>: 12       {{.*}} os_specific_12
+# GNU: <processor specific>: 13       {{.*}} proc_specific_13
+# GNU: <processor specific>: 14       {{.*}} proc_specific_14
+# GNU: <processor specific>: 15       {{.*}} proc_specific_15
+# GNU: <unknown>: 7       {{.*}} unknown_7
+# GNU: <unknown>: 8       {{.*}} unknown_8
+# GNU: <unknown>: 9       {{.*}} unknown_9
 
 !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
- Name:    common
     Type:    STT_COMMON
     Binding: STB_GLOBAL 
   - Name:    tls
     Type:    STT_TLS
     Binding: STB_GLOBAL
   - Name:    gnu_ifunc
     Type:    STT_GNU_IFUNC
     Binding: STB_GLOBAL
-  - Name:    os_specific
+  - Name:    os_specific_11
     Type:    11
     Binding: STB_GLOBAL
-  - Name:    proc_specific
+  - Name:    os_specific_12
+    Type:    12
+    Binding: STB_GLOBAL
+  - Name:    proc_specific_13
     Type:    13
     Binding: STB_GLOBAL
-  - Name:    unknown
+  - Name:    proc_specific_14
+    Type:    14
+    Binding: STB_GLOBAL
+  - Name:    proc_specific_15
+    Type:    15
+    Binding: STB_GLOBAL
+  - Name:    unknown_7
     Type:    7
     Binding: STB_GLOBAL
+  - Name:    unknown_8
+    Type:    8
+    Binding: STB_GLOBAL
+  - Name:    unknown_9
+    Type:    9
+    Binding: STB_GLOBAL

llvm/trunk/test/tools/yaml2obj/symbol-type.yaml

 # RUN: yaml2obj %s > %t
 # RUN: llvm-readobj --symbols %t | FileCheck %s
 
 # CHECK: Name: notype
 # CHECK: Type: None
 # CHECK: Name: normal_type
 # CHECK: Type: Object
 # CHECK: Name: .text
 # CHECK: Type: Section
 # CHECK: Name: known_hex
 # CHECK: Type: Object
-# CHECK: Name: unknown_hex
-# CHECK: Type: 0xB
+
+# CHECK: Name: unknown_hex_11
+# CHECK: Type: OS Specific (0xB)
+
 # CHECK: Name: known_int
 # CHECK: Type: Object
 # CHECK: Name: unknown_int
-# CHECK: Type: 0xB
+# CHECK: Type: OS Specific (0xB)
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
 Symbols:
   - Name:    notype
     Type:    STT_NOTYPE
     Binding: STB_GLOBAL 
   - Name:    normal_type
     Type:    STT_OBJECT
     Binding: STB_GLOBAL
   - Name:    .text
     Type:    STT_SECTION
     Binding: STB_GLOBAL
   - Name:    known_hex
     Type:    0x1
     Binding: STB_GLOBAL
-  - Name:    unknown_hex
+  - Name:    unknown_hex_11
     Type:    0xb
     Binding: STB_GLOBAL
   - Name:    known_int
     Type:    1
     Binding: STB_GLOBAL
   - Name:    unknown_int
     Type:    11
     Binding: STB_GLOBAL