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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Target/Target.h:1217
+    const ArchSpec &GetSpec() const { return m_spec; }
+    Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
----------------
zturner wrote:
> Can this return a reference instead of a pointer?
It can't, because the architecture plugin is not always present (currently we only have the arm one, and I currently don't see a use for others). We could consider a no-op plugin, which gets used if no specific plugin is present. This would even avoid the need for this class, as then the plugin could be responsible for returning the archspec.


================
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+  ConstString name;
+  std::string description;
+  PluginManager::ArchitectureCreateInstance create_callback;
----------------
zturner wrote:
> Why do we need a `ConstString` and a `std::string` here?  I don't think any plugin ever has a dynamically generated name or description, they exclusively originate from string literals.  So, with that in mind, can both of these just be `StringRef`?
They, could be, but I don't see a way to enforce that the names come from string literals, which makes me wonder if the usage of StringRef is a win in this case...

IIRC, the main reason I made this ConstString (instead of std::string, like the description), is that this name eventually makes it's way to the PluginInterface virtual method (which cannot be changed without touching every plugin).


================
Comment at: source/Core/PluginManager.cpp:318-323
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  for (const auto &instances : GetArchitectureInstances()) {
+    if (auto plugin_up = instances.create_callback(arch))
+      return plugin_up;
+  }
+  return nullptr;
----------------
zturner wrote:
> These mutexes have always bothered me.  Are people really registering and unregistering plugins dynamically?  It seems to me all of this always happens at debugger startup.  Are the mutexes necessary?
I'm pretty sure they aren't needed. I was just following what the other plugins do.


https://reviews.llvm.org/D31172