[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

[Greg Clayton via Phabricator via lldb-commits]

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