[PATCH] [MC] Disassembled CFG reconstruction

[Ahmed Bougacha]

Hi Quentin,

Thanks a lot for reviewing! I updated the patch following your
comments, see my response for some of them.

On Mon, May 20, 2013 at 4:49 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> ================
> Comment at: include/llvm/MC/MCModule.h:50
> @@ -51,2 +49,3 @@
>    /// createAtom - Creates a new MCAtom covering the specified offset range.
> -  MCAtom *createAtom(MCAtom::AtomType Type, uint64_t Begin, uint64_t End);
> +  MCTextAtom *createTextAtom(uint64_t Begin, uint64_t End);
> +  MCDataAtom *createDataAtom(uint64_t Begin, uint64_t End);
> ----------------
> Do you think you can stick to the old API here.
> Hence, could you avoid having two different methods?
> This will help to have some stability in the API if we add new types.

Keeping them separate has the advantage of giving the caller the right
return type, no need for casting.
As for API stability, I'm not sure to what extent it is possible,
given that new atom types now mean new classes.

> ================
> Comment at: include/llvm/MC/MCModule.h:40
> @@ -45,3 +39,3 @@
>   /// remap - Update the interval mapping for an MCAtom.
>   void remap(MCAtom *Atom, uint64_t NewBegin, uint64_t NewEnd);
>
> ----------------
> What are the constraints on Atom argument?
> Should it be part of Atoms?
> If so, explicit that in the comment.

Comment improved locally, for the next round!

> ================
> Comment at: tools/llvm-objdump/llvm-objdump.cpp:323
> @@ +322,3 @@
> +             ++II) {
> +          IP->printInst(&II->Inst, errs(), "");
> +          errs() << "\n";
> ----------------
> It seems to me that we lose the ability to print relocation (DumpAddress).
> You must keep this ability.

It was removed with all the -cfg code, given that it was partly broken
due to changes in object.
But the symbolizer patch should replace it anyway.
However, a feature that goes away is the "Loop begin" annotation. It
wouldn't be difficult to add it back, but this is the kind of thing
that I'll continue working on; I'm not sure if it's critical for the
time being.

-- Ahmed Bougacha

>
> http://llvm-reviews.chandlerc.com/D811
>
> BRANCH
>   mcfunc
>
> ARCANIST PROJECT
>   llvm