[Lldb-commits] [PATCH] D55718: [ARC] Basic support in gdb-remote process plugin

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/lldb/Core/Architecture.h:118-127
+  static constexpr uint8_t max_features_count = 32u;
+  virtual std::bitset<max_features_count> GetFeatures() const { return 0; }
+
+  using ReadRegister = std::function<llvm::Optional<RegisterValue>(
+      ConstString /*name*/, bool /*case_sensitive*/)>;
+  virtual bool SetFeatures(const ReadRegister &func) { return true; }
+
----------------
We currently have been using the lldb_private::Flags class for this kind of stuff. Best to use that here. 


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:29
+  PluginManager::RegisterPlugin(GetPluginNameStatic(),
+                                "ARC-specific algorithms",
+                                &ArchitectureArc::Create);
----------------
Not a great human readable architecture name here. All other plug-ins use the short architecture name ("arm", "mipc", "ppc64"). Best to just use "arc"?


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:46-78
+bool ArchitectureArc::SetFeatures(const ReadRegister &func) {
+
+  auto set_feature = [&func, this](
+      ConstString reg_name, uint_least32_t field_mask, size_t feature_position)
+      -> llvm::Optional<uint64_t> {
+    const bool case_sensitive = false;
+    auto opt_value = func(reg_name, case_sensitive);
----------------
All this code belongs elsewhere. SetFeatures probably needs to be changed to be: Architecture::GetFlags().XXX() where XXX is a method on the lldb_private::Flags class, so this code should go where the code that was calling it was. Kind of weird to have  register reading function passed in.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.h:32-40
+  std::bitset<max_features_count> GetFeatures() const override {
+    return m_features;
+  }
+
+  bool SetFeatures(const ReadRegister &func) override;
+
+  bool MatchFeatures(const ArchSpec &spec) const override;
----------------
Use lldb_private::Flags


================
Comment at: source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:642-669
+          switch (reg.kinds[eRegisterKindDWARF]) {
+          case 0:
+            reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_ARG1;
+            break;
+          case 1:
+            reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_ARG2;
+            break;
----------------
Magic numbers? Can we use enums?


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1222-1223
+      
+      if (!arch_plugin->SetFeatures(reg_reader) && log)
+        log->PutCString("Failed to set architecture features");
+
----------------
All this work should be done in this function then use arch_plug->GetFlags().Set(mask);


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55718/new/

https://reviews.llvm.org/D55718