Sorry, got busy. This looks fine.
Jim On Mar 12, 2014, at 2:29 AM, Andrew MacPherson <[email protected]> wrote: > 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 <[email protected]> > 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, <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
