[PATCH] D65537: Print reasonable representations of type names in llvm-nm, readelf and readobj.

[Sunil Srivastava via Phabricator via llvm-commits]

Sunil_Srivastava marked 3 inline comments as done.
Sunil_Srivastava added inline comments.


================
Comment at: include/llvm/Object/ELFObjectFile.h:45
+constexpr int NumElfSymbolTypes = 16;
 extern const llvm::EnumEntry<unsigned> ElfSymbolTypes[NumElfSymbolTypes];
 
----------------
grimar wrote:
> not relative to this patch, but I wonder why this is not a vector. (I do not like having a `NumElfSymbolTypes`).
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.


================
Comment at: lib/Object/ELFObjectFile.cpp:54
+    {"Proc Specific 14", "<processor specific>: 14", 14},
+    {"Proc Specific 15", "<processor specific>: 15", 15}
+};
----------------
grimar wrote:
> 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}
> 
> ```
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.


================
Comment at: test/tools/llvm-nm/format-sysv-type.test:44
 
-# 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*
----------------
jhenderson wrote:
> jhenderson wrote:
> > 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.
> 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!
OK. Will update the proposal after checking GNU readelf.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65537/new/

https://reviews.llvm.org/D65537