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

[Quentin Colombet]

  Hi Ahmed.

  This is looking good!

  There are a few minor fixes to do, see inlined comments, and one major concern.

  Major concern:
  * What happen if createExprForCAPIVariantKind does not support the given variant?
  The returned value of that method is not checked before being used.

  Thanks,

  -Quentin


================
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);
+
----------------
Using an OwningPtr as a argument and take the ownership within the method seem the right thing to do.

================
Comment at: include/llvm/MC/MCObjectSymbolizer.h:18
@@ +17,3 @@
+
+#include "llvm/MC/MCSymbolizer.h"
+#include "llvm/ADT/IntervalMap.h"
----------------
This appears twice: here and line 21.

================
Comment at: include/llvm/MC/MCObjectSymbolizer.h:37
@@ +36,3 @@
+  typedef DenseMap<uint64_t, object::RelocationRef> AddrToRelocMap;
+  // NOTE: Working around a missing SectionRef operator!= by storing
+  // DataRefImpl.p instead of SectionRef. Feel free to improve!
----------------
FIXME instead of NOTE?

================
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);
+
----------------
What happen if the relocation is not supported?
Also, I think the const keyword is not relevant for the returned type, here.

================
Comment at: include/llvm/MC/MCSymbolizer.h:48
@@ +47,3 @@
+  /// \brief Construct an MCSymbolizer, taking ownership of \p RelInfo.
+  MCSymbolizer(MCContext &Ctx, MCRelocationInfo *RelInfo);
+  virtual ~MCSymbolizer();
----------------
OwningPtr for RelInfo.

================
Comment at: lib/MC/MCDisassembler/Disassembler.cpp:23
@@ -22,2 +22,3 @@
 #include "llvm/Support/TargetRegistry.h"
+#include "llvm/MC/MCRelocationInfo.h"
 
----------------
Lexical order.

================
Comment at: lib/MC/MCExternalSymbolizer.cpp:121
@@ +120,3 @@
+  MI.addOperand(MCOperand::CreateExpr(
+      RelInfo->createExprForCAPIVariantKind(Expr, SymbolicOp.VariantKind)));
+
----------------
Follow up of my comment in the related header:
Are we sure createExprForCAPIVariantKind returns a valid Expr?

================
Comment at: lib/MC/MCObjectSymbolizer.cpp:132
@@ +131,3 @@
+  uint64_t UValue = Value;
+  // XXX: map instead of looping each time?
+  error_code ec;
----------------
Typo: XXX -> FIXME.

================
Comment at: lib/Object/MachOObjectFile.cpp:15
@@ -14,2 +14,3 @@
 
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Object/MachO.h"
----------------
Lexical order.

================
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);
----------------
Why do you cast Value into a uint32_t here?
The API supports that and that would eliminate the FIXME.


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

BRANCH
  mcsymzer

ARCANIST PROJECT
  llvm