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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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:
> 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.
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".


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65537