[PATCH] Add MCSymbolizer for disassembled instruction symbolization/annotation.

[Jim Grosbach]

On May 17, 2013, at 1:36 PM, Quentin Colombet <qcolombet at apple.com> wrote:

> 
>  Hi Ahmed,
> 
>  This is going in the right direction.
>  Thanks for working on this.
> 
>  I made a few comments inline, I let you comment/fix those.
> 
>  In my opinion, the diff would have been simpler if you did not change the order of the arguments of tryAddingSymbolicOperand.
>  What was the rational for changing that order?
> 
>  Generally speaking the patch looks good to me, but I am concerned about the feature we lose for ARM.
> 
>  Jim, what do you think?
> 

Yes, it’s very important to not lose the handling of the low/high ARM relocations. Their variant kinds should definitely stay in the target-specific code, as they have no relation to anything but ARM. In general, the symbolizer needs to be able to interface to target specific code to handle things like this.

-Jim


> 
> ================
> Comment at: lib/MC/MCDisassembler.cpp:26
> @@ +25,3 @@
> +  assert(ctx != 0 && "No MCContext given for symbolic disassembly");
> +  setSymbolizer(new MCExternalSymbolizer(*ctx, getOpInfo, symbolLookUp,
> +                                         disInfo));
> ----------------
> I guess that this method is called only once, but in case, there is the delete counterpart of this new?
> 
> ================
> Comment at: lib/MC/MCExternalSymbolizer.cpp:117
> @@ +116,3 @@
> +  // variantkinds to MCSymbolRefExpr?
> +
> +  //if (SymbolicOp.VariantKind == LLVMDisassembler_VariantKind_ARM_HI16)
> ----------------
> As you noted it in your initial comment, this is a regression compared to what we had.
> ARM is an important target and I am not sure this is acceptable.
> 
> 
> http://llvm-reviews.chandlerc.com/D801

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