[Lldb-commits] [PATCH] D56233: [lldb-server] Add initial support for lldb-server on Windows
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Apr 2 01:02:01 PDT 2019
labath added a comment.
The patch is pretty big, so I haven't done an in-depth review yet. The fact that you've ran clang-format over entire files (and not just the diffs) does not help.
For a start, I've highlighted the parts that I believe can/should go into a separate patch. Some of them are obvious and can be committed immediately. Others should go through a separate review. We can get back to the core of the patch once these have been dealt with (FWIW, I haven't seen anything obviously wrong from a quick glance).
================
Comment at: include/lldb/Core/Debugger.h:338
}
+ bool StartEventHandlerThread();
----------------
This looks like experimental code accidentally left in (?)
================
Comment at: include/lldb/Target/Platform.h:599
error.SetErrorStringWithFormat(
- "Platform::ReadFile() is not supported in the %s platform",
+ "Platform::WriteFile() is not supported in the %s platform",
GetName().GetCString());
----------------
To keep the scope of this patch (which is already pretty big) as small as possible, could you submit this as a separate patch? No review needed.
================
Comment at: include/lldb/lldb-types.h:42-45
+typedef void *process_t; // Process type is HANDLE
+typedef void *thread_t; // Host thread type
+typedef void *file_t; // Host file type
+typedef unsigned int __w64 socket_t; // Host socket type
----------------
Please revert or submit as a separate patch.
================
Comment at: lldb.xcodeproj/project.pbxproj:10522
"-lxml2",
+ "-lLLVMSupport",
"-framework",
----------------
I am surpised that you needed to change the xcode project in any way, as this code should not even build there. Please explain.
================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteRegisterState.py:28
+
+ # Skip O* packet test for Windows. Will add it back when pty is supported.
+ triple = self.dbg.GetSelectedPlatform().GetTriple()
----------------
The fact that you were able to do this told me that this part of the test is actually irrelevant. So, I just went ahead and deleted this part altogether (r357451).
================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py:208-211
+ # In current implementation of llgs on Windows, as a response to '\x03' packet, the debugger
+ # of the native process will trigger a call to DebugBreakProcess that will create a new thread
+ # to handle the exception debug event. So one more stop thread will be notified to the
+ # delegate, e.g. llgs. So tests below to assert the stop threads number will all fail.
----------------
That sounds fun. Is that how things are supposed to work, or is that something you're planning to change/fix? (mostly just curious).
================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py:674-675
+ #@expectedFailureAll(oslist=["windows"])
+ #skipIfWindows
@llgs_test
----------------
delete ?
================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py:1494
threads = self.wait_for_thread_count(3, timeout_seconds=5)
- self.assertEqual(len(threads), 3)
+ self.assertEqual(len(threads) - 1, 3)
----------------
The -1 looks like it needs to be windows-only.
================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py:434
attempts = 0
- MAX_ATTEMPTS = 20
+ MAX_ATTEMPTS = 1
----------------
I don't think this can go in like this. However, I agree that this double-retry loop is pretty ugly. When I get some time, I'll try to see if I can rewrite this part to use reverse connects. That way we won't get any races at all. For the time being, I'd just revert this.
================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/main.cpp:1
//===-- main.cpp ------------------------------------------------*- C++ -*-===//
//
----------------
Could you (as a separate patch) convert this file to use c++ threads? That should allow us to cut down on the ifdefs by using standard std::thread and std::mutex.
================
Comment at: source/Host/common/Socket.cpp:89-104
+ socket_up = llvm::make_unique<TCPSocket>(true, child_processes_inherit);
break;
case ProtocolUdp:
- socket_up =
- llvm::make_unique<UDPSocket>(true, child_processes_inherit);
+ socket_up = llvm::make_unique<UDPSocket>(true, child_processes_inherit);
break;
case ProtocolUnixDomain:
#ifndef LLDB_DISABLE_POSIX
----------------
Please avoid spurious reformats of entire files. This makes it patches hard to review, particularly when they are as large as this one.
For the changes in this specific file, I think you can just commit them as a separate patch. Or revert the file completely, as the only thing you seem to be doing is adding logging.
================
Comment at: source/Host/common/SocketAddress.cpp:268
+#endif
+ printf("SocketAddress::GetAddressInfo - %s\n", error.AsCString());
}
----------------
labath wrote:
> Writing to stdout like this is very rude. If there isn't a suitable way to return this info, then I suggest writing this to the log instead.
This comment still stands.
================
Comment at: source/Host/windows/HostInfoWindows.cpp:98
+ s.clear();
return llvm::convertWideToUTF8(buffer, s);
----------------
Move this into a separate patch, add a unit test checking that string is cleared when this function is called.
================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h:294
+
+ lldb_private::UUID m_uuid;
};
----------------
Move UUID handling into a separate patch.
================
Comment at: source/Plugins/Process/Utility/RegisterContextWindows_x64.cpp:103
+}
+#include <iostream>
+static const RegisterInfo *GetRegisterInfoPtr(const ArchSpec &target_arch) {
----------------
`<iostream>` is banned in llvm. (and it looks like you're not using it anyway).
================
Comment at: source/Plugins/Process/Utility/RegisterContextWindows_x64.h:33
+ uint32_t m_user_register_count;
+ std::vector<lldb_private::RegisterInfo> d_register_infos;
+};
----------------
`d_` ?
================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:64
+namespace lldb_private {
+#if 1
+class ProcessWindowsData {
----------------
delete?
================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:98
+ // Python vcont_s test allows single step into a running process.
+ if (1) {//state == eStateStopped || state == eStateCrashed) {
+ LLDB_LOG(log, "process {0} is in state {1}. Resuming...",
----------------
delete?
================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:16
+
+#include "IDebugDelegate.h"
+#include "ProcessDebugger.h"
----------------
It looks like IDebugDelegate.h is missing from the patch.
================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:138
+ void OnExitProcess(uint32_t exit_code) {
+ if (m_process)
+ m_process->OnExitProcess(exit_code);
----------------
It looks like `m_process` will never be null. Convert it to a reference, and delete all the null-checks?
================
Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.h:16
+
+#include "ForwardDecl.h"
+
----------------
File missing from patch.
================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:565-585
+ free(m_decompression_scratch);
m_decompression_scratch = nullptr;
}
size_t scratchbuf_size = 0;
if (m_compression_type == CompressionType::LZFSE)
- scratchbuf_size = compression_decode_scratch_buffer_size (COMPRESSION_LZFSE);
+ scratchbuf_size =
+ compression_decode_scratch_buffer_size(COMPRESSION_LZFSE);
----------------
spurious reformats.
================
Comment at: tools/lldb-server/SystemInitializerLLGS.cpp:37-52
+#if defined(_WIN32)
+ if (g_ref_count++ == 0) {
+ // Require Windows Sockets version 2.2.
+ auto wVersion = MAKEWORD(2, 2);
+ WSADATA wsaData;
+ auto err = WSAStartup(wVersion, &wsaData);
+ if (err == 0) {
----------------
zturner wrote:
> labath wrote:
> > I think a better option here would be to move this code to something like `Socket::Initialize`. Then we could call this from `SystemInitializerCommon::Initialize`, and it would be available to everyone. This would allow us to remove the copy of this code in PlatformWindows, and replace the `WSAStartup` calls in various unittests with `Socket::Initialize()`
> Also, why do we need the `g_ref_count`? Can't we just print an error message and call exit if the version is not what we expect?
I still believe this should go to Socket::Initialize (as a separate patch).
================
Comment at: tools/lldb-server/lldb-server.cpp:38-74
+namespace llgs {
static void initialize() {
if (auto e = g_debugger_lifetime->Initialize(
llvm::make_unique<SystemInitializerLLGS>(), nullptr))
llvm::consumeError(std::move(e));
}
----------------
The whole file looks good. You can commit this separately straight away. (Maybe add a comment somewhere that the namespace is there to avoid conflicts on windows. Otherwise someone in the future might get the idea to replace the file-local namespace with an anonymous one.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56233/new/
https://reviews.llvm.org/D56233
More information about the lldb-commits
mailing list