Fixed "log enable linux registers" and added a test.

- Eliminated the use of static for methods that read m_register_infos, so that these routines can be implemented in the base class.
- Eliminated m_register_infos in the base class because this is not used when derived classes call UpdateRegisterInfo.
- Also moved the namespace using declarations from headers to source files.

Thanks to Daniel and Samuel for their review feedback.

llvm-svn: 181538

lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.cpp

@@ -1437,7 +1437,7 @@ ProcessMonitor::WriteMemory(lldb::addr_t vm_addr, const void *buf, size_t size,
 }
 
 bool
-ProcessMonitor::ReadRegisterValue(lldb::tid_t tid, unsigned offset,
+ProcessMonitor::ReadRegisterValue(lldb::tid_t tid, unsigned offset, const char* reg_name,
                                   unsigned size, RegisterValue &value)
 {
     bool result;
@@ -1448,7 +1448,7 @@ ProcessMonitor::ReadRegisterValue(lldb::tid_t tid, unsigned offset,
 
 bool
 ProcessMonitor::WriteRegisterValue(lldb::tid_t tid, unsigned offset,
-                                   const RegisterValue &value)
+                                   const char* reg_name, const RegisterValue &value)
 {
     bool result;
     WriteRegOperation op(offset, value, result);

lldb/source/Plugins/Process/FreeBSD/ProcessMonitor.h

@@ -107,7 +107,7 @@ public:
     /// FIXME: The FreeBSD implementation of this function should use tid in order
     ///        to enable support for debugging threaded programs.
     bool
-    ReadRegisterValue(lldb::tid_t tid, unsigned offset,
+    ReadRegisterValue(lldb::tid_t tid, unsigned offset, const char *reg_name,
                       unsigned size, lldb_private::RegisterValue &value);
 
     /// Writes the given value to the register identified by the given
@@ -117,7 +117,7 @@ public:
     /// FIXME: The FreeBSD implementation of this function should use tid in order
     ///        to enable support for debugging threaded programs.
     bool
-    WriteRegisterValue(lldb::tid_t tid, unsigned offset,
+    WriteRegisterValue(lldb::tid_t tid, unsigned offset, const char *reg_name,
                        const lldb_private::RegisterValue &value);
 
     /// Reads all general purpose registers into the specified buffer.

lldb/source/Plugins/Process/Linux/ProcessMonitor.cpp

@@ -449,9 +449,9 @@ WriteOperation::Execute(ProcessMonitor *monitor)
 class ReadRegOperation : public Operation
 {
 public:
-    ReadRegOperation(lldb::tid_t tid, unsigned offset,
+    ReadRegOperation(lldb::tid_t tid, unsigned offset, const char *reg_name,
                      RegisterValue &value, bool &result)
-        : m_tid(tid), m_offset(offset),
+        : m_tid(tid), m_offset(offset), m_reg_name(reg_name),
           m_value(value), m_result(result)
         { }
 
@@ -460,6 +460,7 @@ public:
 private:
     lldb::tid_t m_tid;
     uintptr_t m_offset;
+    const char *m_reg_name;
     RegisterValue &m_value;
     bool &m_result;
 };
@@ -481,7 +482,7 @@ ReadRegOperation::Execute(ProcessMonitor *monitor)
     }
     if (log)
         log->Printf ("ProcessMonitor::%s() reg %s: 0x%" PRIx64, __FUNCTION__,
-                     POSIXThread::GetRegisterNameFromOffset(m_offset), data);
+                     m_reg_name, data);
 }
 
 //------------------------------------------------------------------------------
@@ -490,9 +491,9 @@ ReadRegOperation::Execute(ProcessMonitor *monitor)
 class WriteRegOperation : public Operation
 {
 public:
-    WriteRegOperation(lldb::tid_t tid, unsigned offset,
+    WriteRegOperation(lldb::tid_t tid, unsigned offset, const char *reg_name,
                       const RegisterValue &value, bool &result)
-        : m_tid(tid), m_offset(offset),
+        : m_tid(tid), m_offset(offset), m_reg_name(reg_name),
           m_value(value), m_result(result)
         { }
 
@@ -501,6 +502,7 @@ public:
 private:
     lldb::tid_t m_tid;
     uintptr_t m_offset;
+    const char *m_reg_name;
     const RegisterValue &m_value;
     bool &m_result;
 };
@@ -518,8 +520,7 @@ WriteRegOperation::Execute(ProcessMonitor *monitor)
 #endif
 
     if (log)
-        log->Printf ("ProcessMonitor::%s() reg %s: %p", __FUNCTION__,
+        log->Printf ("ProcessMonitor::%s() reg %s: %p", __FUNCTION__, m_reg_name, buf);
-                     POSIXThread::GetRegisterNameFromOffset(m_offset), buf);
     if (PTRACE(PTRACE_POKEUSER, m_tid, (void*)m_offset, buf, 0))
         m_result = false;
     else
@@ -1715,21 +1716,21 @@ ProcessMonitor::WriteMemory(lldb::addr_t vm_addr, const void *buf, size_t size,
 }
 
 bool
-ProcessMonitor::ReadRegisterValue(lldb::tid_t tid, unsigned offset,
+ProcessMonitor::ReadRegisterValue(lldb::tid_t tid, unsigned offset, const char* reg_name,
                                   unsigned size, RegisterValue &value)
 {
     bool result;
-    ReadRegOperation op(tid, offset, value, result);
+    ReadRegOperation op(tid, offset, reg_name, value, result);
     DoOperation(&op);
     return result;
 }
 
 bool
 ProcessMonitor::WriteRegisterValue(lldb::tid_t tid, unsigned offset,
-                                   const RegisterValue &value)
+                                   const char* reg_name, const RegisterValue &value)
 {
     bool result;
-    WriteRegOperation op(tid, offset, value, result);
+    WriteRegOperation op(tid, offset, reg_name, value, result);
     DoOperation(&op);
     return result;
 }

lldb/source/Plugins/Process/Linux/ProcessMonitor.h

@@ -24,12 +24,10 @@ namespace lldb_private
 class Error;
 class Module;
 class Scalar;
-
 } // End lldb_private namespace.
 
 class ProcessLinux;
 class Operation;
-class ProcessPOSIX;
 
 /// @class ProcessMonitor
 /// @brief Manages communication with the inferior (debugee) process.
@@ -107,7 +105,7 @@ public:
     ///
     /// This method is provided for use by RegisterContextLinux derivatives.
     bool
-    ReadRegisterValue(lldb::tid_t tid, unsigned offset,
+    ReadRegisterValue(lldb::tid_t tid, unsigned offset, const char *reg_name,
                       unsigned size, lldb_private::RegisterValue &value);
 
     /// Writes the given value to the register identified by the given
@@ -115,7 +113,7 @@ public:
     ///
     /// This method is provided for use by RegisterContextLinux derivatives.
     bool
-    WriteRegisterValue(lldb::tid_t tid, unsigned offset,
+    WriteRegisterValue(lldb::tid_t tid, unsigned offset, const char *reg_name,
                        const lldb_private::RegisterValue &value);
 
     /// Reads all general purpose registers into the specified buffer.

lldb/source/Plugins/Process/POSIX/POSIXThread.cpp

@@ -441,11 +441,14 @@ POSIXThread::GetRegisterIndexFromOffset(unsigned offset)
     case ArchSpec::eCore_x86_32_i386:
     case ArchSpec::eCore_x86_32_i486:
     case ArchSpec::eCore_x86_32_i486sx:
-        reg = RegisterContext_i386::GetRegisterIndexFromOffset(offset);
-        break;
-
     case ArchSpec::eCore_x86_64_x86_64:
-        reg = RegisterContext_x86_64::GetRegisterIndexFromOffset(offset);
+        {
+            RegisterContextSP base = GetRegisterContext();
+            if (base) {
+                RegisterContextPOSIX &context = static_cast<RegisterContextPOSIX &>(*base);
+                reg = context.GetRegisterIndexFromOffset(offset);
+            }
+        }
         break;
     }
     return reg;
@@ -454,7 +457,7 @@ POSIXThread::GetRegisterIndexFromOffset(unsigned offset)
 const char *
 POSIXThread::GetRegisterName(unsigned reg)
 {
-    const char * name;
+    const char * name = nullptr;
     ArchSpec arch = Host::GetArchitecture();
 
     switch (arch.GetCore())
@@ -466,11 +469,8 @@ POSIXThread::GetRegisterName(unsigned reg)
     case ArchSpec::eCore_x86_32_i386:
     case ArchSpec::eCore_x86_32_i486:
     case ArchSpec::eCore_x86_32_i486sx:
-        name = RegisterContext_i386::GetRegisterName(reg);
-        break;
-
     case ArchSpec::eCore_x86_64_x86_64:
-        name = RegisterContext_x86_64::GetRegisterName(reg);
+        name = GetRegisterContext()->GetRegisterName(reg);
         break;
     }
     return name;

lldb/source/Plugins/Process/POSIX/POSIXThread.h

@@ -49,17 +49,17 @@ public:
     CreateRegisterContextForFrame (lldb_private::StackFrame *frame);
 
     //--------------------------------------------------------------------------
-    // These static functions provide a mapping from the register offset
+    // These functions provide a mapping from the register offset
     // back to the register index or name for use in debugging or log
     // output.
 
-    static unsigned
+    unsigned
     GetRegisterIndexFromOffset(unsigned offset);
 
-    static const char *
+    const char *
     GetRegisterName(unsigned reg);
 
-    static const char *
+    const char *
     GetRegisterNameFromOffset(unsigned offset);
 
     //--------------------------------------------------------------------------

lldb/source/Plugins/Process/POSIX/RegisterContextFreeBSD_x86_64.cpp

@@ -9,11 +9,13 @@
 
 #include "RegisterContextFreeBSD_x86_64.h"
 
+using namespace lldb_private;
+
 // Computes the offset of the given GPR in the user data area.
 #define GPR_OFFSET(regname)                                                 \
     (offsetof(GPR, regname))
 
-// Updates the FreeBSD specific information (offset and size)
+// Update the FreeBSD specific information (offset and size).
 #define UPDATE_GPR_INFO(reg)                                                \
 do {                                                                        \
     m_register_infos[gpr_##reg].byte_size = sizeof(GPR::reg);               \
@@ -79,14 +81,17 @@ const RegisterInfo *
 RegisterContextFreeBSD_x86_64::GetRegisterInfo()
 {
     // Allocate RegisterInfo only once
-    if (m_register_infos == nullptr)
+    if (!m_register_infos)
     {
         m_register_infos = new RegisterInfo[k_num_registers];
         // Copy the register information from base class
-        memcpy(m_register_infos, RegisterContext_x86_64::GetRegisterInfo(),
+        if (m_register_infos)
-               sizeof(RegisterInfo) * k_num_registers);
+        {
-        // Update the FreeBSD specfic register information(offset and size)
+            memcpy(m_register_infos, RegisterContext_x86_64::GetRegisterInfo(),
-        UpdateRegisterInfo();
+                   sizeof(RegisterInfo) * k_num_registers);
+            // Update the Linux specific register information (offset and size).
+            UpdateRegisterInfo();
+        }
     }
     return m_register_infos;
 }

lldb/source/Plugins/Process/POSIX/RegisterContextFreeBSD_x86_64.h

@@ -12,24 +12,25 @@
 
 #include "Plugins/Process/POSIX/RegisterContext_x86_64.h"
 
-using namespace lldb_private;
-
 class RegisterContextFreeBSD_x86_64:
     public RegisterContext_x86_64
 {
 public:
-    RegisterContextFreeBSD_x86_64(Thread &thread, uint32_t concrete_frame_idx);
+    RegisterContextFreeBSD_x86_64(lldb_private::Thread &thread, uint32_t concrete_frame_idx);
     virtual ~RegisterContextFreeBSD_x86_64();
 
-    size_t GetGPRSize();
+    size_t
+    GetGPRSize();
 
 protected:
     virtual const lldb_private::RegisterInfo *
     GetRegisterInfo();
 
+    virtual void
+    UpdateRegisterInfo();
+
 private:
     static lldb_private::RegisterInfo *m_register_infos;
-    void UpdateRegisterInfo();
 };
 
 #endif

lldb/source/Plugins/Process/POSIX/RegisterContextLinux_x86_64.cpp

@@ -9,11 +9,13 @@
 
 #include "RegisterContextLinux_x86_64.h"
 
+using namespace lldb_private;
+
 // Computes the offset of the given GPR in the user data area.
 #define GPR_OFFSET(regname)                                                 \
     (offsetof(GPR, regname))
 
-// Updates the Linux specific information (offset and size)
+// Update the Linux specific information (offset and size).
 #define UPDATE_GPR_INFO(reg)                                                \
 do {                                                                        \
     m_register_infos[gpr_##reg].byte_size = sizeof(GPR::reg);               \
@@ -114,14 +116,17 @@ const RegisterInfo *
 RegisterContextLinux_x86_64::GetRegisterInfo()
 {
     // Allocate RegisterInfo only once
-    if (m_register_infos == nullptr)
+    if (!m_register_infos)
     {
         m_register_infos = new RegisterInfo[k_num_registers];
         // Copy the register information from base class
-        memcpy(m_register_infos, RegisterContext_x86_64::GetRegisterInfo(),
+        if (m_register_infos)
-               sizeof(RegisterInfo) * k_num_registers);
+        {
-        // Update the Linux specific register information(offset and size)
+            memcpy(m_register_infos, RegisterContext_x86_64::GetRegisterInfo(),
-        UpdateRegisterInfo();
+                   sizeof(RegisterInfo) * k_num_registers);
+            // Update the Linux specific register information (offset and size).
+            UpdateRegisterInfo();
+        }
     }
     return m_register_infos;
 }

lldb/source/Plugins/Process/POSIX/RegisterContextLinux_x86_64.h

@@ -12,24 +12,25 @@
 
 #include "Plugins/Process/POSIX/RegisterContext_x86_64.h"
 
-using namespace lldb_private;
-
 class RegisterContextLinux_x86_64:
     public RegisterContext_x86_64
 {
 public:
-    RegisterContextLinux_x86_64(Thread &thread, uint32_t concrete_frame_idx);
+    RegisterContextLinux_x86_64(lldb_private::Thread &thread, uint32_t concrete_frame_idx);
     virtual ~RegisterContextLinux_x86_64();
 
-    size_t GetGPRSize();
+    size_t
+    GetGPRSize();
 
 protected:
     virtual const lldb_private::RegisterInfo *
     GetRegisterInfo();
 
+    virtual void
+    UpdateRegisterInfo();
+
 private:
     static lldb_private::RegisterInfo *m_register_infos;
-    void UpdateRegisterInfo();
 };
 
 #endif

lldb/source/Plugins/Process/POSIX/RegisterContextPOSIX.h

@@ -36,6 +36,10 @@ public:
     ///    True if the operation succeeded and false otherwise.
     virtual bool UpdateAfterBreakpoint() { return true; }
 
+    /// Determines the index in lldb's register file given a kernel byte offset.
+    virtual unsigned
+    GetRegisterIndexFromOffset(unsigned offset) { return LLDB_INVALID_REGNUM; }
+
     // Checks to see if a watchpoint specified by hw_index caused the inferior
     // to stop.
     virtual bool

lldb/source/Plugins/Process/POSIX/RegisterContext_i386.cpp

@@ -341,7 +341,8 @@ RegisterContext_i386::ReadRegister(const RegisterInfo *reg_info,
 {
     const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
     ProcessMonitor &monitor = GetMonitor();
-    return monitor.ReadRegisterValue(m_thread.GetID(), GetRegOffset(reg), GetRegSize(reg), value);
+    return monitor.ReadRegisterValue(m_thread.GetID(), GetRegOffset(reg),
+                                     GetRegisterName(reg), GetRegSize(reg), value);
 }
 
 bool
@@ -351,11 +352,12 @@ RegisterContext_i386::ReadAllRegisterValues(DataBufferSP &data_sp)
 }
 
 bool RegisterContext_i386::WriteRegister(const RegisterInfo *reg_info,
-                                              const RegisterValue &value)
+                                         const RegisterValue &value)
 {
     const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
     ProcessMonitor &monitor = GetMonitor();
-    return monitor.WriteRegisterValue(m_thread.GetID(), GetRegOffset(reg), value);
+    return monitor.WriteRegisterValue(m_thread.GetID(), GetRegOffset(reg),
+                                      GetRegisterName(reg), value);
 }
 
 bool

lldb/source/Plugins/Process/POSIX/RegisterContext_i386.h

@@ -43,10 +43,10 @@ public:
     const lldb_private::RegisterSet *
     GetRegisterSet(size_t set);
 
-    static unsigned
+    unsigned
     GetRegisterIndexFromOffset(unsigned offset);
 
-    static const char *
+    const char *
     GetRegisterName(unsigned reg);
 
     bool

lldb/source/Plugins/Process/POSIX/RegisterContext_x86_64.cpp

@@ -457,8 +457,6 @@ g_register_infos[k_num_registers] =
     DEFINE_DR(dr, 7)
 };
 
-RegisterInfo *RegisterContext_x86_64::m_register_infos = g_register_infos;
-
 static bool IsGPR(unsigned reg)
 {
     return reg <= k_last_gpr;   // GPR's come first.
@@ -549,7 +547,7 @@ RegisterContext_x86_64::GetRegisterCount()
 const RegisterInfo *
 RegisterContext_x86_64::GetRegisterInfo()
 {
-    return m_register_infos;
+    return g_register_infos;
 }
 
 const RegisterInfo *
@@ -587,7 +585,7 @@ RegisterContext_x86_64::GetRegisterIndexFromOffset(unsigned offset)
     unsigned reg;
     for (reg = 0; reg < k_num_registers; reg++)
     {
-        if (m_register_infos[reg].byte_offset == offset)
+        if (GetRegisterInfo()[reg].byte_offset == offset)
             break;
     }
     assert(reg < k_num_registers && "Invalid register offset.");
@@ -598,7 +596,7 @@ const char *
 RegisterContext_x86_64::GetRegisterName(unsigned reg)
 {
     assert(reg < k_num_registers && "Invalid register offset.");
-    return m_register_infos[reg].name;
+    return GetRegisterInfo()[reg].name;
 }
 
 lldb::ByteOrder
@@ -690,7 +688,8 @@ RegisterContext_x86_64::ReadRegister(const RegisterInfo *reg_info, RegisterValue
     }
     else {
         ProcessMonitor &monitor = GetMonitor();
-        return monitor.ReadRegisterValue(m_thread.GetID(), GetRegisterOffset(reg), GetRegisterSize(reg), value);
+        return monitor.ReadRegisterValue(m_thread.GetID(), GetRegisterOffset(reg),
+                                         GetRegisterName(reg), GetRegisterSize(reg), value);
     }
 
     if (reg_info->encoding == eEncodingVector) {
@@ -788,7 +787,7 @@ RegisterContext_x86_64::WriteRegister(const lldb_private::RegisterInfo *reg_info
     const uint32_t reg = reg_info->kinds[eRegisterKindLLDB];
     if (IsGPR(reg)) {
         ProcessMonitor &monitor = GetMonitor();
-        return monitor.WriteRegisterValue(m_thread.GetID(), GetRegisterOffset(reg), value);
+        return monitor.WriteRegisterValue(m_thread.GetID(), GetRegisterOffset(reg), GetRegisterName(reg), value);
     }
 
     if (IsFPR(reg, m_fpr_type)) {
@@ -890,6 +889,7 @@ RegisterContext_x86_64::ReadRegister(const unsigned reg,
     ProcessMonitor &monitor = GetMonitor();
     return monitor.ReadRegisterValue(m_thread.GetID(),
                                      GetRegisterOffset(reg),
+                                     GetRegisterName(reg),
                                      GetRegisterSize(reg),
                                      value);
 }
@@ -901,6 +901,7 @@ RegisterContext_x86_64::WriteRegister(const unsigned reg,
     ProcessMonitor &monitor = GetMonitor();
     return monitor.WriteRegisterValue(m_thread.GetID(),
                                       GetRegisterOffset(reg),
+                                      GetRegisterName(reg),
                                       value);
 }
 

lldb/source/Plugins/Process/POSIX/RegisterContext_x86_64.h

@@ -161,10 +161,10 @@ public:
     const lldb_private::RegisterSet *
     GetRegisterSet(size_t set);
 
-    static unsigned
+    unsigned
     GetRegisterIndexFromOffset(unsigned offset);
 
-    static const char *
+    const char *
     GetRegisterName(unsigned reg);
 
     virtual bool
@@ -317,9 +317,6 @@ protected:
     virtual bool
     WriteRegister(const unsigned reg, const lldb_private::RegisterValue &value);
 
-private:
-    static lldb_private::RegisterInfo *m_register_infos;
-
 private:
     uint64_t m_gpr[k_num_gpr_registers]; // general purpose registers.
     FPRType  m_fpr_type;                 // determines the type of data stored by union FPR, if any.
@@ -332,7 +329,7 @@ private:
 
     bool CopyXSTATEtoYMM(uint32_t reg, lldb::ByteOrder byte_order);
     bool CopyYMMtoXSTATE(uint32_t reg, lldb::ByteOrder byte_order);
-    static bool IsFPR(unsigned reg, FPRType fpr_type);
+    bool IsFPR(unsigned reg, FPRType fpr_type);
 
     bool ReadGPR();
     bool ReadFPR();

lldb/test/functionalities/register/TestRegisters.py

@@ -2,7 +2,7 @@
 Test the 'register' command.
 """
 
-import os, time
+import os, sys, time
 import re
 import unittest2
 import lldb
@@ -13,6 +13,10 @@ class RegisterCommandsTestCase(TestBase):
 
     mydir = os.path.join("functionalities", "register")
 
+    def setUp(self):
+        TestBase.setUp(self)
+        self.has_teardown = False
+
     def test_register_commands(self):
         """Test commands related to registers, in particular vector registers."""
         if not self.getArchitecture() in ['i386', 'x86_64']:
@@ -51,6 +55,8 @@ class RegisterCommandsTestCase(TestBase):
 
     def common_setup(self):
         exe = os.path.join(os.getcwd(), "a.out")
+        self.log_file = exe + ".log"
+
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Break in main().
@@ -62,10 +68,27 @@ class RegisterCommandsTestCase(TestBase):
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
             substrs = ['stopped', 'stop reason = breakpoint'])
 
+    # platform specific logging of the specified category
+    def log_enable(self, category):
+        self.platform = ""
+        if sys.platform.startswith("darwin"):
+            self.platform = "" # TODO: add support for "log enable darwin registers"
+        if sys.platform.startswith("linux"):
+            self.platform = "linux"
+
+        if self.platform != "":
+            self.runCmd("log enable " + self.platform + " " + str(category) + " registers -v -f " + self.log_file, RUN_SUCCEEDED)
+            if not self.has_teardown:
+                self.has_teardown = True
+                self.addTearDownHook(lambda: os.remove(self.log_file))
+
     def register_commands(self):
         """Test commands related to registers, in particular vector registers."""
         self.common_setup()
 
+        # verify that logging does not assert
+        self.log_enable("registers")
+
         self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
             substrs = ['registers were unavailable'], matching = False)
         self.runCmd("register read xmm0")