Hi Jim, just pinging this again to see if you've had a chance to take a
quick look at the ModulesDidLoad patch, let me know if you'd prefer I
submit this as a general patch for someone else to review. Thanks!
On Fri, Mar 7, 2014 at 10:41 AM, Andrew MacPherson <andrew.m...@gmail.com>wrote:
> Hi Jim,
>
> I've attached a patch here that does what you suggest, I created a
> ModulesDidLoad() on both the Process and the JITLoader and use this now to
> only search the new modules for necessary JIT symbols. This works great and
> startup time does seem faster on programs with large shared libraries.
>
> I didn't make ModulesDidLoad() on the Process virtual for now as there was
> no need for it in this particular case but if you'd prefer I do it anyway
> just let me know. I also left out SymbolsDidLoad() as I'd prefer to have a
> use case that I can test with it and I don't right now, I will certainly
> keep it in mind for later though.
>
> This new patch also does away with the need for the patch I sent yesterday
> as that code (and the crashing code you hit) has been replaced with this
> new method. I'm not sure what the status of building the JITLoaderGDB on
> OSX is right now, I haven't made any commits since the initial patch commit.
>
> Thanks for the help!
>
> Andrew
>
>
>
> On Thu, Mar 6, 2014 at 7:10 PM, <jing...@apple.com> wrote:
>
>> Andrew,
>>
>> I'll have to check out this patch when I get back home. You very nicely
>> turned off the JITLoaderGDB for MacOS X, but now that I've updated, that
>> means I can't tell whether your patch fixes the crash :-( I'm in the
>> middle of something else so I don't want to have to mess with this right
>> now, but my checkout at home is still in the state it was last night, so
>> I'll check it out there.
>>
>> We don't currently have a pluggable mechanism for watching shared library
>> loads, but there is a place where code belonging to Target & Process can
>> observe these changes synchronously. That place is Target::ModulesDidLoad.
>> Currently it handles the breakpoint resetting, and some work that Jason
>> needed to do for the "SystemRuntime plugin". Since your JITLoader is also
>> internal to the process, it would be fine for you to hook in there and look
>> for your symbol. I don't thing we need to come up with some generic
>> mechanism for this yet.
>>
>> However, when we had only one process thing doing a job in
>> Target::ModulesDidLoad it was kinda okay to check for m_process_sp and do
>> what is arguably process related work there:
>>
>> void
>> Target::ModulesDidLoad (ModuleList &module_list)
>> {
>> if (module_list.GetSize())
>> {
>> m_breakpoint_list.UpdateBreakpoints (module_list, true, false);
>> if (m_process_sp)
>> {
>> SystemRuntime *sys_runtime = m_process_sp->GetSystemRuntime();
>> if (sys_runtime)
>> {
>> sys_runtime->ModulesDidLoad (module_list);
>> }
>> }
>> // TODO: make event data that packages up the module_list
>> BroadcastEvent (eBroadcastBitModulesLoaded, NULL);
>> }
>> }
>>
>> but now that there are two process related jobs, we should break this out
>> and make a Process::ModulesDidLoad and do the process related stuff there.
>> If you don't mind splitting this off, then feel free to do so and put your
>> symbol search there rather than in the stop hook.
>>
>> If the work you have to do is applicable to generic processes there's no
>> reason to make Process::ModulesDidLoad virtual, but that might arguably be
>> useful. Since there is Process generic work that is done here as well (the
>> SystemRuntime call) you'll have to deal with how partition the generic &
>> overridable behaviors.
>>
>> We have two patterns for structuring a task that requires some generic
>> work and some subclass, related work in lldb. In some places we just
>> document in the base class method that you have to call the base class
>> method in your derived method. Generally that's in code that aren't going
>> to have a lot of folks touching it, so you can trust that whoever changes
>> it will read the headers first. In other places we have a non-virtual Task
>> method, and a virtual DoTask method, then the Task method does the generic
>> stuff and calls DoTask for the virtual part of the task. The latter is
>> cleaner though it adds another method to the internal API. Anyway, if it
>> makes sense to have this Process::ModulesDidLoad be virtual, feel free to
>> do this either way...
>>
>> You might also want to hook into SymbolsDidLoad, which gets called when a
>> symbol file gets added to a binary already loaded in the process. That
>> would catch the case where your symbol was stripped from the binary, but
>> somebody added a symbol file that contained the symbol mid-way through the
>> debug run.
>>
>> Jim
>>
>> On Mar 6, 2014, at 1:52 AM, Andrew MacPherson <andrew.m...@gmail.com>
>> wrote:
>>
>> > Hi Jim,
>> >
>> > I agree that this part of the patch isn't pretty but I wasn't able to
>> find a way to be notified when a shared library is loaded (exactly as you
>> suggest). We need to check on launch or attach and then whenever a new
>> library is loaded, if there's a better way to do that it would be great to
>> clean this up.
>> >
>> > I can't reproduce the crash on Linux but it looks like we need to
>> unregister the notification callback that's set, that's the only reason I
>> can think of that you would end up in the ProcessStateChangedCallback with
>> an invalid baton pointer. I've attached a patch that I'm hoping will
>> resolve the issue here, it's definitely a bug that it wasn't being
>> unregistered.
>> >
>> > I hadn't intended for the JITLoaderGDB to be enabled on OSX as I wasn't
>> able to test it there, sorry about that. It's likely not useful there right
>> now as it would need modifications to the ObjectFileMachO to do some
>> relocations, and additionally MCJIT in LLVM would need to be modified to
>> call the GDB hook on OSX (though other JIT hosts may work). That said it
>> shouldn't be causing crashes if enabled.
>> >
>> > Thanks for looking into this.
>> >
>> > Cheers,
>> > Andrew
>> >
>> >
>> >
>> > On Thu, Mar 6, 2014 at 4:28 AM, Jim Ingham <jing...@apple.com> wrote:
>> > This part of the patch worries me. If I am debugging a process that
>> doesn't have this JIT loader symbol, this bit means every time that the
>> process stops for any reason you will search the whole world for some
>> symbol that won't be found. That's something we really avoid doing if we
>> can, programs get pretty big and this is not the sort of thing you want to
>> do.
>> >
>> > I don't know how this symbol comes about, is there no event (shared
>> library load or something) that you can hook into to find this symbol?
>> >
>> > This patch is also causing a crash on Mac OS X just running a program.
>> The crash looks like:
>> >
>> > (lldb) bt
>> > * thread #8: tid = 0xf68e5, name =
>> <lldb.process.internal-state(pid=40372)>, function:
>> lldb_private::Process::GetTarget() , stop reason = EXC_BAD_ACCESS (code=1,
>> address=0x100)
>> > frame #0: 0x0000000106f8072c
>> LLDB`lldb_private::Process::GetTarget() at Process.h:2516
>> > frame #1: 0x0000000108e7aa5a
>> LLDB`JITLoaderGDB::GetSymbolAddress(lldb_private::ConstString const&,
>> lldb::SymbolType) const at JITLoaderGDB.cpp:368
>> > frame #2: 0x0000000108e7a8bf LLDB`JITLoaderGDB::SetJITBreakpoint()
>> at JITLoaderGDB.cpp:99
>> > frame #3: 0x0000000108e7a6d8
>> LLDB`JITLoaderGDB::ProcessStateChangedCallback(void*,
>> lldb_private::Process*, lldb::StateType) at JITLoaderGDB.cpp:354
>> > frame #4: 0x0000000108b7b29b
>> LLDB`lldb_private::Process::SynchronouslyNotifyStateChanged(lldb::StateType)
>> at Process.cpp:1223
>> > frame #5: 0x0000000108b89762
>> LLDB`lldb_private::Process::ShouldBroadcastEvent(lldb_private::Event*) at
>> Process.cpp:3846
>> > frame #6: 0x0000000108b8454d
>> LLDB`lldb_private::Process::HandlePrivateEvent(std::__1::shared_ptr<lldb_private::Event>&)
>> at Process.cpp:4141
>> > frame #7: 0x0000000108b8a755
>> LLDB`lldb_private::Process::RunPrivateStateThread() at Process.cpp:4290
>> > frame #8: 0x0000000108b89bfd
>> LLDB`lldb_private::Process::PrivateStateThread(void*) at Process.cpp:4221
>> > frame #9: 0x00000001087d811a LLDB`ThreadCreateTrampoline(void*) at
>> Host.cpp:629
>> > frame #10: 0x00007fff815df899 libsystem_pthread.dylib`_pthread_body
>> > frame #11: 0x00007fff815df72a libsystem_pthread.dylib`_pthread_start
>> > frame #12: 0x00007fff815e3fc9 libsystem_pthread.dylib`thread_start
>> > (lldb) f 2
>> > frame #2: 0x0000000108e7a8bf LLDB`JITLoaderGDB::SetJITBreakpoint() at
>> JITLoaderGDB.cpp:99
>> > 96 log->Printf("JITLoaderGDB::%s looking for JIT
>> register hook",
>> > 97 __FUNCTION__);
>> > 98
>> > -> 99 addr_t jit_addr =
>> GetSymbolAddress(ConstString("__jit_debug_register_code"),
>> > 100 eSymbolTypeAny);
>> > 101 if (jit_addr == LLDB_INVALID_ADDRESS)
>> > 102 return;
>> > (lldb) expr *this
>> > (JITLoaderGDB) $13 = {
>> > lldb_private::JITLoader = {
>> > m_process = 0x0000000000000000 Public:
>> lldb_private::ThreadSafeValue<lldb::StateType> @ Private:
>> lldb_private::ThreadSafeValue<lldb::StateType> @
>> > }
>> > m_jit_objects = size=160215376 {
>> > [0] = {
>> > first = <parent is NULL>
>> > second = <parent is NULL>
>> > }
>> > ...
>> > }
>> > m_jit_break_id = 0
>> > m_notification_callbacks = {
>> > baton = 0x0000000000000001
>> > initialize = 0x00007fc54b00f3b0
>> > process_state_changed = 0x00000001098cb150 (vtable for
>> std::__1::__shared_ptr_pointer<lldb_private::Section*,
>> std::__1::default_delete<lldb_private::Section>,
>> std::__1::allocator<lldb_private::Section> > + 16)
>> > }
>> > }
>> >
>> > Looks like the JIT instance that is getting passed in is not good for
>> some reason.
>> >
>> > Jim
>> >
>> >
>> > On Mar 5, 2014, at 2:12 AM, Andrew MacPherson <andrew.m...@gmail.com>
>> wrote:
>> >
>> >> +void
>> >> +JITLoaderGDB::ProcessStateChangedCallback(void *baton,
>> >> + lldb_private::Process
>> *process,
>> >> + lldb::StateType state)
>> >> +{
>> >> + JITLoaderGDB* instance = static_cast<JITLoaderGDB*>(baton);
>> >> +
>> >> + switch (state)
>> >> + {
>> >> + case eStateConnected:
>> >> + case eStateAttaching:
>> >> + case eStateLaunching:
>> >> + case eStateInvalid:
>> >> + case eStateUnloaded:
>> >> + case eStateExited:
>> >> + case eStateDetached:
>> >> + // instance->Clear(false);
>> >> + break;
>> >> +
>> >> + case eStateRunning:
>> >> + case eStateStopped:
>> >> + // Keep trying to set our JIT breakpoint each time we stop
>> until we
>> >> + // succeed
>> >> + if (!instance->DidSetJITBreakpoint() && process->IsAlive())
>> >> + instance->SetJITBreakpoint();
>> >> + break;
>> >> +
>> >> + case eStateStepping:
>> >> + case eStateCrashed:
>> >> + case eStateSuspended:
>> >> + break;
>> >> + }
>> >> +}
>> >> +
>> >
>> >
>> > <jit-unregister-notification.patch>
>>
>>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
