[PATCH] D68636: [llvm-readobj] - Refine the LLVM-style output to be consistent.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:5598
                                                 const Elf_Shdr *Sec) {
-  DictScope SS(W, "Version symbols");
+  ListScope SS(W, "VersionSymbols");
   if (!Sec)
----------------
rupprecht wrote:
> MaskRay wrote:
> > grimar wrote:
> > > MaskRay wrote:
> > > > jhenderson wrote:
> > > > > grimar wrote:
> > > > > > jhenderson wrote:
> > > > > > > Ditto, though I'm wondering here why the VersionSymbols data includes stuff to do with its section header? If it didn't have that stuff, it would be a list.
> > > > > > > though I'm wondering here why the VersionSymbols data includes stuff to do with its section header?
> > > > > > 
> > > > > > I do not know. The same information is printed under "Sections [" tag anyways, so it is not useful probably:
> > > > > > 
> > > > > > ```
> > > > > >   Section {
> > > > > >     Index: 3
> > > > > >     Name: .gnu.version (30)
> > > > > >     Type: SHT_GNU_versym (0x6FFFFFFF)
> > > > > >     Flags [ (0x0)
> > > > > >     ]
> > > > > >     Address: 0x0
> > > > > >     Offset: 0xB4
> > > > > >     Size: 2
> > > > > >     Link: 0
> > > > > >     Info: 0
> > > > > >     AddressAlignment: 0
> > > > > >     EntrySize: 2
> > > > > >   }
> > > > > > ```
> > > > > > 
> > > > > > Should we remove "Section Name"/"Address"/"Offset"/"Link" and make it to be a list?
> > > > > I'd be inclined to do that personally, but it should be a separate change.
> > > > The Linux Standard Base calls this "Symbol Version Table" but this is named "VersionSymbols" here... What do you think if we just use the regular section type name "SHT_GNU_versym"? It may improve discoverability as well.
> > > >  What do you think if we just use the regular section type name "SHT_GNU_versym"? It may improve discoverability as well.
> > > 
> > > I.e. this is an opposite direction to what this patch does:
> > > 
> > > ```
> > > SHT_GNU_verdef { -> VersionDefinitions [
> > > SHT_GNU_verneed { -> VersionRequirements [
> > > ```
> > > 
> > > It will be only sections for which we use type names. Should we?
> > > I.e. this is an opposite direction to what this patch does:
> > 
> > Yes. .gnu.version is currently not consistent with .gnu.version_r and .gnu.version_d, and I know this patch tries to make them consistent.
> > 
> > I am not clear which direction we should go. I have a very weak preference for SHT_GNU_versym.
> > 
> > The naming does not seem very consistent here. While LSB names .gnu.version_r "version requirements", binutils-gdb elf.h names it "version needs section".
> Count me in the camp that (slightly) prefers seeing keys like "VersionDefinitions" rather than "SHT_GNU_verdef", though mostly on style grounds (not having a mix of CamelCase and SNAKE_CASE). However, I admit this is a bikeshed -- I think this patch (not D68704) will make things look nicer, but I have zero technical objections to it.
OK. Actually D68704 contains 2 independent improvements/changes. Renaming "Version symbols {" -> "SHT_GNU_versym [" is one of them.
Another is removing of the fields and making the top tag to be a list scope instead of beeing a dictionary. It looks useful to me by itself.
I'll update it to remove the renaming and keep this one on hold to collect other possible opinions too.


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

https://reviews.llvm.org/D68636