This is an archive of the discontinued LLVM Phabricator instance.

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
521

Can you do this separately?

570

Can you do this separately?

tools/llvm-readobj/StreamWriter.h
62

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

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.

273

Don't need this->

khemant added inline comments.Feb 23 2016, 3:40 PM
tools/llvm-readobj/ELFDumper.cpp
47

Got it. Will do this.

273

Will do.

521

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> >*'

570

Same as above.

tools/llvm-readobj/StreamWriter.h
62

Will do.

echristo added inline comments.Feb 23 2016, 3:43 PM
tools/llvm-readobj/ELFDumper.cpp
521

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
521

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
521

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
272

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

2212

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.

2217

Bias

2243

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.

2247

80 colums.

2387

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

Do you need the typedef here?

2224

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

2282

why do you need the to_string here?

khemant added inline comments.Mar 1 2016, 2:35 PM
tools/llvm-readobj/ELFDumper.cpp
237

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

2282

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
960

Entry

964

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.