Evan Cheng
2009-Jun-24 21:43 UTC
[LLVMdev] [llvm-commits] JITEventListener for eventual profiling and maybe gdb support
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
Eric Christopher
2009-Jun-24 21:46 UTC
[LLVMdev] [llvm-commits] JITEventListener for eventual profiling and maybe gdb support
On Jun 24, 2009, at 2:43 PM, Evan Cheng wrote:> +// 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.This code is just a move of something already in llvm. I'm currently bugging the performance tools guys here for a better way to do this, and have hopes of something reasonably soon I can add into llvm. That said, as far as I can tell from asking this code doesn't work with any performance tools at apple and is off by default :) -eric
Jeffrey Yasskin
2009-Jun-25 00:43 UTC
[LLVMdev] [llvm-commits] JITEventListener for eventual profiling and maybe gdb support
On Wed, Jun 24, 2009 at 2:43 PM, Evan Cheng<evan.cheng at apple.com> wrote:> 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.Done, although this is just a move.> + > +// 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.As Eric said, this is just moving existing code, and it's #ifdef'ed out. I'll let the Shark folks invent a better solution. :)> +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?Sure, it's unsigned now.> 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 > > _______________________________________________ > llvm-commits mailing list > llvm-commits at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits >-------------- next part -------------- A non-text attachment was scrubbed... Name: event-listener.patch Type: text/x-diff Size: 34384 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090624/18380581/attachment.patch>
Evan Cheng
2009-Jun-25 00:48 UTC
[LLVMdev] [llvm-commits] JITEventListener for eventual profiling and maybe gdb support
Thanks. Please commit. Owen, just a FYI about __jitSymbolTable. I thought you should care because of your thread safe work. Evan On Jun 24, 2009, at 5:43 PM, Jeffrey Yasskin wrote:> On Wed, Jun 24, 2009 at 2:43 PM, Evan Cheng<evan.cheng at apple.com> > wrote: >> 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. > > Done, although this is just a move. > >> + >> +// 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. > > As Eric said, this is just moving existing code, and it's #ifdef'ed > out. I'll let the Shark folks invent a better solution. :) > >> +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? > > Sure, it's unsigned now. > >> 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 >> >> _______________________________________________ >> llvm-commits mailing list >> llvm-commits at cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits >> > <event-listener.patch>_______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Maybe Matching Threads
- [LLVMdev] [llvm-commits] JITEventListener for eventual profiling and maybe gdb support
- [LLVMdev] JITEventListener for eventual profiling and maybe gdb support
- [LLVMdev] JITEventListener for eventual profiling and maybe gdb support
- [LLVMdev] Removing old JIT CodeEmitters for ARM and PPC
- [LLVMdev] Removing old JIT CodeEmitters for ARM and PPC