[LLVMdev] [llvm-commits] JITEventListener for eventual profiling and maybe gdb support

[Evan Cheng]

Hi Jeffrey,

This looks very good. Thanks. Some comments:

+/// JitSymbolEntry - Each function that is JIT compiled results in  
one of these
+/// being added to an array of symbols.  This indicates the name of  
the function
+/// as well as the address range it occupies.  This allows the client  
to map
+/// from a PC value to the name of the function.
+struct JitSymbolEntry {

A nitpick. Please rename it to "JITSymbolEntry" for naming consistency.

+
+// This is a public symbol so the performance tools can find it.
+JitSymbolTable *__jitSymbolTable;
+

Hmm. Is there a better solution? From what I can tell, the event  
listener is responsible for allocating symbol table memory so it  
effectively owns it. Can we register the address with the performance  
tools? We're pushing to be more thread safe.

+struct FunctionEmittedEvent {
+  // Indices are local to the RecordingJITEventListener, since the
+  // JITEventListener interface makes no guarantees about the order of
+  // calls between Listeners.
+  int Index;

Use "unsigned" instead of "int"? Can index ever be negative?

Evan

On Jun 24, 2009, at 1:26 PM, Jeffrey Yasskin wrote:

> Ack, sorry. I should have sent this to llvm-commits instead. :-P
> Followups there please.
>
> On Wed, Jun 24, 2009 at 12:02 PM, Jeffrey  
> Yasskin<jyasskin at google.com> wrote:
>> I intend to use this to support oprofile's ability to symbolize  
>> JITted
>> code through the interface described at
>> http://oprofile.sourceforge.net/doc/devel/jit-interface.html. I
>> believe the interface will also be useful for gdb support. I'm
>> considering adding some flags to the JITEventListener to let the JIT
>> avoid collecting information no listener is going to use, but I won't
>> do that until there's a need.
>>
>> I've added EmittedFunctionDetails in this patch so that I don't have
>> to change the NotifyFunctionEmitted() interface in a future patch. To
>> record line number information, oprofile wants an array of structs of
>> the form:
>>
>>  struct debug_line_info {
>>       unsigned long vma;
>>       unsigned int lineno;
>>       /* The filename format is unspecified, absolute path,  
>> relative etc. */
>>       char const * filename;
>>  };
>>
>> so I'll add enough information to produce that to the
>> EmittedFunctionDetails in a later patch.
>>
>> Chris mentioned that someone may want to extend this to fire events  
>> on
>> stub emission too.
>>
>> Let me know what you think.
>> Jeffrey
>>
> <event-listener.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits