[PATCH] [MC] Disassembled CFG reconstruction

[Quentin Colombet]

  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.

  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.

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