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

Quentin Colombet qcolombet at apple.com
Fri May 17 13:36:28 PDT 2013


  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?


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



More information about the llvm-commits mailing list