[PATCH] [MC] Disassembled CFG reconstruction

[Quentin Colombet]

On May 23, 2013, at 2:41 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:

> On Thu, May 23, 2013 at 10:39 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>>  Hi Ahmed,
>> 
>>  Thanks for the last updates.
>>  We are almost done!
>> 
>>  I am still concerned about the way the MCObjectDisassemble handles the MCModule. Same for the clients of MCObjectDisassemble.
>>  In my opinion, the current approach is half way "MCObjectDisassemble owns the MCModule" and “MCModule is by its own”. I found that confusing.
>> 
>>  With the current model, I think it makes more sense to make MCObjectDisassemble owns the MCModule.
>>  Here is my proposal, that adds some laziness in the process.
>>  * Resurrect the MCModule field in MCObjectDisassemble.
>>  * Assign NULL to the MCModule field in the constructor.
>>  * Delete the MCModule field in the destructor.
>>  * Privatize buildModule.
>>  * Privatize buildCFG.
>>  * Resurrect the accessor: getModule.
>>  * Add a new argument to getModule: getModule(bool WithCFG = true)
>>  Now, as you can imagine, the getModule method will lazily build the MCModule and its MCFunction.
>> 
>>  Here is the code for that function:
>>  MCModule *MCObjectDisassemble::getModule(bool WithCFG) {
>>    if (!Module)
>>      Module = buildModule(MIA); //// old buildModule method
>>    if (WithCFG && Module->func_empty()) //// You may need to add func_empty method
>>      buildCFG();
>>    return Module;
>>  }
>> 
>>  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.
>> 
>>  What do you think?
> 
> Well, the current approach is definitely not perfect; the way I see it
> though, MCObjectDisassembler is just a builder, outlived by the
> MCModule, which is why I'm not convinced by MCOD owning the Module.
Thanks for the clarification, I thought MCOD was meant to live along with MCModule.

> Something close to the alternative you proposed sounds more
> appropriate: the client creates and owns the module; it calls
> MCObjectDisassembler::buildModule/buildCFG - that now both take an
> MCModule parameter -, or more descriptive variants thereof
> (buildSectionAtoms/buildFunctions?), and is then responsible for
> freeing it. That way the ownership is explicit, but the logic for
> section atom / function creation is still contained in
> MCObjectDisassembler.
That makes sense, but in that case why not moving the buildFunction method in the MCModule.

The thing is I do not like this mid-way approach: a MCModule does not known how to complete its internal representation.
Let me give an example.

Assume I want to build a MCModule.
I call the MCOD buildModule (MCOD is a factory of MCModule, I am fine with that).
I then give this MCModule to some other modules (I do not know what they want to do with it).
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.

 Another approach that is straightforward compared to the current patch is to directly returned a MCModule that has been build with the CFG.
Thus, the ownership remains to the clients, but the returned MCModule is complete.

If you think there may be clients that do not want the CFG, we can put a bool for that in buildModule.
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.

What do you think?

Quentin

> 
> 
> -- Ahmed Bougacha
> 
>>  I have inlined a few comments that are partly irrelevant if you go for the proposal.
>> 
>>  Thanks again for working on this.
>> 
>>  Quentin
>> 
>> 
>> ================
>> Comment at: include/llvm/MC/MCModule.h:34
>> @@ -30,1 +33,3 @@
>> +/// either instructions or data.
>> +/// An MCModule is created using MCObjectDisassembler::buildModule.
>> class MCModule {
>> ----------------
>> Typo: An -> A.
>> But here, putting “nothing" would be better :)
>> 
>> ================
>> Comment at: include/llvm/MC/MCObjectDisassembler.h:51
>> @@ +50,3 @@
>> +  /// format-specific features, like mach-o function_start or data_in_code LCs.
>> +  MCModule *buildModule();
>> +
>> ----------------
>> It is still not clear that the client has to free the memory allocated for the module.
>> In fact, I preferred the old method with the getModule (assuming MCObjectDisassemble owns the MCModule).
>> 
>> ================
>> Comment at: tools/llvm-objdump/llvm-objdump.cpp:334
>> @@ +333,3 @@
>> +                   utostr(filenum++) + ".dot").str().c_str(),
>> +                    **FI, IP.get());
>> +    }
>> ----------------
>> Where is the delete now?
>> 
>> 
>> http://llvm-reviews.chandlerc.com/D811
>> 
>> BRANCH
>>  mcfunc
>> 
>> ARCANIST PROJECT
>>  llvm

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130523/50f7448e/attachment.html>