zturner added inline comments.
================
Comment at: include/lldb/Host/windows/PosixApi.h:78-79
+#define WNOHANG 1
+#define WUNTRACED 2
+
----------------
krytarowski wrote:
> I think that these symbols should not be leaked here in the first place.
In general we should avoid mocking posix APIs for Windows. If something is
currently implemented in terms of a Posix API that doesn't exist on Windows, we
should provide an implementation of that function with an entirely new name and
platform-agnostic set of parameters whose interface need not resemble the posix
interface in any way..
For example, I see this is used in `lldb-platform.cpp` when we call
`waitpid()`. Instead, we could add something like `void
WaitForAllChildProcessesToExit();` and then just implement that function on
posix and Windows completely separately. This is more flexible and portable
than trying to fit Windows semantics into a posix api, which doesn't always
work.
================
Comment at: include/lldb/Host/windows/PosixApi.h:108-111
+inline pid_t waitpid(pid_t pid, int *status, int options) {
+ // To be implemented.
+ return pid_t(-1);
+}
----------------
labath wrote:
> What are your plans for this?
>
> I assume you needed this for the fork-server in lldb-platform.cpp. I think
> the best way to address that would be to change the code from spawning a
> separate process for each connection to having a new thread for each
> connection. That should be much more portable and will avoid the need to mock
> posix apis on windows. I can help you with that if you run into any problems
> there.
Are you ok with the behavioral change? What if one connection crashes?
Assuming we wanted to implement this behavior on Windows, the way to do it
would be to create all child processes in a single "job" (aka process group),
as that is the best way to get notifications for when all childs have
terminated.
================
Comment at: source/Host/common/SocketAddress.cpp:263-267
+#ifdef _WIN32
+ error.SetError(err, lldb::eErrorTypeWin32);
+#else
+ error.SetError(err, lldb::eErrorTypePOSIX);
+#endif
----------------
Perhaps we could define something like `lldb::eErrorTypeHostNative` to avoid
this ifdef.
================
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) {
----------------
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?
================
Comment at: tools/lldb-server/SystemInitializerLLGS.cpp:58-61
+#if defined(_WIN32)
+ if (g_ref_count && --g_ref_count == 0)
+ WSACleanup();
+#endif
----------------
I don't think Initialize and Terminate need to be re-entrant. So we can
probably delete the ref count code here
================
Comment at: tools/lldb-server/lldb-server.cpp:40
-static void initialize() {
+namespace llsvr {
+void initialize() {
----------------
labath wrote:
> In other places we use `llgs` (for `LL`db `G`db `S`erver), so I suggest
> sticking to that.
The `static` from before indicates that the function should not have external
linkage, so I think we should keep that. Whether we put it in a namespace or
not is orthogonal, but for global functions in cpp functions, we generally
prefer to mark them all static.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56233/new/
https://reviews.llvm.org/D56233
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
