[llvm-commits] [PATCH] Review request - MCJIT GDB source level debugging support

[Kaylor, Andrew]

Hi Eric,

Thanks for the review.

The ObjectBuffer type definition is in the jitdebugging namespace because it was a more-or-less arbitrary type definition used only in the JIT debugging interface.  I suppose we could get rid of it completely and just use one of the standard types.

I intended the two functions declared in JITRegistrar.h as the abstraction point, but I suppose I can see the value of moving an abstract JITRegistrar class into that file and providing a static function to get a GDB-specific registrar singleton.  So JITRegistrar.h would look something like this:

--------------

class JITRegistrar {
public:
  JITRegistrar() {}
  virtual ~JITRegistrar() {}

  void registerObject(const StringRef Object) = 0;
  void deregister(const StringRef Object) = 0;

  static JITRegistrar& GDBJITRegistrar();
}

--------------

Is that kind of what you had in mind?  If so, I'd probably still leave the JITRegistrar.cpp implementation more less as it is and just rename the file.

-Andy


-----Original Message-----
From: Eric Christopher [mailto:echristo at apple.com] 
Sent: Monday, April 16, 2012 10:17 AM
To: Kaylor, Andrew
Cc: llvm-commits at cs.uiuc.edu; Jim Grosbach; Danil Malyshev
Subject: Re: [llvm-commits] [PATCH] Review request - MCJIT GDB source level debugging support


On Apr 12, 2012, at 6:40 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> The attached patch implements GDB integration for source-level debugging of code JITed using the MCJIT execution engine.
>  

Few comments:

Why is ObjectBuffer in jitdebugging? Seems a bit odd.
You don't need to indent lines 2-n of a comment any more than the first line.
Since we'll probably want to support multiple debuggers we may wish to separate out the gdb jitdebugger support into a separate file. Mind doing that?
No std stream includes :)

-eric