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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
