[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:158
+  memset(&m_context, 0, sizeof(m_context));
+  m_context.ContextFlags = kWinContextFlags;
   if (::SuspendThread(
----------------
amccarth wrote:
> As far as I understand what InitializeContext does, this seems a suitable replacement for how it's used here.
> 
> But if someone were to change the flag to include CONTEXT_XSTATE, then this would no longer work, because the xstate is variable length.
> 
> I would suggest:
> 
> 1.  Eliminate the kWinContextFlags (defined at the top of this file) and just use CONTEXT_ALL here.  The extra name doesn't seem to clarify anything, and the distance between the definition and usage can make it harder for folks to understand this code.
> 
> 2.  Add a comment saying this is a substitute for InitializeContext for Wine, which makes searching the code for `InitializeContext` useful.
Good point re `CONTEXT_XSTATE`; as far as I see the NativeRegisterContext classes share that limitation as well.

As for the change suggestions, 1. makes sense to me.

As for 2, while wine was the reason for looking at this originally, it's not a place where InitializeContext makes much sense to begin with, IMO. If you read the current original code, we just use it for getting a (variably sized) CONTEXT pointer in a large zero-initialized buffer, to use for memcpying over a fixed sized CONTEXT elsewhere. None of that feels like what InitializeContext does.

Or put another way; I wouldn't like to point out wine here, as I wouldn't suggest this change if I felt the use of InitializeContext was justified here. (In that case I'd probably fix wine instead.) If we'd get rid of the fixed size allocation of `m_context`, using InitializeContext for allocating a pointer would make sense though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70742/new/

https://reviews.llvm.org/D70742