Page MenuHomePhabricator

[llvm-readobj] - Refine the LLVM-style output to be consistent.
ClosedPublic

Authored by grimar on Oct 8 2019, 5:58 AM.

Details

Summary

Our LLVM-style output was inconsistent.
This patch changes the output in the following way:

  1. SHT_GNU_verdef { -> VersionDefinitions [
  2. SHT_GNU_verneed { -> VersionRequirements [
  3. Version symbols [ -> VersionSymbols [
  4. EH_FRAME Header [ -> EHFrameHeader {

Diff Detail

Event Timeline

grimar created this revision.Oct 8 2019, 5:58 AM

Did you remember to run LLD tests? I'd expect to see changes there...

tools/llvm-readobj/DwarfCFIEHPrinter.h
102 ↗(On Diff #223834)

The data printed isn't a list, because it prints some metadata. I think this should be a dictionary too.

tools/llvm-readobj/ELFDumper.cpp
5128 ↗(On Diff #223834)

I don't think the ElfHeader should be a ListScope: it's not a list, but a dictionary (unlike, say, symbols).

5598 ↗(On Diff #223834)

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.

grimar added a comment.Oct 8 2019, 6:37 AM

Did you remember to run LLD tests? I'd expect to see changes there...

Sure. That will be a separate change, once we agree with LLVM side.

grimar marked an inline comment as done.Oct 8 2019, 6:50 AM
grimar added inline comments.
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

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?

jhenderson added inline comments.Oct 8 2019, 6:57 AM
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

I'd be inclined to do that personally, but it should be a separate change.

MaskRay added inline comments.Oct 8 2019, 7:38 AM
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

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.

grimar updated this revision to Diff 223861.Oct 8 2019, 7:42 AM
grimar marked 4 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
grimar marked an inline comment as done.Oct 8 2019, 7:45 AM
grimar added inline comments.
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

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?

Did you remember to run LLD tests? I'd expect to see changes there...

Sure. That will be a separate change, once we agree with LLVM side.

Just to clarify, are you still using the SVN layout rather than the monorepo? Using the monorepo would allow having all the changes in the same patch.

Aside from the discussion on the titles of Version data, this looks good to me. I don't have a strong preference whichever way is used.

grimar added a comment.EditedOct 8 2019, 8:24 AM

Did you remember to run LLD tests? I'd expect to see changes there...

Sure. That will be a separate change, once we agree with LLVM side.

Just to clarify, are you still using the SVN layout rather than the monorepo? Using the monorepo would allow having all the changes in the same patch.

Yes, I do not use monorepo yet. Going to switch soon as later this month svn will become readonly.

MaskRay added inline comments.Oct 8 2019, 8:12 PM
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

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

grimar planned changes to this revision.Oct 9 2019, 1:35 AM
lenary removed a subscriber: lenary.Oct 9 2019, 6:23 AM
rupprecht added inline comments.Oct 9 2019, 10:57 AM
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

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.

grimar marked an inline comment as done.Oct 10 2019, 1:05 AM
grimar added inline comments.
tools/llvm-readobj/ELFDumper.cpp
5598 ↗(On Diff #223834)

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.

MaskRay accepted this revision.Oct 16 2019, 6:08 AM

Fangrui, so do you mean you are agree with the following renaming?

SHT_GNU_verdef { -> VersionDefinitions [
SHT_GNU_verneed { -> VersionRequirements

(this patch was in "planned changes" status and needs a rebasing, then some changes will go away I think,
but this renaming disagreement was the reason I've not update it. If it is OK for you,
I'll probably need to do one more update for it).

Fangrui, so do you mean you are agree with the following renaming?

SHT_GNU_verdef { -> VersionDefinitions [
SHT_GNU_verneed { -> VersionRequirements

(this patch was in "planned changes" status and needs a rebasing, then some changes will go away I think,
but this renaming disagreement was the reason I've not update it. If it is OK for you,
I'll probably need to do one more update for it).

Yes, I think the renaming is right (I was on a subway, so it was inconvenient for me to make more comments). I think I proposed a scheme that I don't like myself ;-) glibc names SHT_GNU_versym #define SHT_GNU_versym 0x6fffffff /* Version symbol table. */. It is totally fine to name it VersionSymbols here.

grimar updated this revision to Diff 225307.Oct 16 2019, 2:21 PM
grimar edited the summary of this revision. (Show Details)
  • Rebased.
This revision is now accepted and ready to land.Oct 16 2019, 2:21 PM
rupprecht accepted this revision.Oct 16 2019, 2:37 PM
MaskRay accepted this revision.Oct 16 2019, 7:02 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 3:23 AM