clayborg added a comment.
Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if the
arch is supported. Most of the time we will set this once and never need to
change it, even when we attach, change arch, etc. The new suggestion will allow
us to use the same arch plug-in without re-fetching it. Let me know what you
think as the changes are thrown out there to see what people's opinions are.
================
Comment at: include/lldb/Target/Target.h:916
+ Architecture *GetArchitecturePlugin() { return m_architecture_up.get(); }
+
----------------
We might want to make this lazy? Only fill it in if anyone asks. Another way to
ensure this stays up to date it to ask it if it supports the target's arch each
time.
```
Architecture *Target::GetArchitecturePlugin() {
if (m_architecture_up && !m_architecture_up->Supports(m_arch))
m_architecture_up.reset();
if (!m_architecture_up)
m_architecture_up = PluginManager::CreateArchitectureInstance(m_arch);
return m_architecture_up.get();
}
```
Then we never need to worry about clearing this or updating it when the arch
changes. MacOS is the typical place this will happen where the user types:
```
(lldb) file a.out
(lldb) attach 123
```
And where a.out has 2 architectures: i386 and x86_64. By default we will load
the 64 bit arch, but when we attach we might end up switching. Same thing for
exec. We have a mac test case for debugging through exec calls where we switch
between i386 and x86_64 a few times to ensure breakpoints correctly unresolve
and re-resolve.
================
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:22
+ConstString ArchitectureArm::GetPluginNameStatic() {
+ return ConstString("ArchitectureArm");
+}
----------------
Other plug-ins have typically stuck with lower case names and very simple. I
would suggest simply "arm" as the name. Each plug-in has its own namespace, so
the names can be easy and short.
================
Comment at: source/Target/Target.cpp:1253
m_arch = executable_sp->GetArchitecture();
+ m_architecture_up = PluginManager::CreateArchitectureInstance(m_arch);
if (log)
----------------
We can avoid this if we change Target::GetArchitecturePlugin as noted above.
================
Comment at: source/Target/Target.cpp:1318
m_arch = other;
+ m_architecture_up = PluginManager::CreateArchitectureInstance(m_arch);
+ }
----------------
We can avoid this if we change Target::GetArchitecturePlugin as noted above.
================
Comment at: source/Target/Target.cpp:1334
m_arch = other;
+ m_architecture_up = PluginManager::CreateArchitectureInstance(m_arch);
ModuleSP executable_sp = GetExecutableModule();
----------------
We can avoid this if we change Target::GetArchitecturePlugin as noted above.
https://reviews.llvm.org/D31172
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
