[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:119-123
+  virtual std::vector<ConstString>
+  ConfigurationRegisterNames() const { return {}; }
+
+  using ConfigurationRegisterValues = std::map<ConstString, uint32_t>;
+  virtual bool SetFlagsFrom(const ConfigurationRegisterValues &) { return true; }
----------------
Any reason these definitions need to be in this class? Seems like something the plug-ins that access these should do, and this code shouldn't be in this file.


================
Comment at: include/lldb/Core/Architecture.h:125
+
+  virtual bool TestFlag(Flags::ValueType bit) const { return false; }
+
----------------
We should add a "Flags m_flags" as an instance variable and just provide accessors:

```
class Architecture {
  Flags GetFlags() { return m_flags; }
  const Flags GetFlags() const { return m_flags; }
};
```


================
Comment at: include/lldb/Core/Architecture.h:127
+
+  virtual bool MatchFlags(const ArchSpec &spec) const { return true; }
+
----------------
It is not clear what this function does and it doesn't seem like a function that belongs on this class.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:44-71
+bool ArchitectureArc::SetFlagsFrom(const ConfigurationRegisterValues &regs) {
+  ConstString rf_build("rf_build");
+  auto it = regs.find(rf_build);
+  if (it == regs.end())
+    return false;
+
+  // RF_BUILD "Number of Entries" bit.
----------------
This entire function's contents should be placed where it is called and not in the Architecture class.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:73-75
+bool ArchitectureArc::TestFlag(Flags::ValueType bit) const {
+  return m_flags.Test(bit);
+}
----------------
Remove and use accessor to m_flags.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:77-79
+std::vector<ConstString> ArchitectureArc::ConfigurationRegisterNames() const {
+  return { ConstString("rf_build"), ConstString("isa_config") };
+}
----------------
Remove from this class and inline at call site.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:81-86
+bool ArchitectureArc::MatchFlags(const ArchSpec &spec) const {
+  return ((spec.GetByteOrder() == eByteOrderBig) ==
+         m_flags.Test(arc_features::big_endian)) &&
+         ((spec.GetAddressByteSize() == 4) ==
+         m_flags.Test(arc_features::address_size_32));
+}
----------------
Remove from this class and inline at call site.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.h:36-42
+  std::vector<ConstString> ConfigurationRegisterNames() const override;
+
+  bool SetFlagsFrom(const ConfigurationRegisterValues &) override;
+
+  bool TestFlag(Flags::ValueType bit) const override;
+
+  bool MatchFlags(const ArchSpec &spec) const override;
----------------
These four functions don't seem like they belong on the Architecture class. It would be best to just have the code that calls these methods inline the code where needed.


================
Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.h:48
+
+  lldb_private::Flags m_flags;
+};
----------------
I would put this in the base class and then add just an accessor:

```
class Architecture {
  Flags GetFlags() { return m_flags; }
  const Flags GetFlags() const { return m_flags; }
};
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55718