[PATCH] D48281: [llvm-readobj] Add -hex-dump (-x) option

[David Blaikie via llvm-commits]

On Fri, Jun 29, 2018 at 9:59 AM Paul Semel via Phabricator <
reviews at reviews.llvm.org> wrote:

> paulsemel added inline comments.
>
>
> ================
> Comment at: include/llvm/Object/MachO.h:307-309
> +  Expected<SectionRef> getSection(unsigned SectionIndex) const;
> +  std::error_code getSection(StringRef SectionName,
> +                             const SectionRef *&Result) const;
> ----------------
> echristo wrote:
> > dblaikie wrote:
> > > 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.
> > In particular, I'd have thought we wanted getSectionContents here?
> 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)
>

Might make sense to expose an API for getting the section by name/index
then? (then use the existing API to get its contents) - though what general
APIs are there for getting sections already (guess just walking all the
sections in an object file?) & could those be used instead? (will a custom
implementation per object file format be significantly better than a
generic one over whatever section access is already provided?)


>
> Yes I agree about the two different schemes, I'm gonna change it right now
> :)
>
>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:665
> +  else {
> +    error(Obj->getSection((int)SectionIndex, Sec));
> +    if (!Sec)
> ----------------
> dblaikie wrote:
> > 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?)
> Well, that's not a problem related to this patch, that's a more general
> problem about this function  taking an integer..
>

Well, it's a problem with this patch in the sense that this patch is
introducing code that may invoke undefined behavior (if I'm remembering
correctly) when converting a long to an int, right?


>
>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:697
> +    // Then, the (4 - i) spaces that are in between the rows.
> +    // Least, if we cut in a middle of a row, we add the remaining
> characters,
> +    // which is (8 - (k * 2))
> ----------------
> dblaikie wrote:
> > Typo 'Least' -> 'Last' (or Lastly)?
> Yes, sorry about it :/
>

No worries at all (:


>
>
> ================
> Comment at: tools/llvm-readobj/ELFDumper.cpp:153
>    void printSectionAsString(StringRef StringName) override;
> +  void printSectionAsHex(StringRef StringName) override;
>    void printHashTable() override;
> ----------------
> echristo wrote:
> > dblaikie wrote:
> > > 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).
> > +1
> 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 ?
>

You could have default (or non-virtual, until proven necessary)
implementations in the ObjDumper base class - or, more likely, say a
"dumpSection" static free function in llvm-readobj.cpp (like the other ones
there - dumpInput, dumpArchive, etc) that's called from where you currently
call dumper->printSectionAsHex - instead "printSectionAsHex(*Obj,
SectionName);" or the like)


>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D48281
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180629/abc49f8a/attachment.html>