[PATCH] D74391: [llvm-readelf] Match GNU readelf more more closely when dumping notes

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5054
     } else if (!Descriptor.empty()) {
-      OS << "   description data:";
+      OS << "    Description data:";
       for (uint8_t B : Descriptor)
----------------
jhenderson wrote:
> MaskRay wrote:
> > arichardson wrote:
> > > arichardson wrote:
> > > > rupprecht wrote:
> > > > > I'm not able to reproduce this part. Using the example binary generated by llvm/test/tools/llvm-readobj/ELF/note-freebsd.s:
> > > > > 
> > > > > ```
> > > > > $ readelf --version
> > > > > GNU readelf (GNU Binutils) 2.34.50.20200211
> > > > > ...
> > > > > $ readelf -n /tmp/freebsd.o
> > > > > 
> > > > > Displaying notes found in: .note.foo
> > > > >   Owner                Data size        Description
> > > > >   FreeBSD              0x00000000       NT_THRMISC (thrmisc structure)
> > > > >   FreeBSD              0x00000000       NT_PROCSTAT_PROC (proc data)
> > > > > 
> > > > > Displaying notes found in: .note.bar
> > > > >   Owner                Data size        Description
> > > > >   FreeBSD              0x00000000       NT_PROCSTAT_FILES (files data)
> > > > > 
> > > > > Displaying notes found in: .note.baz
> > > > >   Owner                Data size        Description
> > > > >   FreeBSD              0x0000001c       Unknown note type: (0x00000003)
> > > > >    description data: 4c 6f 72 65 6d 20 69 70 73 75 6d 20 64 6f 6c 6f 72 20 73 69 74 20 61 6d 65 74 00 00
> > > > > ```
> > > > > 
> > > > > Are you comparing against readelf sources that have a local patch to capitalize description?
> > > > Interesting, I am using `GNU readelf (GNU Binutils) 2.32.51`. Will check if it's changed upstream since.
> > > Looking at the binutils sources, it seems like they use the upper-case variant (with four spaces) for GNU notes, but the lowercase one (with three spaces) for unknown ones.
> > > 
> > > I can revert this part of the change to remain exactly compatible with GNU readelf?
> > If you have a suggestion for GNU readelf output, it is recommended sending an email to https://sourceware.org/ml/binutils/2020-02/ to get some feedback...
> +1 to feeding back to GNU on this one. It seems like a bug to me that their output is inconsistent, and I don't think we should try to match it in that case.
> I can revert this part of the change to remain exactly compatible with GNU readelf?
I'm 100% ok with this change if this part is reverted (also w/ James' suggestion about refactoring that one method).

I'm actually wondering if we should accept a difference here too -- this feels like it falls in the bucket of the GNU tool having a bug that we don't want to emulate. OTOH, I'm *sure* based on experience there is some tool out there parsing `readelf | grep 'description data'` that would be broken...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74391