[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 a comment.
This revision now requires changes to proceed.

It would be nice to see context when you submit diffs. with SVN:

  svn diff -x -U9999999 ...

Or git:

  git diff -U9999999



================
Comment at: include/lldb/Core/Architecture.h:15
 
+class DynamicRegisterInfo;
+
----------------
Is DynamicRegisterInfo really in the top level namespace?


================
Comment at: include/lldb/Core/Architecture.h:18
 namespace lldb_private {
+class ArchSpec;
 
----------------
include the lldb-forward.h and remove this


================
Comment at: include/lldb/Utility/ArchSpec.h:91-92
 
+  // ARC configuration flags
+  enum ARCflags { eARC_rf32 = 0 /*0b00*/, eARC_rf16 = 2 /*0b10*/ };
+
----------------
It would be great to find a way that ArchSpec.h doesn't get polluted with everyone's flags by putting the flag definitions in the Architecture.h file. Not mandatory for this patch, but something to think about.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:76-93
+void ArchitectureArc::AdjustRegisterInfo(DynamicRegisterInfo &reg_info) const {
+  auto reg_count = reg_info.GetNumRegisters();
+  decltype(reg_count) dwarf_index = 0;
+  for (decltype(reg_count) i = 0; i < reg_count; ++i) {
+    auto &reg = *reg_info.GetRegisterInfoAtIndex(i);
+    auto &reg_num = reg.kinds[eRegisterKindDWARF];
+    if (LLDB_INVALID_REGNUM == reg_num) {
----------------
What is this code doing? Please add comments. Also, you have a ConfigureArchitecture in ProcessGDBRemote.cpp, can't you just do this work in that function and avoid the need to add Architecture::AdjustRegisterInfo() in the first place? And if so, is the architecture plug-in even needed?


================
Comment at: source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:625
 
+    case llvm::Triple::arc:
+      {
----------------
Context would really help here to see the surrounding code.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4530-4541
+  const RegisterInfo *rf_build_info = nullptr;
+
+  const auto reg_count = dyn_reg_info.GetNumRegisters();
+  // At least 16 first registers are not BCRs.
+  decltype(reg_count) skip_gpr = 16u;
+  for (auto reg_num = skip_gpr; reg_num < reg_count; ++reg_num) {
+    const auto reg_info = dyn_reg_info.GetRegisterInfoAtIndex(reg_num);
----------------
Make:
```
const lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(const lldb_private::ConstString &reg_name) const;
```
public and call it. No need to duplicate the name search here.. Code will be:
```
const RegisterInfo *rf_build_info = dyn_reg_info.GetRegisterInfo(reg_name);
```


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4683-4691
+      if (llvm::Triple::arc == arch_to_use.GetMachine()) {
+        if (!ConfigureArchitecture(*this, m_register_info, arch_to_use))
+          return false;        
+        GetTarget().SetArchitecture(arch_to_use);
       }
+
+      auto arch_plugin = GetTarget().GetArchitecturePlugin();
----------------
This code seems like it could be cleaned up. 
- ConfigureArchitecture should be named ConfigureARCArchitecture if it truly is ARC specfic
- why doesn't ConfigureArchitecture call GetTarget().SetArchitecture(arch_to_use) itself?
- Do we really want to return if ConfigureArchitecture() returns false? Do we not want to adjust the register info?
- why doesn't ConfigureArchitecture just fix up the dynamic register info and avoid the call to AdjustRegisterInfo in an architecture plug-in



================
Comment at: source/Target/Platform.cpp:1872-1876
+  case llvm::Triple::arc: {
+    static const uint8_t g_hex_opcode[] = { 0xff, 0x7f };
+    trap_opcode = g_hex_opcode;
+    trap_opcode_size = sizeof(g_hex_opcode);
+  } break;
----------------
The software breakpoint opcode would be a great candidate to move to the Architecture plug-ins. Not needed for this patch since this is how is has been done for all architectures up till now, but just noting this here.


================
Comment at: source/Target/Target.cpp:70
     : m_spec(spec),
-      m_plugin_up(PluginManager::CreateArchitectureInstance(spec)) {}
+      m_plugin_up(PluginManager::CreateArchitectureInstance(m_spec)) {}
 
----------------
is this change needed?


================
Comment at: source/Target/Target.cpp:74
   m_spec = spec;
-  m_plugin_up = PluginManager::CreateArchitectureInstance(spec);
+  m_plugin_up = PluginManager::CreateArchitectureInstance(m_spec);
   return *this;
----------------
is this change needed?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55718