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

Wed Dec 19 02:45:54 PST 2018

tatyana-krasnukha marked an inline comment as not done.
tatyana-krasnukha added a comment.

`ARCflags` are used by ABISysV_arc (related patch D55724 <https://reviews.llvm.org/D55724>). I would be glad to move it to architecture plugin, but I ought to add SetFlags/GetFlags to Architecture interface in this case. Then we'll have the same members in ArchSpec and in Architecture, that may look confusing.

================
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);
----------------
clayborg wrote:
> 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?
If we cannot configure the architecture it is a failure and the register info is incorrect, so just return false. Maybe should clear `m_register_info`  in this case...

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55718