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

[Paul Semel via Phabricator via llvm-commits]

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)

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


================
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 :/


================
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 ?


Repository:
  rL LLVM

https://reviews.llvm.org/D48281