Simplify NativeProcessProtocol::GetArchitecture/GetByteOrder

[labath]

Summary

These functions used to return bool to signify whether they were able to
retrieve the data. This is redundant because the ArchSpec and ByteOrder
already have their own "invalid" states, *and* because both of the
current implementations (linux, netbsd) can always provide a valid
result.

This allows us to simplify bits of the code handling these values.

[eugene]

include/lldb/Host/common/NativeProcessProtocol.h

104 ↗

(On Diff #121903)

Why return reference instead of a value?

[labath]

include/lldb/Host/common/NativeProcessProtocol.h

104 ↗

(On Diff #121903)

I'd actually reverse that: Why value instead of a reference ? :D

Returning by reference allows us to potentially avoid a copy. It does mean that the process object has to hold the archspec as a member, but that is currently not an issue, and we can always change that.

[krytarowski]

The NetBSD part looks good.

[clayborg]

Looks fine. Seems like you should use your a const reference in a few places and this will be good to go?

source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

1036 ↗

(On Diff #121903)

Avoid a copy here by using a const reference?

source/Plugins/Process/NetBSD/NativeThreadNetBSD.cpp

147 ↗

(On Diff #121903)

Void a copy here and use a const reference?

[eugene]

include/lldb/Host/common/NativeProcessProtocol.h

104 ↗

(On Diff #121903)

Why value instead of a reference ? :D

IMO returning by value is a default option, for 2 reasons:

1. References and pointers introduce implicit lifetime dependencies (prone to use-after-free)
2. Value can change over time, and client might not expect it to happen.

Potentially avoid a copy in place where a copy was made previously and didn't seem to hurt anybody seems like a bad trade-off for one more place where we can get a dangling pointer.

[clayborg]

If it isn't expensive to copy, then we should probably just return by value.

[labath]

include/lldb/Host/common/NativeProcessProtocol.h

104 ↗

(On Diff #121903)

That's kinda true, but simply returning by reference does not force you to hold on to the reference everywhere. It just leaves the decision up to the caller -- if you put the result in a variable, you get a copy; if you put it in a reference (or just directly use it) then you don't. And if I see something like const ArchSpec &arch = ..., I will take a good look at what's the lifetime of the object this refers to.

I'm not what you base your "default" statement on, but it certainly does not seem to be the default in llvm. Just look at llvm::Triple::str() -- it returns a reference to the string object private member.

ArchSpec is 56 bytes long (on x86_64), and it contains a string (potential heap allocation), so it's not exactly cheap to copy. GetByteOrder() is called on every register read, so it would be great to avoid copying an ArchSpec just to read a register. I'm not saying this will make register reading blazingly fast, but I think it's a step in the right direction.

[labath]

Avoid a couple of ArchSpec copies, as suggested.

lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h

 //===-- NativeProcessProtocol.h ---------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_NativeProcessProtocol_h_
 #define liblldb_NativeProcessProtocol_h_
 
 #include "NativeBreakpointList.h"
 #include "NativeThreadProtocol.h"
 #include "NativeWatchpointList.h"
+#include "lldb/Core/ArchSpec.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/MainLoop.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/TraceOptions.h"
 #include "lldb/lldb-private-forward.h"
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
public:
   virtual Status DeallocateMemory(lldb::addr_t addr) = 0;
 
   virtual lldb::addr_t GetSharedLibraryInfoAddress() = 0;
 
   virtual bool IsAlive() const;
 
   virtual size_t UpdateThreads() = 0;
 
-  virtual bool GetArchitecture(ArchSpec &arch) const = 0;
+  virtual const ArchSpec &GetArchitecture() const = 0;
 
   //----------------------------------------------------------------------
   // Breakpoint functions
   //----------------------------------------------------------------------
   virtual Status SetBreakpoint(lldb::addr_t addr, uint32_t size,
                                bool hardware) = 0;
 
   virtual Status RemoveBreakpoint(lldb::addr_t addr, bool hardware = false);
public:
   bool IsRunning() const {
     return m_state == lldb::eStateRunning || IsStepping();
   }
 
   bool IsStepping() const { return m_state == lldb::eStateStepping; }
 
   bool CanResume() const { return m_state == lldb::eStateStopped; }
 
-  bool GetByteOrder(lldb::ByteOrder &byte_order) const;
+  lldb::ByteOrder GetByteOrder() const {
+    return GetArchitecture().GetByteOrder();
+  }
 
   virtual llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   GetAuxvData() const = 0;
 
   //----------------------------------------------------------------------
   // Exit Status
   //----------------------------------------------------------------------
   virtual llvm::Optional<WaitStatus> GetExitStatus();

lldb/trunk/source/Host/common/NativeProcessProtocol.cpp

 //===-- NativeProcessProtocol.cpp -------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/common/NativeProcessProtocol.h"
-
-#include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/State.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/common/NativeThreadProtocol.h"
 #include "lldb/Host/common/SoftwareBreakpoint.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/Process.h"
NativeThreadProtocol *NativeProcessProtocol::GetThreadByID(lldb::tid_t tid) {
   return GetThreadByIDUnlocked(tid);
 }
 
 bool NativeProcessProtocol::IsAlive() const {
   return m_state != eStateDetached && m_state != eStateExited &&
          m_state != eStateInvalid && m_state != eStateUnloaded;
 }
 
-bool NativeProcessProtocol::GetByteOrder(lldb::ByteOrder &byte_order) const {
-  ArchSpec process_arch;
-  if (!GetArchitecture(process_arch))
-    return false;
-  byte_order = process_arch.GetByteOrder();
-  return true;
-}
-
 const NativeWatchpointList::WatchpointMap &
 NativeProcessProtocol::GetWatchpointMap() const {
   return m_watchpoint_list.GetWatchpointMap();
 }
 
 llvm::Optional<std::pair<uint32_t, uint32_t>>
 NativeProcessProtocol::GetHardwareDebugSupportInfo() const {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));

lldb/trunk/source/Host/common/NativeRegisterContext.cpp

if (bytes_read != src_len) {
     return error;
   }
 
   // We now have a memory buffer that contains the part or all of the register
   // value. Set the register value using this memory data.
   // TODO: we might need to add a parameter to this function in case the byte
   // order of the memory data doesn't match the process. For now we are assuming
   // they are the same.
-  lldb::ByteOrder byte_order;
-  if (process.GetByteOrder(byte_order)) {
-    error.SetErrorString("NativeProcessProtocol::GetByteOrder () failed");
-    return error;
-  }
-
-  reg_value.SetFromMemoryData(reg_info, src, src_len, byte_order, error);
+  reg_value.SetFromMemoryData(reg_info, src, src_len, process.GetByteOrder(),
+                              error);
 
   return error;
 }
 
 Status NativeRegisterContext::WriteRegisterValueToMemory(
     const RegisterInfo *reg_info, lldb::addr_t dst_addr, size_t dst_len,
     const RegisterValue &reg_value) {
 
   uint8_t dst[RegisterValue::kMaxRegisterByteSize];
 
   Status error;
 
   NativeProcessProtocol &process = m_thread.GetProcess();
 
   // TODO: we might need to add a parameter to this function in case the byte
   // order of the memory data doesn't match the process. For now we are
   // assuming
   // they are the same.
-  lldb::ByteOrder byte_order;
-  if (!process.GetByteOrder(byte_order))
-    return Status("NativeProcessProtocol::GetByteOrder () failed");
-
-  const size_t bytes_copied =
-      reg_value.GetAsMemoryData(reg_info, dst, dst_len, byte_order, error);
+  const size_t bytes_copied = reg_value.GetAsMemoryData(
+      reg_info, dst, dst_len, process.GetByteOrder(), error);
 
   if (error.Success()) {
     if (bytes_copied == 0) {
       error.SetErrorString("byte copy failed.");
     } else {
       size_t bytes_written;
       error = process.WriteMemory(dst_addr, dst, bytes_copied, bytes_written);
       if (error.Fail())

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h

Status AllocateMemory(size_t size, uint32_t permissions,
                         lldb::addr_t &addr) override;
 
   Status DeallocateMemory(lldb::addr_t addr) override;
 
   lldb::addr_t GetSharedLibraryInfoAddress() override;
 
   size_t UpdateThreads() override;
 
-  bool GetArchitecture(ArchSpec &arch) const override;
+  const ArchSpec &GetArchitecture() const override { return m_arch; }
 
   Status SetBreakpoint(lldb::addr_t addr, uint32_t size,
                        bool hardware) override;
 
   Status RemoveBreakpoint(lldb::addr_t addr, bool hardware = false) override;
 
   void DoStopIDBumped(uint32_t newBumpId) override;
 

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 size_t NativeProcessLinux::UpdateThreads() {
   // The NativeProcessLinux monitoring threads are always up to date
   // with respect to thread state and they keep the thread list
   // populated properly. All this method needs to do is return the
   // thread count.
   return m_threads.size();
 }
 
-bool NativeProcessLinux::GetArchitecture(ArchSpec &arch) const {
-  arch = m_arch;
-  return true;
-}
-
 Status NativeProcessLinux::GetSoftwareBreakpointPCOffset(
     uint32_t &actual_opcode_size) {
   // FIXME put this behind a breakpoint protocol class that can be
   // set per architecture.  Need ARM, MIPS support here.
   static const uint8_t g_i386_opcode[] = {0xCC};
   static const uint8_t g_s390x_opcode[] = {0x00, 0x01};
   static const uint8_t g_ppc64le_opcode[] = {0x08, 0x00, 0xe0, 0x7f}; // trap
 

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp

 
 NativeRegisterContextLinux::NativeRegisterContextLinux(
     NativeThreadProtocol &native_thread, uint32_t concrete_frame_idx,
     RegisterInfoInterface *reg_info_interface_p)
     : NativeRegisterContextRegisterInfo(native_thread, concrete_frame_idx,
                                         reg_info_interface_p) {}
 
 lldb::ByteOrder NativeRegisterContextLinux::GetByteOrder() const {
-  // Get the target process whose privileged thread was used for the register
-  // read.
-  lldb::ByteOrder byte_order = lldb::eByteOrderInvalid;
-
-  if (!m_thread.GetProcess().GetByteOrder(byte_order)) {
-    // FIXME log here
-  }
-
-  return byte_order;
+  return m_thread.GetProcess().GetByteOrder();
 }
 
 Status NativeRegisterContextLinux::ReadRegisterRaw(uint32_t reg_index,
                                                    RegisterValue &reg_value) {
   const RegisterInfo *const reg_info = GetRegisterInfoAtIndex(reg_index);
   if (!reg_info)
     return Status("register %" PRIu32 " not found", reg_index);
 

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp

if (offset > sizeof(struct user_pt_regs)) {
     int regset = NT_FPREGSET;
     struct iovec ioVec;
 
     ioVec.iov_base = &regs;
     ioVec.iov_len = sizeof regs;
     error = NativeProcessLinux::PtraceWrapper(
         PTRACE_GETREGSET, m_thread.GetID(), &regset, &ioVec, sizeof regs);
     if (error.Success()) {
-      ArchSpec arch;
-      if (m_thread.GetProcess().GetArchitecture(arch))
-        value.SetBytes((void *)(((unsigned char *)(&regs)) + offset), 16,
-                       arch.GetByteOrder());
-      else
-        error.SetErrorString("failed to get architecture");
+                     m_thread.GetProcess().GetByteOrder());
     }
   } else {
     elf_gregset_t regs;
     int regset = NT_PRSTATUS;
     struct iovec ioVec;
 
     ioVec.iov_base = &regs;
     ioVec.iov_len = sizeof regs;
     error = NativeProcessLinux::PtraceWrapper(
         PTRACE_GETREGSET, m_thread.GetID(), &regset, &ioVec, sizeof regs);
     if (error.Success()) {
-      ArchSpec arch;
-      if (m_thread.GetProcess().GetArchitecture(arch))
-        value.SetBytes((void *)(((unsigned char *)(regs)) + offset), 8,
-                       arch.GetByteOrder());
-      else
-        error.SetErrorString("failed to get architecture");
+                     m_thread.GetProcess().GetByteOrder());
     }
   }
   return error;
 }
 
 Status NativeRegisterContextLinux_arm64::DoWriteRegisterValue(
     uint32_t offset, const char *reg_name, const RegisterValue &value) {
   Status error;

lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp

Status NativeRegisterContextLinux_mips64::Read_SR_Config(uint32_t offset,
                                                          uint32_t size,
                                                          RegisterValue &value) {
   GPR_linux_mips regs;
   ::memset(&regs, 0, sizeof(GPR_linux_mips));
 
   Status error = NativeProcessLinux::PtraceWrapper(
       PTRACE_GETREGS, m_thread.GetID(), NULL, &regs, sizeof regs);
   if (error.Success()) {
-    lldb_private::ArchSpec arch;
-    if (m_thread.GetProcess().GetArchitecture(arch)) {
-      void *target_address = ((uint8_t *)&regs) + offset +
-                             4 * (arch.GetMachine() == llvm::Triple::mips);
-      value.SetUInt(*(uint32_t *)target_address, size);
-    } else
-      error.SetErrorString("failed to get architecture");
+    const lldb_private::ArchSpec &arch =
+        m_thread.GetProcess().GetArchitecture();
   }
   return error;
 }
 
 Status NativeRegisterContextLinux_mips64::DoReadWatchPointRegisterValue(
     lldb::tid_t tid, void *watch_readback) {
   return NativeProcessLinux::PtraceWrapper(PTRACE_GET_WATCH_REGS,
                                            m_thread.GetID(), watch_readback);

lldb/trunk/source/Plugins/Process/Linux/NativeThreadLinux.cpp

bool NativeThreadLinux::GetStopReason(ThreadStopInfo &stop_info,
   llvm_unreachable("unhandled StateType!");
 }
 
 NativeRegisterContextSP NativeThreadLinux::GetRegisterContext() {
   // Return the register context if we already created it.
   if (m_reg_context_sp)
     return m_reg_context_sp;
 
-  ArchSpec target_arch;
-  if (!m_process.GetArchitecture(target_arch))
-    return NativeRegisterContextSP();
-
   const uint32_t concrete_frame_idx = 0;
   m_reg_context_sp.reset(
       NativeRegisterContextLinux::CreateHostNativeRegisterContextLinux(
-          target_arch, *this, concrete_frame_idx));
+          m_process.GetArchitecture(), *this, concrete_frame_idx));
 
   return m_reg_context_sp;
 }
 
 Status NativeThreadLinux::SetWatchpoint(lldb::addr_t addr, size_t size,
                                         uint32_t watch_flags, bool hardware) {
   if (!hardware)
     return Status("not implemented");

lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h

Status AllocateMemory(size_t size, uint32_t permissions,
                         lldb::addr_t &addr) override;
 
   Status DeallocateMemory(lldb::addr_t addr) override;
 
   lldb::addr_t GetSharedLibraryInfoAddress() override;
 
   size_t UpdateThreads() override;
 
-  bool GetArchitecture(ArchSpec &arch) const override;
+  const ArchSpec &GetArchitecture() const override { return m_arch; }
 
   Status SetBreakpoint(lldb::addr_t addr, uint32_t size,
                        bool hardware) override;
 
   Status GetLoadedModuleFileSpec(const char *module_path,
                                  FileSpec &file_spec) override;
 
   Status GetFileLoadAddress(const llvm::StringRef &file_name,

lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

 
 lldb::addr_t NativeProcessNetBSD::GetSharedLibraryInfoAddress() {
   // punt on this for now
   return LLDB_INVALID_ADDRESS;
 }
 
 size_t NativeProcessNetBSD::UpdateThreads() { return m_threads.size(); }
 
-bool NativeProcessNetBSD::GetArchitecture(ArchSpec &arch) const {
-  arch = m_arch;
-  return true;
-}
-
 Status NativeProcessNetBSD::SetBreakpoint(lldb::addr_t addr, uint32_t size,
                                           bool hardware) {
   if (hardware)
     return Status("NativeProcessNetBSD does not support hardware breakpoints");
   else
     return SetSoftwareBreakpoint(addr, size);
 }
 

lldb/trunk/source/Plugins/Process/NetBSD/NativeThreadNetBSD.cpp

bool NativeThreadNetBSD::GetStopReason(ThreadStopInfo &stop_info,
   llvm_unreachable("unhandled StateType!");
 }
 
 NativeRegisterContextSP NativeThreadNetBSD::GetRegisterContext() {
   // Return the register context if we already created it.
   if (m_reg_context_sp)
     return m_reg_context_sp;
 
-  ArchSpec target_arch;
-  if (!m_process.GetArchitecture(target_arch))
-    return NativeRegisterContextSP();
-
   const uint32_t concrete_frame_idx = 0;
   m_reg_context_sp.reset(
       NativeRegisterContextNetBSD::CreateHostNativeRegisterContextNetBSD(
-          target_arch, *this, concrete_frame_idx));
+          m_process.GetArchitecture(), *this, concrete_frame_idx));
 
   return m_reg_context_sp;
 }
 
 Status NativeThreadNetBSD::SetWatchpoint(lldb::addr_t addr, size_t size,
                                          uint32_t watch_flags, bool hardware) {
   if (!hardware)
     return Status("not implemented");

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

if (reg_index == std::numeric_limits<uint32_t>::max()) {
     return SendErrorResponse(0x29);
   }
 
   // Note debugserver would send an E30 here.
   if ((packet.GetBytesLeft() < 1) || (packet.GetChar() != '='))
     return SendIllFormedResponse(
         packet, "P packet missing '=' char after register number");
 
-  // Get process architecture.
-  ArchSpec process_arch;
-  if (!m_debugged_process_up ||
-      !m_debugged_process_up->GetArchitecture(process_arch)) {
-    if (log)
-      log->Printf("GDBRemoteCommunicationServerLLGS::%s failed to retrieve "
-                  "inferior architecture",
-                  __FUNCTION__);
-    return SendErrorResponse(0x49);
-  }
-
   // Parse out the value.
   uint8_t reg_bytes[32]; // big enough to support up to 256 bit ymmN register
   size_t reg_size = packet.GetHexBytesAvail(reg_bytes);
 
   // Get the thread to use.
   NativeThreadProtocol *thread = GetThreadFromSuffix(packet);
   if (!thread) {
     if (log)
GDBRemoteCommunicationServerLLGS::Handle_P(StringExtractorGDBRemote &packet) {
   if ((reg_size != reg_info->byte_size) &&
       !(reg_info->dynamic_size_dwarf_expr_bytes)) {
     return SendIllFormedResponse(packet, "P packet register size is incorrect");
   }
 
   // Build the reginfos response.
   StreamGDBRemote response;
 
-  RegisterValue reg_value(reg_bytes, reg_size, process_arch.GetByteOrder());
+  RegisterValue reg_value(
+      reg_bytes, reg_size,
+      m_debugged_process_up->GetArchitecture().GetByteOrder());
   Status error = reg_context_sp->WriteRegister(reg_info, reg_value);
   if (error.Fail()) {
     if (log)
       log->Printf("GDBRemoteCommunicationServerLLGS::%s failed, write of "
                   "requested register %" PRIu32 " (%s) failed: %s",
                   __FUNCTION__, reg_index, reg_info->name, error.AsCString());
     return SendErrorResponse(0x32);
   }