Page MenuHomePhabricator

Enable readobj to emit readelf like output.
ClosedPublic

Authored by khemant on Oct 27 2015, 12:14 PM.

Details

Summary

This is first of a series that will enable to have an alternative dumping style for llvm-readobj tool. This particular patch prints output similar to readelf -h.

Diff Detail

Repository
rL LLVM

Event Timeline

khemant updated this revision to Diff 38580.Oct 27 2015, 12:14 PM
khemant retitled this revision from to Enable readobj to emit readelf like output..
khemant updated this object.
khemant added a reviewer: llvm-commits.
khemant set the repository for this revision to rL LLVM.
davide added a subscriber: davide.Oct 28 2015, 9:26 PM

Hi. I have a question: is there anything in particular that bothers you about the format used currently by readobj? Can't you just use a script to do the postprocessing for you?

Hello Davide,
The present format is good enough to convey the information. It is however not sufficient or sometimes difficult for present users of GNU tools (which are quite ubiquitous) to make a switch as it is complete redesign of the post processing tools they may have based on.

-Hemant

I'm not necessarily against this, but I think the patch should be generic for all targets and I've highlighted an example of where it isn't. Instead of conditionals I think you might just want to abstract out the dumping mechanism as well.

Also, don't forget to add llvm-commits as a subscriber to your patch when you upload it so that email goes to the list right away.

-eric

tools/llvm-readobj/ELFDumper.cpp
723 ↗(On Diff #38580)

Example...

What I have noticed is All enums are made to string as is in the LLVM style. However, GNU has different naming conventions such as "W" instead of SHF_WRITE string as is. This will cause a lot of code bloat. An easy solution is to store GNU flavor of string with usuale string inside

khemant added a comment.EditedOct 29 2015, 10:17 AM

My browser submitted the comment before completion. Apologies.
Here is complete comment:
What I have noticed is all enums are made to string as is in the LLVM style. However, GNU has different naming conventions such as "W" instead of SHF_WRITE string as is. This will cause a lot of code bloat. An easy solution is to store GNU flavor of string with usual string inside

template<typename T>
struct EnumEntry {
  StringRef Name;
  T Value;
};

Any thoughts on this?

Instead of conditionals I think you might just want to abstract out the dumping mechanism as well.

Can you explain this a bit more.

-Hemant

emaste added a subscriber: emaste.Dec 16 2015, 2:24 PM
khemant updated this revision to Diff 43402.Dec 21 2015, 2:39 PM
khemant removed rL LLVM as the repository for this revision.
khemant set the repository for this revision to rL LLVM.Dec 22 2015, 9:30 AM
khemant added inline comments.Jan 5 2016, 9:19 AM
tools/llvm-readobj/StreamWriter.h
323 ↗(On Diff #43402)

This is unnecessary. Formatted raw stream is a better fit. Next patch will make use of that.

khemant updated this revision to Diff 44135.Jan 6 2016, 11:16 AM

Updated to use Formatted stream. Adds the string used by readelf for enums.

Any comments/suggestions? Is this patch a good start for readelf like output format?

khemant added a reviewer: ruiu.Jan 8 2016, 2:48 PM
khemant added a subscriber: ruiu.
khemant updated this revision to Diff 45914.Jan 25 2016, 1:58 PM
khemant updated this object.
khemant edited reviewers, added: echristo; removed: ruiu.
khemant added inline comments.Jan 27 2016, 9:32 AM
tools/llvm-readobj/ELFDumper.cpp
2296 ↗(On Diff #45914)

Some versions of Cygwin apparently have newlib that do not support std::to_string. There needs to be a change to remove these calls.

echristo edited edge metadata.Feb 5 2016, 2:23 PM

Some inline comments.

tools/llvm-readobj/ELFDumper.cpp
1087–1090 ↗(On Diff #45914)

Can we get these two things to use the same interface?

2296 ↗(On Diff #45914)

Is this an issue for any targets we have a bot for?

tools/llvm-readobj/StreamWriter.cpp
15 ↗(On Diff #45914)

Just use stringstream here?

tools/llvm-readobj/StreamWriter.h
29 ↗(On Diff #45914)

More descriptive/comment relating it to the option would be good.

khemant updated this revision to Diff 47264.Feb 8 2016, 3:08 PM
khemant edited edge metadata.

Thank you echristo for comments. Modified as per your comments

khemant marked 3 inline comments as done.Feb 8 2016, 3:08 PM
khemant added inline comments.
tools/llvm-readobj/ELFDumper.cpp
2296 ↗(On Diff #45914)

Yes. There are a couple of cygwin bots I believe. I found this when I checked this commit:
520b6856921a0d4b9f079f532b53a56199521cf2

One inline comment that'll require a bit of reworking, but otherwise LGTM for now.

Thanks!

-eric

tools/llvm-readobj/ELFDumper.cpp
2305 ↗(On Diff #47264)

Ugh.

tools/llvm-readobj/llvm-readobj.h
45 ↗(On Diff #47264)

Hate to ask at the last minute but can you make this an enum (admittedly with two choices) so that the checks in the code are positive? (e.g. OutputStyle == GNU and OutputStyle == LLVM)

khemant added inline comments.Feb 9 2016, 12:52 PM
tools/llvm-readobj/llvm-readobj.h
45 ↗(On Diff #47264)

Sure.

khemant updated this revision to Diff 47361.Feb 9 2016, 1:52 PM

Add enum instead of a bool.

khemant marked an inline comment as done.Feb 9 2016, 1:53 PM

Added enum

echristo accepted this revision.Feb 9 2016, 2:00 PM
echristo edited edge metadata.

Couple inline comments. LGTM.

tools/llvm-readobj/StreamWriter.cpp
5 ↗(On Diff #47361)

Needed?

tools/llvm-readobj/llvm-readobj.h
45 ↗(On Diff #47361)

"Output"

This revision is now accepted and ready to land.Feb 9 2016, 2:00 PM
This revision was automatically updated to reflect the committed changes.