<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 23, 2013, at 2:41 PM, Ahmed Bougacha <<a href="mailto:ahmed.bougacha@gmail.com">ahmed.bougacha@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Thu, May 23, 2013 at 10:39 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><blockquote type="cite"><br> Hi Ahmed,<br><br> Thanks for the last updates.<br> We are almost done!<br><br> I am still concerned about the way the MCObjectDisassemble handles the MCModule. Same for the clients of MCObjectDisassemble.<br> In my opinion, the current approach is half way "MCObjectDisassemble owns the MCModule" and “MCModule is by its own”. I found that confusing.<br><br> With the current model, I think it makes more sense to make MCObjectDisassemble owns the MCModule.<br> Here is my proposal, that adds some laziness in the process.<br> * Resurrect the MCModule field in MCObjectDisassemble.<br> * Assign NULL to the MCModule field in the constructor.<br> * Delete the MCModule field in the destructor.<br> * Privatize buildModule.<br> * Privatize buildCFG.<br> * Resurrect the accessor: getModule.<br> * Add a new argument to getModule: getModule(bool WithCFG = true)<br> Now, as you can imagine, the getModule method will lazily build the MCModule and its MCFunction.<br><br> Here is the code for that function:<br> MCModule *MCObjectDisassemble::getModule(bool WithCFG) {<br> if (!Module)<br> Module = buildModule(MIA); //// old buildModule method<br> if (WithCFG && Module->func_empty()) //// You may need to add func_empty method<br> buildCFG();<br> return Module;<br> }<br><br> Note: you may want to return a const MCModule *. You may also want to make getModule a const method and the MCModule field a mutable field.<br><br> What do you think?<br></blockquote><br>Well, the current approach is definitely not perfect; the way I see it<br>though, MCObjectDisassembler is just a builder, outlived by the<br>MCModule, which is why I'm not convinced by MCOD owning the Module.<br></div></blockquote><div dir="auto">Thanks for the clarification, I thought MCOD was meant to live along with MCModule.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Something close to the alternative you proposed sounds more<br>appropriate: the client creates and owns the module; it calls<br>MCObjectDisassembler::buildModule/buildCFG - that now both take an<br>MCModule parameter -, or more descriptive variants thereof<br>(buildSectionAtoms/buildFunctions?), and is then responsible for<br>freeing it. That way the ownership is explicit, but the logic for<br>section atom / function creation is still contained in<br>MCObjectDisassembler.</div></blockquote>That makes sense, but in that case why not moving the buildFunction method in the MCModule.</div><div><br></div><div>The thing is I do not like this mid-way approach: a MCModule does not known how to complete its internal representation.</div><div>Let me give an example.</div><div><br></div><div>Assume I want to build a MCModule.</div><div>I call the MCOD buildModule (MCOD is a factory of MCModule, I am fine with that).</div><div>I then give this MCModule to some other modules (I do not know what they want to do with it).</div><div>Assume one of this module wants to use the MCFunctions supposedly contained in the MCModule... This is not obvious that it has to get a (similar) MCOD and uses it to complete its internal representation of a MCModule.</div><div><br></div><div> Another approach that is straightforward compared to the current patch is to directly returned a MCModule that has been build with the CFG.</div><div>Thus, the ownership remains to the clients, but the returned MCModule is complete.</div><div><br></div><div>If you think there may be clients that do not want the CFG, we can put a bool for that in buildModule.</div><div>Regarding whether we should privatize buildCFG or not, I do not have a strong opinion, and in fact the answer is related to the previous question.</div><div><br></div><div>What do you think?</div><div><br></div><div>Quentin</div><div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><br>-- Ahmed Bougacha<br><br><blockquote type="cite"> I have inlined a few comments that are partly irrelevant if you go for the proposal.<br><br> Thanks again for working on this.<br><br> Quentin<br><br><br>================<br>Comment at: include/llvm/MC/MCModule.h:34<br>@@ -30,1 +33,3 @@<br>+/// either instructions or data.<br>+/// An MCModule is created using MCObjectDisassembler::buildModule.<br>class MCModule {<br>----------------<br>Typo: An -> A.<br>But here, putting “nothing" would be better :)<br><br>================<br>Comment at: include/llvm/MC/MCObjectDisassembler.h:51<br>@@ +50,3 @@<br>+ /// format-specific features, like mach-o function_start or data_in_code LCs.<br>+ MCModule *buildModule();<br>+<br>----------------<br>It is still not clear that the client has to free the memory allocated for the module.<br>In fact, I preferred the old method with the getModule (assuming MCObjectDisassemble owns the MCModule).<br><br>================<br>Comment at: tools/llvm-objdump/llvm-objdump.cpp:334<br>@@ +333,3 @@<br>+ utostr(filenum++) + ".dot").str().c_str(),<br>+ **FI, IP.get());<br>+ }<br>----------------<br>Where is the delete now?<br><br><br><a href="http://llvm-reviews.chandlerc.com/D811">http://llvm-reviews.chandlerc.com/D811</a><br><br>BRANCH<br> mcfunc<br><br>ARCANIST PROJECT<br> llvm</blockquote></div></blockquote></div><br></body></html>