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

[Sunil Srivastava via Phabricator via llvm-commits]

Sunil_Srivastava marked an inline comment as done.
Sunil_Srivastava 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*
----------------
jhenderson wrote:
> 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.
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65537