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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inline comments and let me know what you think.



================
Comment at: include/lldb/Utility/ArchSpec.h:91-92
 
+  // ARC configuration flags
+  enum ARCflags { eARC_rf32 = 0 /*0b00*/, eARC_rf16 = 2 /*0b10*/ };
+
----------------
Since no other place needs these flags, it would be nice to define them in ProcessGDBRemote.cpp and remove them from here? Eventually we should get rid of all flags in here and move then to the Architecture.h subclass headers.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4549
+    // The target is configured to use reduced register file.
+    arch_to_use.SetFlags(ArchSpec::eARC_rf16);
+    // ABI uses this information to determine how many registers it may
----------------
Do we need to set this flags in the arch_to_use still? The eARC_rf16 only seems to be used in this file. If you plan on using eARC_rf16 in other parts of the code, then we should define it in ArchSpec as you have it, otherwise we should remove it and avoid polluting that header if possible 


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4564
+    if (LLDB_INVALID_REGNUM == reg_num) {
+      if (ArchSpec::eARC_rf16 & arch.GetFlags()) {
+      // In reduced mode we don't have r4-r9, r16-r25.
----------------
the rf16 could be passed into this function as an argument and then the enum can be removed from ArchSpec.h?


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4704-4707
+        if (!arc::ConfigureArchitecture(*this, m_register_info, arch_to_use))
+          return false;
+
+        arc::AdjustRegisterInfo(m_register_info, arch_to_use);
----------------
This logic still seems possibly incorrect as we will skip the finalize call below? Shouldn't this just be:

```
if (arc::ConfigureArchitecture(*this, m_register_info, arch_to_use))
  arc::AdjustRegisterInfo(m_register_info, arch_to_use);
```
And fall through to below?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55718