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

[Ahmed Bougacha]

================
Comment at: include/llvm/MC/MCDisassembler.h:104
@@ +103,3 @@
+  /// This takes ownership of \p Symzer, and deletes the previously set one.
+  void setSymbolizer(MCSymbolizer *Symzer);
+
----------------
Quentin Colombet wrote:
> Using an OwningPtr as a argument and take the ownership within the method seem the right thing to do.
I didn't think about using these; now they're everywhere!

================
Comment at: include/llvm/MC/MCRelocationInfo.h:42
@@ +41,3 @@
+  /// \brief Create an MCExpr for the relocation \p Rel.
+  virtual const MCExpr *createExprForRelocation(object::RelocationRef Rel);
+
----------------
Quentin Colombet wrote:
> What happen if the relocation is not supported?
> Also, I think the const keyword is not relevant for the returned type, here.
Added a comment.
Const is because the MC*Expr::Create* functions return const MCExprs.

================
Comment at: lib/MC/MCExternalSymbolizer.cpp:121
@@ +120,3 @@
+  MI.addOperand(MCOperand::CreateExpr(
+      RelInfo->createExprForCAPIVariantKind(Expr, SymbolicOp.VariantKind)));
+
----------------
Quentin Colombet wrote:
> Follow up of my comment in the related header:
> Are we sure createExprForCAPIVariantKind returns a valid Expr?
We're actually not, good find! There used to be an assertion in in MCRelocationInfo for unknown variant kinds, didn't make the change here.

================
Comment at: lib/Target/ARM/Disassembler/ARMDisassembler.cpp:507
@@ +506,3 @@
+  // FIXME: Does it make sense for value to be negative?
+  return Dis->tryAddingSymbolicOperand(MI, (uint32_t)Value, Address, isBranch,
+                                       /* Offset */ 0, InstSize);
----------------
Quentin Colombet wrote:
> Why do you cast Value into a uint32_t here?
> The API supports that and that would eliminate the FIXME.
This is done to match the behavior of the old ARM code (ARMDisassembler.cpp:525), which interpreted Value as un uint32. As it is used to look for a symbol at the same address, if Value was a negative int32_t, cast to a int64_t then to a uint64_t, the result probably wouldn't be what is expected, which is, I assume, the rationale behind the original code.


http://llvm-reviews.chandlerc.com/D801

BRANCH
  mcsymzer

ARCANIST PROJECT
  llvm