[Lldb-commits] [PATCH] D17152: Improve ReadRegister for RegisterContextWindowsx86

[Adrian McCarthy via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.

In some circumstances (notably, certain minidumps), the thread CONTEXT does not have values for the control registers (EIP, ESP, EBP, EFLAGS).  There are flags in the CONTEXT which indicate which portions are valid, but those flags weren't checked.  The old code would not detect this and give a garbage value for the register.  The new code will log the problem and return an error.

I consolidated the error checking and logging into a helper function, which makes the big switch statement easier to read and verify.

Ran tests to ensure this doesn't break anything.  Manually verified that a minidump without info on the control registers now indicates the problem instead of giving bad information.

http://reviews.llvm.org/D17152

Files:
  source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp
  source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.h

Index: source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.h
===================================================================
--- source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.h
+++ source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.h
@@ -41,6 +41,9 @@
 
     bool ReadRegister(const RegisterInfo *reg_info, RegisterValue &reg_value) override;
 
+private:
+    bool
+    ReadRegisterHelper(DWORD flags_required, const char *reg_name, DWORD value, RegisterValue &reg_value) const;
 };
 
 }
Index: source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp
===================================================================
--- source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp
+++ source/Plugins/Process/Windows/Common/x86/RegisterContextWindows_x86.cpp
@@ -131,48 +131,42 @@
     switch (reg)
     {
         case lldb_eax_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EAX", m_context.Eax);
-            reg_value.SetUInt32(m_context.Eax);
-            break;
+            return ReadRegisterHelper(CONTEXT_INTEGER, "EAX", m_context.Eax, reg_value);
         case lldb_ebx_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EBX", m_context.Ebx);
-            reg_value.SetUInt32(m_context.Ebx);
-            break;
+            return ReadRegisterHelper(CONTEXT_INTEGER, "EBX", m_context.Ebx, reg_value);
         case lldb_ecx_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from ECX", m_context.Ecx);
-            reg_value.SetUInt32(m_context.Ecx);
-            break;
+            return ReadRegisterHelper(CONTEXT_INTEGER, "ECX", m_context.Ecx, reg_value);
         case lldb_edx_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EDX", m_context.Edx);
-            reg_value.SetUInt32(m_context.Edx);
-            break;
+            return ReadRegisterHelper(CONTEXT_INTEGER, "EDX", m_context.Edx, reg_value);
         case lldb_edi_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EDI", m_context.Edi);
-            reg_value.SetUInt32(m_context.Edi);
-            break;
+            return ReadRegisterHelper(CONTEXT_INTEGER, "EDI", m_context.Edi, reg_value);
         case lldb_esi_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from ESI", m_context.Esi);
-            reg_value.SetUInt32(m_context.Esi);
-            break;
+            return ReadRegisterHelper(CONTEXT_INTEGER, "ESI", m_context.Esi, reg_value);
         case lldb_ebp_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EBP", m_context.Ebp);
-            reg_value.SetUInt32(m_context.Ebp);
-            break;
+            return ReadRegisterHelper(CONTEXT_CONTROL, "EBP", m_context.Ebp, reg_value);
         case lldb_esp_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from ESP", m_context.Esp);
-            reg_value.SetUInt32(m_context.Esp);
-            break;
+            return ReadRegisterHelper(CONTEXT_CONTROL, "ESP", m_context.Esp, reg_value);
         case lldb_eip_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EIP", m_context.Eip);
-            reg_value.SetUInt32(m_context.Eip);
-            break;
+            return ReadRegisterHelper(CONTEXT_CONTROL, "EIP", m_context.Eip, reg_value);
         case lldb_eflags_i386:
-            WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from EFLAGS", m_context.EFlags);
-            reg_value.SetUInt32(m_context.EFlags);
-            break;
+            return ReadRegisterHelper(CONTEXT_CONTROL, "EFLAGS", m_context.EFlags, reg_value);
         default:
             WINWARN_IFALL(WINDOWS_LOG_REGISTERS, "Requested unknown register %u", reg);
             break;
     }
+    return false;
+}
+
+bool
+RegisterContextWindows_x86::ReadRegisterHelper(DWORD flags_required, const char *reg_name, DWORD value,
+                                               RegisterValue &reg_value) const
+{
+    if ((m_context.ContextFlags & flags_required) != flags_required)
+    {
+        WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Thread context doesn't have %s", reg_name);
+        return false;
+    }
+    WINLOG_IFALL(WINDOWS_LOG_REGISTERS, "Read value 0x%x from %s", value, reg_name);
+    reg_value.SetUInt32(value);
     return true;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D17152.47674.patch
Type: text/x-patch
Size: 4416 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160211/71e56ecb/attachment-0001.bin>