Page MenuHomePhabricator

[llvm-readobj] Separate `Symbol Version` dumpers into `LLVM style` and `GNU style`
ClosedPublic

Authored by Higuoxing on Mar 10 2019, 12:50 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2019, 12:50 AM
Higuoxing updated this revision to Diff 190006.Mar 10 2019, 1:02 AM

Format codes.

Higuoxing updated this revision to Diff 190010.Mar 10 2019, 4:33 AM

Updating D59186: [llvm-readobj] Separate Symbol Version dumpers into LLVM style and GNU style

Are you definitely going to implement a GNU style after this? Otherwise, you've removed any form of support from the GNU dumper, without replacing it with something.

tools/llvm-readobj/ELFDumper.cpp
3364–3365 ↗(On Diff #190010)

I think these unused named parameters might result in warnings on some build systems. Ditto below.

3366 ↗(On Diff #190010)

Here, and below, I don't think you should mention the section names, unless you have actually derived the names from the section passed in.

4488 ↗(On Diff #190010)

It may be worth factoring out this->dumper() into a local variable, since you use it in a number of places.

4519 ↗(On Diff #190010)

I know this was there before, but report_fatal_error is not a good way of reporting errors, since it implies an internal bug in LLVM. It should report a normal parse error or similar in the same way as other errors in llvm-readobj.

Ditto below.

I don't mind if it's in a follow-up patch though.

Are you definitely going to implement a GNU style after this? Otherwise, you've removed any form of support from the GNU dumper, without replacing it with something.

Yes, I would like to implement these functions. But I'm afraid these functions make this patch too big. I would like to make these patches in series, so you could review them one by one.

I'll leave this to James. AFAIK he will do a BoF at LLVM 2019. I think
I need to listen to it to understand the desired direction of tools.

(splitting of formats is my concern)

I'll leave this to James. AFAIK he will do a BoF at LLVM 2019.

Cool! I would like to watch it on Youtube ...
Thank you for taking time review this.

I'll leave this to James. AFAIK he will do a BoF at LLVM 2019.

Cool! I would like to watch it on Youtube ...
Thank you for taking time review this.

BoFs are not usually recorded, I believe, but there will be a write-up published on llvm-dev hopefully after the event.

I'll leave this to James. AFAIK he will do a BoF at LLVM 2019.

Cool! I would like to watch it on Youtube ...
Thank you for taking time review this.

BoFs are not usually recorded

Not this time, James )) Don't blame me when you be on youtube..

I am just kidding. Waiting for BoF..

Higuoxing updated this revision to Diff 191235.EditedMar 18 2019, 8:00 PM

Addressed @jhenderson 's comments

It may be worth factoring out this->dumper() into a local variable, since you use it in a number of places.

Done

I know this was there before, but report_fatal_error is not a good way of reporting errors, since it implies an internal bug in LLVM. It should report a normal parse error or similar in the same way as other errors in llvm-readobj.

I would like to fix this issue here and that in LLVM style dumper in next patch


I implemented 3 dumpers for .gnu.version, .gnu.version_r, .gnu.version_d, with some small adjustments.

  • GNU readelf treats 0x0 flags as none, so I add one in SymVersionFlags
  • I separate getSymbolVersion into getSymbolVersion and getSymbolVersionByIndex. Because when dumping .gnu.version section, we should print symbol version for symbols that have valid version name.

However, there are some issues.

  • Currently, when dumping .gnu.version_d section, we assume a symbol can have only one predecessor. But GNU readelf could dump these predecessors correctly.
  • When dumping some fields that contains offset into string table, if the offset is invalid, GNU readelf could give warning, and give the malformed value.

Shall I separate this patch into several small patches?

I'm always in favour of splitting up changes into multiple smaller reviews, as long as it doesn't make things more complicated by requiring temporary logic or whatever that is just going to be removed again immediately. It'll hopefully help diffing the changes too.

I'm always in favour of splitting up changes into multiple smaller reviews, as long as it doesn't make things more complicated by requiring temporary logic or whatever that is just going to be removed again immediately. It'll hopefully help diffing the changes too.

Ok, thanks :)

Higuoxing updated this revision to Diff 191345.EditedMar 19 2019, 10:27 AM

Update.

Addressed @jhenderson 's comments.

  • Using the actual section name.
  • Add FIXME comments near report_fatal_error() (I would like to fix this in the future).
rupprecht added inline comments.Mar 19 2019, 11:32 AM
test/tools/yaml2obj/verdef-section.yaml
2 ↗(On Diff #191345)

These tests should -- temporarily -- run both llvm-readobj+llvm-readelf and verify it's the same (or at least the version sections match)

(i.e. you probably can't cmp the whole thing like other tests do, but the same filecheck should work)

Higuoxing updated this revision to Diff 191425.Mar 19 2019, 5:55 PM

Addressed @rupprecht 's comments.

  • Add llvm-readelf to test cases.

I didn't modify the tests in tests/tools/yaml2obj, because I think those tests are for yaml2obj not llvm-readobj. Am I right?

I didn't modify the tests in tests/tools/yaml2obj, because I think those tests are for yaml2obj not llvm-readobj. Am I right?

Yes, that's right.

What's the relation between this patch and D59545?

What's the relation between this patch and D59545?

I want to implement dumpers for these sections. In .gnu.version, we should print the version name of a versioning symbol, so I need a helper function to find the version name by its vs_index.

What's the relation between this patch and D59545?

I want to implement dumpers for these sections. In .gnu.version, we should print the version name of a versioning symbol, so I need a helper function to find the version name by its vs_index.

Right, so does this patch depend on D59545 then?

tools/llvm-readobj/ELFDumper.cpp
4488 ↗(On Diff #190010)

This hasn't been done?

Higuoxing updated this revision to Diff 191500.Mar 20 2019, 8:38 AM

Right, so does this patch depend on D59545 then?

No, this is the first patch, which makes way for future implementation.

I'm happy with this, once my suggestions have been fixed.

test/tools/llvm-readobj/elf-versioninfo.test
83 ↗(On Diff #191500)

Nit: extra blank line.

Higuoxing updated this revision to Diff 191652.Mar 21 2019, 3:32 AM

Addressed comments. Thanks for reviewing!

LGTM, with one last comment. Please wait for @rupprecht to confirm that he is happy.

tools/llvm-readobj/ELFDumper.cpp
4524 ↗(On Diff #191652)

ELFDumper<ELFT> not auto

Higuoxing updated this revision to Diff 191659.Mar 21 2019, 4:14 AM

Thanks :) I will work on following patches.

rupprecht accepted this revision.Mar 21 2019, 11:05 AM
rupprecht added inline comments.
test/tools/llvm-readobj/elf-versioninfo.test
6 ↗(On Diff #191696)

nit: use LLVM-VERDEF/GNU-VERDEF for consistent hyphens (e.g. LLVM_VERDEF-NEXT should be LLVM-VERDEF-NEXT)

This revision is now accepted and ready to land.Mar 21 2019, 11:05 AM
Higuoxing updated this revision to Diff 191888.Mar 22 2019, 8:39 AM

Addressed comments.

This revision was automatically updated to reflect the committed changes.
Higuoxing reopened this revision.Mar 22 2019, 9:21 AM

Oops, I made a mistake in this patch, so I reverted the change. I'm sorry.

This revision is now accepted and ready to land.Mar 22 2019, 9:21 AM

Oops, I made a mistake in this patch, so I reverted the change. I'm sorry.

@Higuoxing The reversion doesn't appear to have been committed

RKSimon added a subscriber: dyung.Mar 22 2019, 2:09 PM

Oops, I made a mistake in this patch, so I reverted the change. I'm sorry.

@Higuoxing The reversion doesn't appear to have been committed

It looks like you only reverted ElfDumper.cpp at rL356777 - @dyung reverted the tests at rL356811

ormris added a subscriber: ormris.Mar 22 2019, 2:10 PM

Oops, I made a mistake in this patch, so I reverted the change. I'm sorry.

@Higuoxing The reversion doesn't appear to have been committed

It looks like you only reverted ElfDumper.cpp at rL356777 - @dyung reverted the tests at rL356811

I am sorry, after reverting the change, I went to sleep last night ... I should have double checked this change.

Higuoxing updated this revision to Diff 192029.EditedMar 24 2019, 6:45 AM

Hi, @jhenderson, @rupprecht

I am sorry that this patch should be reviewed once again. I made a mistake in previous patch, which I will illustrate below.

Higuoxing requested review of this revision.Mar 24 2019, 6:51 AM
Higuoxing marked an inline comment as done.
Higuoxing added inline comments.
tools/llvm-readobj/ELFDumper.cpp
4495 ↗(On Diff #192029)

The version of a symbol in .gnu.version should be calculated using Versym->vs_index & VERSYM_VERSION (0x7fff). Before this patch, we use *VersymBuf as its version. I think it's not a right way, though it does the right thing in most time.

jhenderson added inline comments.Mar 25 2019, 2:46 AM
test/tools/llvm-readobj/elf-versioninfo.test
119 ↗(On Diff #192029)

Nit: no newline at EOF.

test/tools/yaml2obj/verneed-section.yaml
74 ↗(On Diff #192029)

Nit: missing new line at EOF here.

tools/llvm-readobj/ELFDumper.cpp
4495 ↗(On Diff #192029)

I wasn't able to find any documentation for this outside of code, but I see that it is already used elsewhere in this file in a similar manner, and I found other examples of it in code elsewhere, so this looks fine to me.

I'm not quite sure what you mean by:

I think it's not a right way, though it does the right thing in most time.

Which way don't you think is right? What conditions does it do the right thin most of the time?

Higuoxing marked an inline comment as done.Mar 25 2019, 3:45 AM
Higuoxing added inline comments.
tools/llvm-readobj/ELFDumper.cpp
4495 ↗(On Diff #192029)

Which way don't you think is right? What conditions does it do the right thin most of the time?

I think using *VersymBuf is not a good way to indicate the symbol version, and we should use Versym->vs_index & VERSYM_VERSION. Usually, the version of a symbol should be a number that not so big. So, *VersymBuf (1 byte) should be equal to Versym->vs_index & VERSYM_VERSION (2 bytes).

Higuoxing updated this revision to Diff 192065.Mar 25 2019, 3:47 AM

Update diff according to comments.

jhenderson added inline comments.Mar 25 2019, 3:48 AM
test/tools/llvm-readobj/elf-versioninfo.test
11 ↗(On Diff #192029)

Maybe you should make one of the symbols in this section have a version > 0x7fff, e.g. 0xffff?

tools/llvm-readobj/ELFDumper.cpp
4495 ↗(On Diff #192029)

Right, gotcha. LGTM. In fact, it is wise to not do *VersymBuf, because that won't take account of any Endian issues.

Higuoxing marked an inline comment as done.Mar 25 2019, 3:52 AM
Higuoxing added inline comments.
test/tools/llvm-readobj/elf-versioninfo.test
11 ↗(On Diff #192029)

This test file uses a pre-compiled object file, I want (replace this/add one test) using yaml2obj in next patch.

jhenderson accepted this revision.Mar 25 2019, 3:57 AM

LGTM.

test/tools/llvm-readobj/elf-versioninfo.test
11 ↗(On Diff #192029)

Right, of course.

This revision is now accepted and ready to land.Mar 25 2019, 3:57 AM

LGTM.

Thanks a lot!

This revision was automatically updated to reflect the committed changes.