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

What's this part for?

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

335

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.

337–338

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

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

335

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?

337–338

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?

include/lldb/Host/windows/windows.h

 #define _WIN32_WINNT _WIN32_WINNT_VISTA
 #define WIN32_LEAN_AND_MEAN
 #define NOGDI
 #define NOMINMAX
 #include <windows.h>
 #undef GetUserName
 #undef LoadImage
 #undef CreateProcess
-#undef LoadImage
-#undef GetUserName
 #undef far
 #undef near
 #undef FAR
 #undef NEAR
 #define FAR
 #define NEAR
 
 #endif  // LLDB_lldb_windows_h_

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:
-    bool InitializeContextDataBuffer(lldb::DataBufferSP &buffer, CONTEXT **context_ptr);
+    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;
 
     bool m_context_stale;
 };
 }
 
 #endif // #ifndef liblldb_RegisterContextPOSIX_x86_H_

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

 //===-- RegisterContextWindows_x86.h ----------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include <cstdint>
+
 #include "lldb/lldb-private-types.h"
 #include "lldb/Core/DataBufferHeap.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/RegisterValue.h"
 #include "lldb/Host/windows/HostThreadWindows.h"
 #include "lldb/Host/windows/windows.h"
 
 #include "lldb-x86-register-enums.h"

 {
     if (!CacheAllRegisterValues())
         return false;
 
     CONTEXT *dest_context = nullptr;
     if (!InitializeContextDataBuffer(data_sp, &dest_context))
         return false;
 
-    // Write the OS's internal CONTEXT structure into the buffer.
-    if (!CopyContext(dest_context, kWinContextFlags, m_context_ptr))
-        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));
     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.

 RegisterContextWindows_x86::HardwareSingleStep(bool enable)
 {
     return false;
 }
 
 bool
 RegisterContextWindows_x86::InitializeContextDataBuffer(DataBufferSP &buffer, CONTEXT **context_ptr)
 {
-    DWORD length = 0;
-    if (!::InitializeContext(nullptr, kWinContextFlags, nullptr, &length) && GetLastError() != ERROR_INSUFFICIENT_BUFFER)
-        return false;
+    // 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));
+    std::intptr_t address = reinterpret_cast<std::intptr_t>(buffer->GetBytes());
+    address += kAlignment - (address % kAlignment);
+    *context_ptr = reinterpret_cast<CONTEXT *>(address);
+
+    (*context_ptr)->ContextFlags = CONTEXT_CONTROL | CONTEXT_INTEGER;
 
-    buffer.reset(new DataBufferHeap(length, 0));
-    if (!::InitializeContext(buffer->GetBytes(), kWinContextFlags, context_ptr, &length))
-    {
-        buffer.reset();
-        return false;
-    }
     return true;
 }
 
 bool
 RegisterContextWindows_x86::CacheAllRegisterValues()
 {
     if (!m_context_stale)
         return true;