Don't use CopyContext in RegisterContextWin

[amccarth]

Summary

Bug 22410

Eliminate CopyContext and related functions to allow for debugging on Vista.

CopyContext is necessary to safely get the XState, but LLDB doesn't currently
use the XState. CopyContext is available as of Windows 7 SP1, so it can't be
used on Vista.

I left the structure of the code to continue to ensure that we're handling
the CONTEXT alignment and so that we can switch back to CopyContext in the
future.

I went to lldb/Host/windows.h to lower NTDDI_VERSION, which specifies the minimum
version of the SDK we use, and found it was already set to NTDDI_VISTA. This
was surprising since the CopyContext function was defined even though it's
inside a #if (NTDDI_VERSION >= NTDDI_WIN7SP1) block in <WinBase.h>. It turns out
that the SDK doesn't define NTDDI_WIN7SP1, so the statement boiled down to

While I was in lldb/Host/windows.h, I eliminated two redundant #undefs.

[zturner]

include/lldb/Host/windows/windows.h

22–23 ↗

(On Diff #19788)

What's this part for?

source/Plugins/Process/Windows/x86/RegisterContextWindows_x86.cpp

319

Is alignment to 16 necessary? I think that's only if you use CopyContext() and InitializeContext(). If you're not using any of the AVX stuff, I don't think it has special alignment requirements. We might be able to get rid of this whole function, and all the cruft associated with the heap buffer, and just store the CONTEXT directly in the class. Note that CONTEXT in WinNT.h is declared with DECLSPEC_ALIGN(8), so I think everything is already taken care of.

321–322

checkout llvm/Support/MathExtras.h. There are functions to do this kind of math for you. (This only applies though if it's not already aligned correctly. If it is all this code goes away)

[amccarth]

Addressed review comments.

[amccarth]

include/lldb/Host/windows/windows.h

22–23 ↗

(On Diff #19788)

Removed duplicate lines (see lines 19-20). I was in here to roll NTDDI_VERSION back to Vista, but it was already Vista. The notes explain how this "worked".

source/Plugins/Process/Windows/x86/RegisterContextWindows_x86.cpp

319

One definition of CONTEXT in winnt.h is tagged with __declspec(align(16)), so I assumed it was. Another is tagged 8, and one is untagged. The heap allocation seemed to be returning blocks aligned only to 4-byte boundaries (not even 8!?).

Do you know how to tell definitively which CONTEXT applies?

321–322

Cool. I've switched to llvm::alignAddr.

But I'll remove this altogether if the alignment isn't needed.

[amccarth]

More aggressive simplification of CONTEXT handling on Windows.

[zturner]

Looks very nice. I will commit later this afternoon. No issues popped up while running the test suite I assume? Same number of failures before and after the patch?

I would try setting a breakpoint by line number, then doing a "source list" and a "frame info" (I think that's the command anyway to see the stack frame), and a "disasm" and make sure all the values look sane. Of course normally the test suite would catch all this, but since it still fails for many tests I usually like to do this sanity check first when messign with the register stuff (in addition to running the tests obviously)

[amccarth]

Don't commit yet.

ninja check-lldb was giving me good results last night, but now it never
finishes. Looks like I get about 15 python_d.exe processes and 3 a.out
processes that hang. All bug one Python process are consuming no CPU, so
it looks like those are deadlocked. I'm investigating.

[zturner]

Hmm. Could you try memsetting the CONTEXT to 0 before calling GetThreadContext()? We already do it in WriteAllRegisterValues before calling *SetThreadContext*, but we don't do it before calling GetThreadContext(). In the previous code, we were allocating a new DataBufferHeap, which I think will 0-initialize it.

[amccarth]

Restored initialization of the ContextFlags in the context.

[zturner]

Did that fix the deadlocks?

[amccarth]

Yes. I'm getting the same number of successful tests as before. The
interactive tests worked as well, except for source list, which shows
nothing.

[zturner]

Did the source list work before?

source/Plugins/Process/Windows/x86/RegisterContextWindows_x86.h

public:
 
     uint32_t SetHardwareWatchpoint(lldb::addr_t addr, size_t size, bool read, bool write) override;
 
     bool ClearHardwareWatchpoint(uint32_t hw_index) override;
 
     bool HardwareSingleStep(bool enable) override;
 
   private:
-    static bool InitializeContextDataBuffer(lldb::DataBufferSP &buffer, CONTEXT **context_ptr);
-
     bool CacheAllRegisterValues();
 
-    // The system CONTEXT structure.  m_context_ptr is backed by m_cached_context, but
-    // m_context_ptr may not point to the beginning of the buffer allocated in m_cached_context,
-    // due to alignment requirements of CONTEXT structures.
-    lldb::DataBufferSP m_cached_context;
-    CONTEXT *m_context_ptr;
-
+    CONTEXT m_context;
     bool m_context_stale;
 };
 }
 
 #endif // #ifndef liblldb_RegisterContextPOSIX_x86_H_

source/Plugins/Process/Windows/x86/RegisterContextWindows_x86.cpp

 #include "lldb/Host/windows/windows.h"
 
 #include "lldb-x86-register-enums.h"
 #include "RegisterContext_x86.h"
 #include "RegisterContextWindows_x86.h"
 #include "TargetThreadWindows.h"
 
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/Support/MathExtras.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 #define DEFINE_GPR(reg, alt) #reg, alt, 4, 0, eEncodingUint, eFormatHexUppercase
 #define DEFINE_GPR_BIN(reg, alt) #reg, alt, 4, 0, eEncodingUint, eFormatBinary
 
 namespace

 }
 
 //------------------------------------------------------------------
 // Constructors and Destructors
 //------------------------------------------------------------------
 RegisterContextWindows_x86::RegisterContextWindows_x86(Thread &thread, uint32_t concrete_frame_idx)
     : RegisterContext(thread, concrete_frame_idx)
     , m_context_stale(true)
-    , m_context_ptr(nullptr)
+    , m_context()
 {
 }
 
 RegisterContextWindows_x86::~RegisterContextWindows_x86()
 {
 }
 
 void

 RegisterContextWindows_x86::ReadRegister(const RegisterInfo *reg_info, RegisterValue &reg_value)
 {
     if (!CacheAllRegisterValues())
         return false;
 
     switch (reg_info->kinds[eRegisterKindLLDB])
     {
         case lldb_eax_i386:
-            reg_value.SetUInt32(m_context_ptr->Eax);
+            reg_value.SetUInt32(m_context.Eax);
             break;
         case lldb_ebx_i386:
-            reg_value.SetUInt32(m_context_ptr->Ebx);
+            reg_value.SetUInt32(m_context.Ebx);
             break;
         case lldb_ecx_i386:
-            reg_value.SetUInt32(m_context_ptr->Ecx);
+            reg_value.SetUInt32(m_context.Ecx);
             break;
         case lldb_edx_i386:
-            reg_value.SetUInt32(m_context_ptr->Edx);
+            reg_value.SetUInt32(m_context.Edx);
             break;
         case lldb_edi_i386:
-            reg_value.SetUInt32(m_context_ptr->Edi);
+            reg_value.SetUInt32(m_context.Edi);
             break;
         case lldb_esi_i386:
-            reg_value.SetUInt32(m_context_ptr->Esi);
+            reg_value.SetUInt32(m_context.Esi);
             break;
         case lldb_ebp_i386:
-            reg_value.SetUInt32(m_context_ptr->Ebp);
+            reg_value.SetUInt32(m_context.Ebp);
             break;
         case lldb_esp_i386:
-            reg_value.SetUInt32(m_context_ptr->Esp);
+            reg_value.SetUInt32(m_context.Esp);
             break;
         case lldb_eip_i386:
-            reg_value.SetUInt32(m_context_ptr->Eip);
+            reg_value.SetUInt32(m_context.Eip);
             break;
         case lldb_eflags_i386:
-            reg_value.SetUInt32(m_context_ptr->EFlags);
+            reg_value.SetUInt32(m_context.EFlags);
             break;
     }
     return true;
 }
 
 bool
 RegisterContextWindows_x86::WriteRegister(const RegisterInfo *reg_info, const RegisterValue &reg_value)
 {
     // Since we cannot only write a single register value to the inferior, we need to make sure
     // our cached copy of the register values are fresh.  Otherwise when writing EAX, for example,
     // we may also overwrite some other register with a stale value.
     if (!CacheAllRegisterValues())
         return false;
 
     switch (reg_info->kinds[eRegisterKindLLDB])
     {
         case lldb_eax_i386:
-            m_context_ptr->Eax = reg_value.GetAsUInt32();
+            m_context.Eax = reg_value.GetAsUInt32();
             break;
         case lldb_ebx_i386:
-            m_context_ptr->Ebx = reg_value.GetAsUInt32();
+            m_context.Ebx = reg_value.GetAsUInt32();
             break;
         case lldb_ecx_i386:
-            m_context_ptr->Ecx = reg_value.GetAsUInt32();
+            m_context.Ecx = reg_value.GetAsUInt32();
             break;
         case lldb_edx_i386:
-            m_context_ptr->Edx = reg_value.GetAsUInt32();
+            m_context.Edx = reg_value.GetAsUInt32();
             break;
         case lldb_edi_i386:
-            m_context_ptr->Edi = reg_value.GetAsUInt32();
+            m_context.Edi = reg_value.GetAsUInt32();
             break;
         case lldb_esi_i386:
-            m_context_ptr->Esi = reg_value.GetAsUInt32();
+            m_context.Esi = reg_value.GetAsUInt32();
             break;
         case lldb_ebp_i386:
-            m_context_ptr->Ebp = reg_value.GetAsUInt32();
+            m_context.Ebp = reg_value.GetAsUInt32();
             break;
         case lldb_esp_i386:
-            m_context_ptr->Esp = reg_value.GetAsUInt32();
+            m_context.Esp = reg_value.GetAsUInt32();
             break;
         case lldb_eip_i386:
-            m_context_ptr->Eip = reg_value.GetAsUInt32();
+            m_context.Eip = reg_value.GetAsUInt32();
             break;
         case lldb_eflags_i386:
-            m_context_ptr->EFlags = reg_value.GetAsUInt32();
+            m_context.EFlags = reg_value.GetAsUInt32();
             break;
     }
 
     // Physically update the registers in the target process.
     TargetThreadWindows &wthread = static_cast<TargetThreadWindows &>(m_thread);
-    return ::SetThreadContext(wthread.GetHostThread().GetNativeThread().GetSystemHandle(), m_context_ptr);
+    return ::SetThreadContext(wthread.GetHostThread().GetNativeThread().GetSystemHandle(), &m_context);
 }
 
 bool
 RegisterContextWindows_x86::ReadAllRegisterValues(lldb::DataBufferSP &data_sp)
 {
     if (!CacheAllRegisterValues())
         return false;
-
-    CONTEXT *dest_context = nullptr;
-    if (!InitializeContextDataBuffer(data_sp, &dest_context))
-        return false;
-
-    // In the future, we should use CopyContext to safely get XState.  Since
-    // we're not using XState at this time, we're doing a straight memcpy to
-    // avoid relying on AVX APIs that aren't available prior to Windows 7 SP1.
-    memcpy(data_sp->GetBytes(), m_context_ptr, sizeof(*m_context_ptr));
+    if (data_sp->GetByteSize() < sizeof(m_context))
+    {
+        data_sp.reset(new DataBufferHeap(sizeof(CONTEXT), 0));
+    }
+    memcpy(data_sp->GetBytes(), &m_context, sizeof(m_context));
     return true;
 }
 
 bool
 RegisterContextWindows_x86::WriteAllRegisterValues(const lldb::DataBufferSP &data_sp)
 {
-    // data_sp could only ever have been generated by a call to ReadAllRegisterValues(), so
-    // m_cached_context should already have the correct size and alignment properties.
-    assert(m_cached_context->GetByteSize() == data_sp->GetByteSize());
-
-    // As a result, we can simply memcpy the entire buffer and assume that the alignment remains
-    // the same.
-    memcpy(m_cached_context->GetBytes(), data_sp->GetBytes(), data_sp->GetByteSize());
+    assert(data_sp->GetByteSize() >= sizeof(m_context));
+    memcpy(&m_context, data_sp->GetBytes(), sizeof(m_context));
 
-    // m_context_ptr still points to the beginning of the CONTEXT structure, so use that for
-    // updating the thread state.
     TargetThreadWindows &wthread = static_cast<TargetThreadWindows &>(m_thread);
-    if (!::SetThreadContext(wthread.GetHostThread().GetNativeThread().GetSystemHandle(), m_context_ptr))
+    if (!::SetThreadContext(wthread.GetHostThread().GetNativeThread().GetSystemHandle(), &m_context))
         return false;
 
     return true;
 }
 
 uint32_t
 RegisterContextWindows_x86::ConvertRegisterKindToRegisterNumber(lldb::RegisterKind kind, uint32_t num)
 {

 
 bool
 RegisterContextWindows_x86::HardwareSingleStep(bool enable)
 {
     return false;
 }
 
 bool
-RegisterContextWindows_x86::InitializeContextDataBuffer(DataBufferSP &buffer, CONTEXT **context_ptr)
-{
-    // In the future, we should use InitializeContext to ensure proper size and
-    // alignment.  Since we're not using XState at this time, we're doing a
-    // straight allocation and manually aligning to a 16-byte boundary in order
-    // to avoid relying on AVX APIs that aren't available prior to Windows 7 SP1.
-    const std::size_t kAlignment = 16;
-    buffer.reset(new DataBufferHeap(sizeof(CONTEXT) + kAlignment, 0));
-    *context_ptr = reinterpret_cast<CONTEXT *>(llvm::alignAddr(buffer->GetBytes(), kAlignment));
-
-    (*context_ptr)->ContextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER;
-
-    return true;
-}
-
-bool
 RegisterContextWindows_x86::CacheAllRegisterValues()
 {
     if (!m_context_stale)
         return true;
 
-    if (!m_cached_context && !InitializeContextDataBuffer(m_cached_context, &m_context_ptr))
-        return false;
-
     TargetThreadWindows &wthread = static_cast<TargetThreadWindows &>(m_thread);
-    if (!::GetThreadContext(wthread.GetHostThread().GetNativeThread().GetSystemHandle(), m_context_ptr))
+    memset(&m_context, 0, sizeof(m_context));
+    m_context.ContextFlags = kWinContextFlags;
+    if (!::GetThreadContext(wthread.GetHostThread().GetNativeThread().GetSystemHandle(), &m_context))
         return false;
     m_context_stale = false;
     return true;
 }