[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG344bdeb797b3: [LLDB] Avoid using InitializeContext for
zero-initializing a CONTEXT. NFC. (authored by mstorsjo).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70742/new/
https://reviews.llvm.org/D70742
Files:
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
+++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
@@ -154,15 +154,8 @@
return true;
TargetThreadWindows = static_cast(m_thread);
- uint8_t buffer[2048];
- memset(buffer, 0, sizeof(buffer));
- PCONTEXT tmpContext = NULL;
- DWORD contextLength = (DWORD)sizeof(buffer);
- if (!::InitializeContext(buffer, kWinContextFlags, ,
- )) {
-return false;
- }
- memcpy(_context, tmpContext, sizeof(m_context));
+ memset(_context, 0, sizeof(m_context));
+ m_context.ContextFlags = kWinContextFlags;
if (::SuspendThread(
wthread.GetHostThread().GetNativeThread().GetSystemHandle()) ==
(DWORD)-1) {
Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
+++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
@@ -154,15 +154,8 @@
return true;
TargetThreadWindows = static_cast(m_thread);
- uint8_t buffer[2048];
- memset(buffer, 0, sizeof(buffer));
- PCONTEXT tmpContext = NULL;
- DWORD contextLength = (DWORD)sizeof(buffer);
- if (!::InitializeContext(buffer, kWinContextFlags, ,
- )) {
-return false;
- }
- memcpy(_context, tmpContext, sizeof(m_context));
+ memset(_context, 0, sizeof(m_context));
+ m_context.ContextFlags = kWinContextFlags;
if (::SuspendThread(
wthread.GetHostThread().GetNativeThread().GetSystemHandle()) ==
(DWORD)-1) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.
amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. That's fair. LGTM. 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
[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.
mstorsjo marked an inline comment as done. mstorsjo added inline comments. Comment at: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:158 + memset(_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
[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.
amccarth added a comment. I'm good with the change, but have a couple small requests. I hope to hear from others, too, as this area is outside my wheelhouse. Comment at: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:158 + memset(_context, 0, sizeof(m_context)); + m_context.ContextFlags = kWinContextFlags; if (::SuspendThread( 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. 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
[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.
mstorsjo created this revision.
mstorsjo added reviewers: labath, amccarth, asmith, aleksandr.urakov.
Herald added a project: LLDB.
InitializeContext is useful for allocating a CONTEXT struct in an unaligned
byte buffer. In this case, we already have a CONTEXT we want to initialize, and
we only used this as a very roundabout way of zero initializing it.
Instead just memset the CONTEXT we have, and set the ContextFlags field
manually.
This matches how it is done in NativeRegisterContextWindows_*.cpp.
This also makes LLDB run successfully in Wine (for a trivial tested case at
least), as Wine haven't implemented the InitializeContext function.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D70742
Files:
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
+++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
@@ -154,15 +154,8 @@
return true;
TargetThreadWindows = static_cast(m_thread);
- uint8_t buffer[2048];
- memset(buffer, 0, sizeof(buffer));
- PCONTEXT tmpContext = NULL;
- DWORD contextLength = (DWORD)sizeof(buffer);
- if (!::InitializeContext(buffer, kWinContextFlags, ,
- )) {
-return false;
- }
- memcpy(_context, tmpContext, sizeof(m_context));
+ memset(_context, 0, sizeof(m_context));
+ m_context.ContextFlags = kWinContextFlags;
if (::SuspendThread(
wthread.GetHostThread().GetNativeThread().GetSystemHandle()) ==
(DWORD)-1) {
Index: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
+++ lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp
@@ -154,15 +154,8 @@
return true;
TargetThreadWindows = static_cast(m_thread);
- uint8_t buffer[2048];
- memset(buffer, 0, sizeof(buffer));
- PCONTEXT tmpContext = NULL;
- DWORD contextLength = (DWORD)sizeof(buffer);
- if (!::InitializeContext(buffer, kWinContextFlags, ,
- )) {
-return false;
- }
- memcpy(_context, tmpContext, sizeof(m_context));
+ memset(_context, 0, sizeof(m_context));
+ m_context.ContextFlags = kWinContextFlags;
if (::SuspendThread(
wthread.GetHostThread().GetNativeThread().GetSystemHandle()) ==
(DWORD)-1) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
