[PATCH] [MC] Disassembled CFG reconstruction

[Ahmed Bougacha]

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


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