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
