[LLVMdev] X86 Disassembler

[Chris Lattner]

On Sep 3, 2009, at 5:25 PM, Sean Callanan wrote:

> I was away doing other things for a while, but I have an API patch  
> separated out, which (in addition to being much smaller than past  
> megapatches) corrects two issues Chris identified in his most recent  
> set of patches:
>
> - First, it makes the API a good deal simpler.  Now, you can  
> instantiate a single MCDisassembler and, each time you want an  
> instruction disassembled, you can simply pass it a MemoryRegion, an  
> offset, and an MCInst to populate.
>
> - Second, it adds MCDisassembler to the list of things you can get  
> from a TargetMachine, so you don't have to #include something from  
> lib/Target/X86 just to use the X86 disassembler.
>
> Please let me know what you think of this API.  Thanks for your time.

Hi Sean,

Sorry for the delay, this got buried in my inbox.

This API looks great, please commit it with a couple of minor tweaks:

+++ include/llvm/MC/MCDisassembler.h	(revision 0)

+#include "llvm/MC/MCInst.h"
+#include "llvm/Support/MemoryObject.h"
+#include "llvm/Support/raw_ostream.h"

Please forward declare these classes instead of #including their  
headers.

+  virtual bool          getInstruction(MCInst& instr,
+                                       uint64_t& size,
+                                       MemoryObject &region,
+                                       uint64_t address,
+                                       raw_ostream &vStream) const = 0;

"MCInst &instr", likewise with size.  Can "region" be passed in by  
const reference?


+++ lib/MC/MCDisassembler.cpp	(revision 0)
@@ -0,0 +1,20 @@
+//===-- lib/MC/MCDisassembler.h - Disassembler interface --------*- C+ 
+ -*-===//

Please fix the comment line (.h -> .cpp)

+namespace llvm {
+
+MCDisassembler::MCDisassembler() {
+}
+

In the .cpp files, please change them to 'using namespace llvm;' at  
the top to avoid nesting the entire file in a namespace.


Thanks Sean,

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090908/fee07436/attachment.html>