This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Add -hex-dump (-x) option
ClosedPublic

Authored by paulsemel on Jun 18 2018, 7:15 AM.

Details

Summary

This follows the same behavior as GNU objcopy --hex-dump option.

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Jun 18 2018, 7:15 AM

Could you add a test case?

Could you add a test case?

You're totally right. Plus, there is a little bug in the code, will fix it and upload the tests in the same time today !

paulsemel updated this revision to Diff 152038.Jun 20 2018, 2:17 AM

Fixed overflow on section when section end is inbetween rows.
Added test.

Could/should this be format-agnostic? (looks like this change only implements the functionality for ELF - but I'd imagine the existence of sections and their contents might be general enough to allow for different object file formats to support this without each format having custom handling)

Could/should this be format-agnostic? (looks like this change only implements the functionality for ELF - but I'd imagine the existence of sections and their contents might be general enough to allow for different object file formats to support this without each format having custom handling)

Well, I think it is actually feasable. I'm not really familiar with all the formats handled by readobj, but I can give it a shot !

paulsemel updated this revision to Diff 152467.Jun 22 2018, 6:46 AM

Added other binary formats handling.

dblaikie added inline comments.Jun 26 2018, 2:16 PM
include/llvm/Object/MachO.h
307–309 ↗(On Diff #152467)

Is there really no suitable existing API for getting the contents of a section? (like the ones above that use DataRefImpl?)

& these two APIs using different schemes (one returning Expected<SectionRef> the other returning error_code and taking a SectionRef out parameter) seem unnecessarily inconsistent - can they use a matching approach (I'd favor the Expected<SectionRef> option)?

Maybe @echristo has some ideas/familiarity here.

tools/llvm-readobj/COFFDumper.cpp
665 ↗(On Diff #152467)

Is there a need for bounds checking before casting that long down to an integer? (what's the intended behavior when the value is too large to fit in an integer? Is that tested?)

697 ↗(On Diff #152467)

Typo 'Least' -> 'Last' (or Lastly)?

tools/llvm-readobj/ELFDumper.cpp
153

Does this need support in each format explicitly - or is there a way to generalize it over all formats in one place? (ie: could all formats support the ability to retrieve the section contents, and then the printing would be generalized over that)

Certainly looks like there's a lot of duplication in the different dumper function implementations - at least would be good to pull a lot of that out into a generalized helper even if there is some format-specific stuff to be handled (though I'm not sure if there should be any format-specific stuff to handle).

echristo added inline comments.Jun 27 2018, 8:35 PM
include/llvm/Object/MachO.h
307–309 ↗(On Diff #152467)

In particular, I'd have thought we wanted getSectionContents here?

tools/llvm-readobj/ELFDumper.cpp
153

+1

paulsemel added inline comments.Jun 29 2018, 9:59 AM
include/llvm/Object/MachO.h
307–309 ↗(On Diff #152467)

Well, there is an API for getting section content, but it requires to have to so called section.
Here I want to get the section either by index or by name, so I don't see any problem with the two functions. (this scheme was already present for the ELF and COFF files, just added it to MachO)

Yes I agree about the two different schemes, I'm gonna change it right now :)

tools/llvm-readobj/COFFDumper.cpp
665 ↗(On Diff #152467)

Well, that's not a problem related to this patch, that's a more general problem about this function taking an integer..

697 ↗(On Diff #152467)

Yes, sorry about it :/

tools/llvm-readobj/ELFDumper.cpp
153

I actually didn't see any generic helper in this project, so I duplicated. I definitely think that it is doable.
Do you have an idea where I can write generic functions with this architecture ?

paulsemel updated this revision to Diff 153699.Jul 2 2018, 5:57 AM

Refactor code to create a generic function.
Applied David and Eric' suggestions.

Perhaps generalized further still - if every format has the ability to retrieve a section by name or by index and return a generic llvm::object::SectionRef - then the code can be written in general over that, query the SectionRef for the name and contents, etc?

Perhaps generalized further still - if every format has the ability to retrieve a section by name or by index and return a generic llvm::object::SectionRef - then the code can be written in general over that, query the SectionRef for the name and contents, etc?

Well, I thought about it, but the problem is that the interface are a bit different.. I tried to change the interface COFF (which is the only one that needs changes), but it would break too much things if I do so..

Perhaps generalized further still - if every format has the ability to retrieve a section by name or by index and return a generic llvm::object::SectionRef - then the code can be written in general over that, query the SectionRef for the name and contents, etc?

Well, I thought about it, but the problem is that the interface are a bit different.. I tried to change the interface COFF (which is the only one that needs changes), but it would break too much things if I do so..

Just to add one more argument to this. I think that getting the sections by Index/Name is not implemented as virtual method in ObjectFile. Thus, we might better do this way not to get confused on what can (for the moment) be generic and what can't be.
So what do you think ?

ping @dblaikie :)
I'd really like to go forward on this one, to be able to refact the -p option :)

This revision is now accepted and ready to land.Jul 10 2018, 12:53 PM

Ok I'm going to land this one, so that I can refactor the (-p) option, with the helpers created in this patch !

This revision was automatically updated to reflect the committed changes.