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>
>
>
diff --git a/include/lldb/Target/JITLoader.h b/include/lldb/Target/JITLoader.h
index 890afb1..bc520e1 100644
--- a/include/lldb/Target/JITLoader.h
+++ b/include/lldb/Target/JITLoader.h
@@ -56,7 +56,7 @@ public:
//------------------------------------------------------------------
/// Called after attaching a process.
///
- /// Allow DynamicLoader plug-ins to execute some code after
+ /// Allow JITLoader plug-ins to execute some code after
/// attaching to a process.
//------------------------------------------------------------------
virtual void
@@ -65,12 +65,19 @@ public:
//------------------------------------------------------------------
/// Called after launching a process.
///
- /// Allow DynamicLoader plug-ins to execute some code after
+ /// Allow JITLoader plug-ins to execute some code after
/// the process has stopped for the first time on launch.
//------------------------------------------------------------------
virtual void
DidLaunch () = 0;
+ //------------------------------------------------------------------
+ /// Called after a new shared object has been loaded so that it can
+ /// be probed for JIT entry point hooks.
+ //------------------------------------------------------------------
+ virtual void
+ ModulesDidLoad (lldb_private::ModuleList &module_list) = 0;
+
protected:
//------------------------------------------------------------------
// Member variables.
diff --git a/include/lldb/Target/JITLoaderList.h b/include/lldb/Target/JITLoaderList.h
index dea3a6f..f933a61 100644
--- a/include/lldb/Target/JITLoaderList.h
+++ b/include/lldb/Target/JITLoaderList.h
@@ -47,6 +47,9 @@ public:
void
DidAttach();
+ void
+ ModulesDidLoad (ModuleList &module_list);
+
private:
std::vector<lldb::JITLoaderSP> m_jit_loaders_vec;
lldb_private::Mutex m_jit_loaders_mutex;
diff --git a/include/lldb/Target/Process.h b/include/lldb/Target/Process.h
index a79b3b4..b40a02e 100644
--- a/include/lldb/Target/Process.h
+++ b/include/lldb/Target/Process.h
@@ -2575,6 +2575,9 @@ public:
void
SendAsyncInterrupt ();
+ void
+ ModulesDidLoad (ModuleList &module_list);
+
protected:
void
diff --git a/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp b/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
index 97d6258..e6a6d07 100644
--- a/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ b/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -55,38 +55,43 @@ struct jit_descriptor
JITLoaderGDB::JITLoaderGDB (lldb_private::Process *process) :
JITLoader(process),
m_jit_objects(),
- m_jit_break_id(LLDB_INVALID_BREAK_ID)
+ m_jit_break_id(LLDB_INVALID_BREAK_ID),
+ m_jit_descriptor_addr(LLDB_INVALID_ADDRESS)
{
- m_notification_callbacks.baton = this;
- m_notification_callbacks.initialize = nullptr;
- m_notification_callbacks.process_state_changed =
- ProcessStateChangedCallback;
- m_process->RegisterNotificationCallbacks(m_notification_callbacks);
}
JITLoaderGDB::~JITLoaderGDB ()
{
if (LLDB_BREAK_ID_IS_VALID(m_jit_break_id))
m_process->GetTarget().RemoveBreakpointByID (m_jit_break_id);
- m_jit_break_id = LLDB_INVALID_BREAK_ID;
- m_process->UnregisterNotificationCallbacks(m_notification_callbacks);
}
void JITLoaderGDB::DidAttach()
{
- SetJITBreakpoint();
+ Target &target = m_process->GetTarget();
+ ModuleList &module_list = target.GetImages();
+ SetJITBreakpoint(module_list);
}
void JITLoaderGDB::DidLaunch()
{
- SetJITBreakpoint();
+ Target &target = m_process->GetTarget();
+ ModuleList &module_list = target.GetImages();
+ SetJITBreakpoint(module_list);
+}
+
+void
+JITLoaderGDB::ModulesDidLoad(ModuleList &module_list)
+{
+ if (!DidSetJITBreakpoint() && m_process->IsAlive())
+ SetJITBreakpoint(module_list);
}
//------------------------------------------------------------------
// Setup the JIT Breakpoint
//------------------------------------------------------------------
void
-JITLoaderGDB::SetJITBreakpoint()
+JITLoaderGDB::SetJITBreakpoint(lldb_private::ModuleList &module_list)
{
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_JIT_LOADER));
@@ -97,11 +102,22 @@ JITLoaderGDB::SetJITBreakpoint()
log->Printf("JITLoaderGDB::%s looking for JIT register hook",
__FUNCTION__);
- addr_t jit_addr = GetSymbolAddress(ConstString("__jit_debug_register_code"),
- eSymbolTypeAny);
+ addr_t jit_addr = GetSymbolAddress(
+ module_list, ConstString("__jit_debug_register_code"), eSymbolTypeAny);
if (jit_addr == LLDB_INVALID_ADDRESS)
return;
+ m_jit_descriptor_addr = GetSymbolAddress(
+ module_list, ConstString("__jit_debug_descriptor"), eSymbolTypeData);
+ if (m_jit_descriptor_addr == LLDB_INVALID_ADDRESS)
+ {
+ if (log)
+ log->Printf(
+ "JITLoaderGDB::%s failed to find JIT descriptor address",
+ __FUNCTION__);
+ return;
+ }
+
if (log)
log->Printf("JITLoaderGDB::%s setting JIT breakpoint",
__FUNCTION__);
@@ -131,26 +147,18 @@ JITLoaderGDB::JITDebugBreakpointHit(void *baton,
bool
JITLoaderGDB::ReadJITDescriptor(bool all_entries)
{
+ if (m_jit_descriptor_addr == LLDB_INVALID_ADDRESS)
+ return false;
+
Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_JIT_LOADER));
Target &target = m_process->GetTarget();
- ModuleList &images = target.GetImages();
-
- addr_t jit_addr = GetSymbolAddress(ConstString("__jit_debug_descriptor"),
- eSymbolTypeData);
- if (jit_addr == LLDB_INVALID_ADDRESS)
- {
- if (log)
- log->Printf(
- "JITLoaderGDB::%s failed to find JIT descriptor address",
- __FUNCTION__);
- return false;
- }
+ ModuleList &module_list = target.GetImages();
jit_descriptor jit_desc;
const size_t jit_desc_size = sizeof(jit_desc);
Error error;
- size_t bytes_read =
- m_process->DoReadMemory(jit_addr, &jit_desc, jit_desc_size, error);
+ size_t bytes_read = m_process->DoReadMemory(
+ m_jit_descriptor_addr, &jit_desc, jit_desc_size, error);
if (bytes_read != jit_desc_size || !error.Success())
{
if (log)
@@ -209,7 +217,7 @@ JITLoaderGDB::ReadJITDescriptor(bool all_entries)
// load the symbol table right away
module_sp->GetObjectFile()->GetSymtab();
- images.AppendIfNeeded(module_sp);
+ module_list.AppendIfNeeded(module_sp);
ModuleList module_list;
module_list.Append(module_sp);
@@ -251,7 +259,7 @@ JITLoaderGDB::ReadJITDescriptor(bool all_entries)
}
}
}
- images.Remove(module_sp);
+ module_list.Remove(module_sp);
m_jit_objects.erase(it);
}
}
@@ -328,48 +336,15 @@ JITLoaderGDB::DidSetJITBreakpoint() const
return LLDB_BREAK_ID_IS_VALID(m_jit_break_id);
}
-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;
- }
-}
-
addr_t
-JITLoaderGDB::GetSymbolAddress(const ConstString &name, SymbolType symbol_type) const
+JITLoaderGDB::GetSymbolAddress(ModuleList &module_list, const ConstString &name,
+ SymbolType symbol_type) const
{
SymbolContextList target_symbols;
Target &target = m_process->GetTarget();
- ModuleList &images = target.GetImages();
- if (!images.FindSymbolsWithNameAndType(name, symbol_type, target_symbols))
+ if (!module_list.FindSymbolsWithNameAndType(name, symbol_type,
+ target_symbols))
return LLDB_INVALID_ADDRESS;
SymbolContext sym_ctx;
diff --git a/source/Plugins/JITLoader/GDB/JITLoaderGDB.h b/source/Plugins/JITLoader/GDB/JITLoaderGDB.h
index 15b54e3..5fa66b8 100644
--- a/source/Plugins/JITLoader/GDB/JITLoaderGDB.h
+++ b/source/Plugins/JITLoader/GDB/JITLoaderGDB.h
@@ -63,13 +63,17 @@ public:
virtual void
DidLaunch ();
+ virtual void
+ ModulesDidLoad (lldb_private::ModuleList &module_list);
+
private:
lldb::addr_t
- GetSymbolAddress(const lldb_private::ConstString &name,
+ GetSymbolAddress(lldb_private::ModuleList &module_list,
+ const lldb_private::ConstString &name,
lldb::SymbolType symbol_type) const;
void
- SetJITBreakpoint();
+ SetJITBreakpoint(lldb_private::ModuleList &module_list);
bool
DidSetJITBreakpoint() const;
@@ -93,7 +97,7 @@ private:
JITObjectMap m_jit_objects;
lldb::user_id_t m_jit_break_id;
- lldb_private::Process::Notifications m_notification_callbacks;
+ lldb::addr_t m_jit_descriptor_addr;
};
diff --git a/source/Target/JITLoaderList.cpp b/source/Target/JITLoaderList.cpp
index a3b24c4..24a73b7 100644
--- a/source/Target/JITLoaderList.cpp
+++ b/source/Target/JITLoaderList.cpp
@@ -67,3 +67,11 @@ JITLoaderList::DidAttach()
for (auto const &jit_loader : m_jit_loaders_vec)
jit_loader->DidAttach();
}
+
+void
+JITLoaderList::ModulesDidLoad(ModuleList &module_list)
+{
+ Mutex::Locker locker(m_jit_loaders_mutex);
+ for (auto const &jit_loader : m_jit_loaders_vec)
+ jit_loader->ModulesDidLoad(module_list);
+}
diff --git a/source/Target/Process.cpp b/source/Target/Process.cpp
index 793cdec..75443f7 100644
--- a/source/Target/Process.cpp
+++ b/source/Target/Process.cpp
@@ -3068,7 +3068,7 @@ Process::Launch (ProcessLaunchInfo &launch_info)
if (dyld)
dyld->DidLaunch();
- // GetJITLoaders().DidLaunch();
+ GetJITLoaders().DidLaunch();
SystemRuntime *system_runtime = GetSystemRuntime ();
if (system_runtime)
@@ -3117,7 +3117,7 @@ Process::LoadCore ()
if (dyld)
dyld->DidAttach();
- //GetJITLoaders().DidAttach();
+ GetJITLoaders().DidAttach();
SystemRuntime *system_runtime = GetSystemRuntime ();
if (system_runtime)
@@ -3396,7 +3396,7 @@ Process::CompleteAttach ()
if (dyld)
dyld->DidAttach();
- // GetJITLoaders().DidAttach();
+ GetJITLoaders().DidAttach();
SystemRuntime *system_runtime = GetSystemRuntime ();
if (system_runtime)
@@ -6030,3 +6030,14 @@ Process::ResolveIndirectFunction(const Address *address, Error &error)
return function_addr;
}
+void
+Process::ModulesDidLoad (ModuleList &module_list)
+{
+ SystemRuntime *sys_runtime = GetSystemRuntime();
+ if (sys_runtime)
+ {
+ sys_runtime->ModulesDidLoad (module_list);
+ }
+
+ GetJITLoaders().ModulesDidLoad (module_list);
+}
diff --git a/source/Target/Target.cpp b/source/Target/Target.cpp
index 6235741..d759801 100644
--- a/source/Target/Target.cpp
+++ b/source/Target/Target.cpp
@@ -1172,11 +1172,7 @@ Target::ModulesDidLoad (ModuleList &module_list)
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);
- }
+ m_process_sp->ModulesDidLoad (module_list);
}
// TODO: make event data that packages up the module_list
BroadcastEvent (eBroadcastBitModulesLoaded, NULL);
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
