[PATCH] [MC] Disassembled CFG reconstruction

[Ahmed Bougacha]

On Wed, May 22, 2013 at 11:46 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>   Hi Ahmed,
>
>   You did a great job, especially with the comments, which are now in the recommended doxygen layout!
>   One comment though, you should do the same for MCFunction.h :).
>
>   Regarding the relocation ability we lost, you are right, this was broken. Thus, I agree that we can temporarily remove it.
>   Please make sure to add a FIXME stating that we should use the MCSymbolizier to enable this feature (or something along this line), so that we keep track of that.
>
>   I have spotted a few things that should be fixed.
>
>   Minor Concern:
>   - Some #include are not sorted alphabetically. See inline comments.
>
>   Major Concer:
>   - MCObjectDisassemble should free the MCModule, not the client of MCObjectDisassemble. See the inlined comments.
>
>   The alternative is that MCObjectDisassemble should not own the MCModule. In that case, MCModule constructor should take a MCObjectDisassemble as Argument (this will replace the MCObjectDisassemble::buildModule) and the MCModule should perform the CFG construction (i.e., move MCObjectDisassemble::buildCFG into MCModule).
>
>   I have to admit that I prefer the interface change. However, it is not a strong requirement.
>   You can go with the simple fix, i.e., MCObjectDisassemble frees the MCModule, and we can update the interfaces later if we want.

I've changed it so that the object disassembler returns the module,
which you then have to pass to buildCFG. I've kept it in OD though,
what do you think?

I've also explicited the ownerships in a few more places, because most
of the MC CFG stuff is read-only, and it makes sense to enforce the
fact that an MCBB only exists within an MCFN, which itself only exists
within an MCModule, etc..

-- Ahmed Bougacha

>   Thanks again,
>
>   -Quentin
>
>
> ================
> Comment at: include/llvm/MC/MCObjectDisassembler.h:49
> @@ +48,3 @@
> +  /// \brief Get the module built by this object disassembler.
> +  /// NOTE: MCObjectDisassembler doesn't delete the module, you are responsible
> +  /// for cleanup.
> ----------------
> I still do not like this memory management.
> If the memory is created by the MCObjectDisassembler, it should be freed by this same object.
> Could you move the clean-up code in the destructor of this class?
>
> ================
> Comment at: lib/MC/MCFunction.cpp:9
> @@ +8,3 @@
> +//===----------------------------------------------------------------------===//
> +#include "llvm/Support/raw_ostream.h"
> +#include "llvm/MC/MCAtom.h"
> ----------------
> Please sort the includes alphabetically.
>
> ================
> Comment at: lib/MC/MCObjectDisassembler.cpp:16
> @@ +15,3 @@
> +#include "llvm/MC/MCModule.h"
> +#include "llvm/ADT/SetVector.h"
> +#include "llvm/ADT/STLExtras.h"
> ----------------
> Sort includes alphabetically.
>
> ================
> Comment at: tools/llvm-objdump/MachODump.cpp:13
> @@ -12,2 +12,3 @@
>  //===----------------------------------------------------------------------===//
> +#include "llvm/ADT/StringExtras.h"
>
> ----------------
> Move this #include with the other llvm/ADT includes.

Missed all those, fixed!

> ================
> Comment at: tools/llvm-objdump/llvm-objdump.cpp:336
> @@ +335,3 @@
> +    }
> +    delete Mod;
> +  }
> ----------------
> Should not need an explicit delete here. See my comment in MCObjectDisassemble.h.
> If OD owns the Module, it is responsible for freeing it.
>
>
> http://llvm-reviews.chandlerc.com/D811
>
> BRANCH
>   mcfunc
>
> ARCANIST PROJECT
>   llvm