Page MenuHomePhabricator

llvm-readobj: enable GNU output style sections and relocations printing for ELF files
ClosedPublic

Authored by khemant on Feb 22 2016, 2:42 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

khemant updated this revision to Diff 48735.Feb 22 2016, 2:42 PM
khemant retitled this revision from to llvm-readobj: enable GNU output style sections and relocations printing for ELF files.
khemant updated this object.
khemant added reviewers: echristo, Bigcheese.
khemant set the repository for this revision to rL LLVM.
khemant added subscribers: llvm-commits, echristo, Bigcheese.
echristo accepted this revision.Feb 23 2016, 2:42 PM
echristo edited edge metadata.

A few inline comments to separate out some seemingly inconsequential changes. Otherwise the general idea seems fine, I haven't looked deeply at it, but a quick glance seemed like it was ok.

tools/llvm-readobj/ELFDumper.cpp
523 ↗(On Diff #48735)

Can you do this separately?

572 ↗(On Diff #48735)

Can you do this separately?

tools/llvm-readobj/StreamWriter.h
62 ↗(On Diff #48735)

Can you make this change separately?

This revision is now accepted and ready to land.Feb 23 2016, 2:42 PM
Bigcheese added inline comments.Feb 23 2016, 2:51 PM
tools/llvm-readobj/ELFDumper.cpp
47 ↗(On Diff #48735)

I would prefer this as:

#define TYPEDEF_ELF_TYPES(ELFT) ...

So that it's clear that it's a typedef and what it depends on.

272 ↗(On Diff #48735)

Don't need this->

khemant added inline comments.Feb 23 2016, 3:40 PM
tools/llvm-readobj/ELFDumper.cpp
47 ↗(On Diff #48735)

Got it. Will do this.

272 ↗(On Diff #48735)

Will do.

523 ↗(On Diff #48735)

This change was needed due to series of const functions that compiler figured out when I make the pointer to ELFDumper in the DumpStyle class a const pointer.

ELFDumper.cpp:521:11: note: no known conversion for implicit 'this' parameter from 'const {anonymous}::ELFDumper<llvm::object::ELFType<(llvm::support::endianness)0u, true> >* const' to '{anonymous}::ELFDumper<llvm::object::ELFType<(llvm::support::endianness)0u, true> >*'

572 ↗(On Diff #48735)

Same as above.

tools/llvm-readobj/StreamWriter.h
62 ↗(On Diff #48735)

Will do.

echristo added inline comments.Feb 23 2016, 3:43 PM
tools/llvm-readobj/ELFDumper.cpp
523 ↗(On Diff #48735)

Sure, it just seems like that part of the change could be separable. Including the pointer? If not, that's fine too.

khemant updated this revision to Diff 48853.Feb 23 2016, 3:47 PM
khemant edited edge metadata.

Addressed comments noted in previous patch. Please re-accept this version.

khemant added inline comments.Feb 23 2016, 3:49 PM
tools/llvm-readobj/ELFDumper.cpp
523 ↗(On Diff #48853)

This is the patch that introduced the pointer. I would like to have it const from beginning instead of retrofitting in future as it will cause larger unneeded churn.

Still LGTM. Thanks!

tools/llvm-readobj/ELFDumper.cpp
523 ↗(On Diff #48853)

Oh sure. Never that. First maybe, but not after.

rafael edited edge metadata.Feb 25 2016, 6:39 AM

Thanks for implementing this. I do find the readelf output more human friendly. It is nice to have both in the same tool.!

Bigcheese requested changes to this revision.Feb 25 2016, 12:49 PM
Bigcheese edited edge metadata.
Bigcheese added inline comments.
tools/llvm-readobj/ELFDumper.cpp
271 ↗(On Diff #48853)

The usage of this looks pretty weird in code. I'd prefer we not use operator overloads that don't "do what the ints do".

2231 ↗(On Diff #48853)

We only use the ELF spec names for types and variables that _directly_ map to the ELF file. Please use the llvm naming convention for these.

2236 ↗(On Diff #48853)

Bias

2262 ↗(On Diff #48853)

format cannot directly take a packed_endian_specific_integral as it is not implemented in a type safe way. It directly passes everything to snprintf, which skips the byte swapping conversion operator.

2266 ↗(On Diff #48853)

80 colums.

2406 ↗(On Diff #48853)

llvm style: Number, Type, Size, ...

This revision now requires changes to proceed.Feb 25 2016, 12:49 PM
khemant updated this revision to Diff 49229.Feb 26 2016, 1:34 PM
khemant edited edge metadata.

Updated to reflect changes requested by Bigcheese

rafael added inline comments.Mar 1 2016, 1:50 PM
tools/llvm-readobj/ELFDumper.cpp
237 ↗(On Diff #49229)

Do you need the typedef here?

2239 ↗(On Diff #49229)

Is there a type safe api you could use?
format_decimal maybe?

2297 ↗(On Diff #49229)

why do you need the to_string here?

khemant added inline comments.Mar 1 2016, 2:35 PM
tools/llvm-readobj/ELFDumper.cpp
237 ↗(On Diff #49229)

Correct. This is not needed in base class as I only use object here.

2297 ↗(On Diff #49229)

Correct. This is no longer needed.

khemant updated this revision to Diff 49554.Mar 1 2016, 3:06 PM
khemant edited edge metadata.

Removed unneeded typedefs in base class and any to_string that was operand to << operator of raw_ostream.

rafael added inline comments.Mar 8 2016, 6:25 AM
tools/llvm-readobj/ELFDumper.cpp
956 ↗(On Diff #49554)

Entry

960 ↗(On Diff #49554)

These are just Entry.AltName, no?

khemant updated this revision to Diff 50058.Mar 8 2016, 11:07 AM

Use AltName for section type string.

khemant marked 10 inline comments as done.Mar 8 2016, 11:10 AM
Bigcheese accepted this revision.Mar 14 2016, 2:20 PM
Bigcheese edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 14 2016, 2:20 PM
This revision was automatically updated to reflect the committed changes.
khemant marked an inline comment as done.