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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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*
----------------
Sunil_Srivastava wrote:
> Sunil_Srivastava wrote:
> > 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.
> 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.
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65537