<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 29, 2018 at 9:59 AM Paul Semel via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">paulsemel added inline comments.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Object/MachO.h:307-309<br>
+ Expected<SectionRef> getSection(unsigned SectionIndex) const;<br>
+ std::error_code getSection(StringRef SectionName,<br>
+ const SectionRef *&Result) const;<br>
----------------<br>
echristo wrote:<br>
> dblaikie wrote:<br>
> > Is there really no suitable existing API for getting the contents of a section? (like the ones above that use DataRefImpl?)<br>
> > <br>
> > & 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)?<br>
> > <br>
> > Maybe @echristo has some ideas/familiarity here.<br>
> In particular, I'd have thought we wanted getSectionContents here?<br>
Well, there is an API for getting section content, but it requires to have to so called section.<br>
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)<br></blockquote><div><br></div><div>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?)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Yes I agree about the two different schemes, I'm gonna change it right now :)<br>
<br>
<br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:665<br>
+ else {<br>
+ error(Obj->getSection((int)SectionIndex, Sec));<br>
+ if (!Sec)<br>
----------------<br>
dblaikie wrote:<br>
> 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?)<br>
Well, that's not a problem related to this patch, that's a more general problem about this function taking an integer..<br></blockquote><div><br>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?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:697<br>
+ // Then, the (4 - i) spaces that are in between the rows.<br>
+ // Least, if we cut in a middle of a row, we add the remaining characters,<br>
+ // which is (8 - (k * 2))<br>
----------------<br>
dblaikie wrote:<br>
> Typo 'Least' -> 'Last' (or Lastly)?<br>
Yes, sorry about it :/<br></blockquote><div><br>No worries at all (:<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: tools/llvm-readobj/ELFDumper.cpp:153<br>
void printSectionAsString(StringRef StringName) override;<br>
+ void printSectionAsHex(StringRef StringName) override;<br>
void printHashTable() override;<br>
----------------<br>
echristo wrote:<br>
> dblaikie wrote:<br>
> > 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)<br>
> > <br>
> > 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).<br>
> +1 <br>
I actually didn't see any generic helper in this project, so I duplicated. I definitely think that it is doable.<br>
Do you have an idea where I can write generic functions with this architecture ?<br></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Repository:<br>
rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D48281" rel="noreferrer" target="_blank">https://reviews.llvm.org/D48281</a><br>
<br>
<br>
<br>
</blockquote></div></div>