<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 8, 2016, at 2:29 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com" class="">plotfi@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 8, 2016, at 1:53 PM, Kevin Enderby <<a href="mailto:enderby@apple.com" class="">enderby@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 8, 2016, at 11:27 AM, Puyan Lotfi <<a href="mailto:puyan.lotfi.llvm@gmail.com" class="">puyan.lotfi.llvm@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Kevin<div class=""><br class=""></div><div class="">I have a new patch attached.</div><div class=""><br class=""></div><div class="">I am fine with only supporting <span style="font-size:13px" class="">LC_SEGMENT_64 or LC_SEGMENT_64</span><span style="font-size:13px" class=""> for the time being but I adding a message in case the extract code encounters a segment other than those types.</span></div></div></div></blockquote><div class=""><br class=""></div>I’m not understanding why you consider it an error to have load commands other than an LC_SEGMENT_64 or LC_SEGMENT_64 used with Mach-O files used with your option. Is this not expected work with something an x86_64 Mach-O compiled for a Mac OS X? It would have other load commands for like LC_SYMTAB, LC_UUID, LC_FUNCTION_STARTS, etc. Or is this option only to work in some GPU Mach-O files with only LC_SEGMENT_64 or LC_SEGMENT_64 load commands?</div><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">The latter is correct. I can add support for those other segments in another commit, but right now I haven't written the code to do that. I don't consider it an error, but I figured the extract should print if it encounters a segment it doesn't support.</div></div></div></div></blockquote><div><br class=""></div>I guess maybe I don’t understand what you are saying. There are only two segment load commands, the LC_SEGMENT_64 and LC_SEGMENT_64. Are you using the word “segment” to mean “load command”? If not what do you mean by a "encounters a segment it doesn't support”?</div><div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""> Would you like me to add support for all of the possible segment types??</div></div></div></div></blockquote><div><br class=""></div>What other “segment types” are there? Sorry I might be lost I’m just not following you.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><span style="font-size:13px" class=""><br class=""></span></div><div class=""><span style="font-size:13px" class="">I have added some additional flags to narrow down which segment to dump.</span></div></div></div></blockquote><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><span style="font-size:13px" class="">Also, I added a flag and some code to print the section to stderr in a hex format for testing</span></div></div></div></blockquote><div class=""><br class=""></div>For test wouldn’t you just extract the sections using your option and test that it is the same as a file one you add to the test suite?</div><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">I could, but how should I do that? I mean, should I just run cmp on both files?</div></div></div></div></blockquote><div><br class=""></div>That’s what I would do. As that is the functionality you are testing/</div><div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""> Can I even do that if the test is run on a system without cmp? Sorry, I was just trying to do the thing where I could rely on FileCheck the most here.</div></div></div></div></blockquote><div><br class=""></div>I’m not a test expert. Maybe there is a way to test that to files are the same with FileCheck. I guess you might want to see if that is possible.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">I don’t really understand the need for the code to dump the section to stderr. There is already the option -section to dump Mach-O sections. Also seems odd that you use the ‘C’ printf style of coding here. Maybe something more C++ like would fit better with the llvm code if you really want to keep this bit.</div><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">Ah, yeah you're right what I am doing here is weird.</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><span style="font-size:13px" class=""> and I've added a test case as well.</span></div></div></div></blockquote><div class=""><br class=""></div>Seems like this could be a bit smaller and test the extracted file and use your -extract-macho-sections-dir option. Without that it seems like the test will write into the location of the input file.</div><div class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div><div class="">Ah right I think I forgot to add that flag to the test. </div><div class=""><br class=""></div><div class="">PL</div><br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><span style="font-size:13px" class=""><br class=""></span></div><div class=""><span style="font-size:13px" class="">Let me known how this looks to you.</span></div><div class=""><span style="font-size:13px" class=""><br class=""></span></div><div class=""><span style="font-size:13px" class="">Thanks</span></div><div class=""><span style="font-size:13px" class=""><br class=""></span></div><div class=""><span style="font-size:13px" class="">PL</span></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Aug 5, 2016 at 4:22 PM, Kevin Enderby <span dir="ltr" class=""><<a href="mailto:enderby@apple.com" target="_blank" class="">enderby@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Puyan,<br class="">
<br class="">
Seems like the functionality would be easier to test if it didn’t write the output files to the same directory as the input file. And of course you’ll want to add a test case as the llvm tradition with any changes.<br class="">
<br class="">
Also I’m not an llvm style expert but you can likely shorten the logic and not use the a switch statement for your return value from ExtractLoadCommand(). Also it looks like it should get an error with any Mach-O file that has non LC_SEGMENT_64 or LC_SEGMENT_64 commands? Is that correct and what you want.<br class="">
<br class="">
You also might just want to return a bool for success of failure from ExtractSectionData() and your other routines.<br class="">
<br class="">
Also it llvm seems to like the shortest bit if code so I don’t think you need the “int Res” and could just use an if statement.<br class="">
<br class="">
Kev<br class="">
<div class=""><div class="h5"><br class="">
> On Aug 5, 2016, at 4:06 PM, Puyan Lotfi <<a href="mailto:puyan.lotfi.llvm@gmail.com" class="">puyan.lotfi.llvm@gmail.com</a>> wrote:<br class="">
><br class="">
> Hi Kevin, All:<br class="">
><br class="">
> I have a macho-extract tool kicking around locally that I threw together a while back that simply dumps each section in a MachO to disk, and I've decided it's probably high-time to upstream the functionality into llvm-objdump.<br class="">
><br class="">
> Let me know if this looks good enough to check in, or otherwise feedback would be nice too.<br class="">
><br class="">
> Thanks<br class="">
><br class="">
> PL<br class="">
><br class="">
><br class="">
</div></div>> <llvm-objdump-macho-extract.<wbr class="">patch><br class="">
<br class="">
</blockquote></div><br class=""></div>
<span id="cid:9CF173AF-71CD-46A4-8602-14EDECAA5F16@apple.com" class=""><llvm-objdump-macho-extract2.patch></span></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></body></html>