Use GUIDs to make pipe names unique on Windows.

[zturner]

General comment. It's usually good to create diffs with -U999999. This will include the entire file that changed in the diffs, as opposed to just changed lines. Useful for generating more diff context so reviewer can see more of the file. (Although in this case it's my fault for forgetting to tell you about that).

source/Host/windows/PipeWindows.cpp

100–101 ↗

(On Diff #19592)

Don't use global scope identifiers before typenames unless it's necessary to disambiguate. The same rule doesn't apply to functions though, ::GlobalFunction() is ok.

102–103 ↗

(On Diff #19592)

Is it correct to use SUCCEEDED() / FAILED() with RPC calls? They dont' return HRESULTs. Seems like the return values are just standard windows error codes.

115 ↗

(On Diff #19592)

I would just set the error to the return code of whichever call failed. Normally we use GetLastError() here, but The RPC functions seem to return the error value directly.

[amccarth]

Addressed review comments.

source/Host/windows/PipeWindows.cpp

100–101 ↗

(On Diff #19592)

It is necessary to disambiguate for UUID. I've removed it from RPC_CSTR.

102–103 ↗

(On Diff #19592)

Whoops. I thought RPC_STATUS was an SCODE, which is the same as an HRESULT. I've fixed the code to check for multiple possible successful results.

115 ↗

(On Diff #19592)

Done.

When I thought it was an HRESULT, there was no obvious way to map an HRESULT back to a Win32 error. Now I see that RPC_STATUSes are Win32 errors, so I'm returning the status directly.

[zturner]

source/Host/windows/PipeWindows.cpp

109 ↗

(On Diff #19614)

Does this just only append the first character to the string? Seems like it should just be unique_string. Otherwise lgtm. I can commit later.

[amccarth]

Corrected pointer dereference. The reinterpret_cast is necessary because RPC_CSTR is a pointer to unsigned char and llvm::StringRef doesn't have a constructor for that. If I ditch the RPC_CSTR and just declare unique_string as a pointer to char, then I'd have to cast it in the call to UuidToString, which is uglier.

[zturner]

In D7509#120920, @amccarth wrote:

Corrected pointer dereference. The reinterpret_cast is necessary because RPC_CSTR is a pointer to unsigned char and llvm::StringRef doesn't have a constructor for that. If I ditch the RPC_CSTR and just declare unique_string as a pointer to char, then I'd have to cast it in the call to UuidToString, which is uglier.

If RPC_CSTR is a pointer to unsigned char*, then it sounds like it's probably unicode. In that case the += operator on SmallString<> won't work correctly. We need to use llvm::convertUTF16ToUTF8String() in that case.

[amccarth]

In D7509#120962, @zturner wrote:

In D7509#120920, @amccarth wrote:

Corrected pointer dereference. The reinterpret_cast is necessary because RPC_CSTR is a pointer to unsigned char and llvm::StringRef doesn't have a constructor for that. If I ditch the RPC_CSTR and just declare unique_string as a pointer to char, then I'd have to cast it in the call to UuidToString, which is uglier.

If RPC_CSTR is a pointer to unsigned char*, then it sounds like it's probably unicode. In that case the += operator on SmallString<> won't work correctly. We need to use llvm::convertUTF16ToUTF8String() in that case.

Nope. RPC_CSTR is a pointer to unsigned char (not a pointer to a pointer).

I explicitly call UuidToStringA (the ANSI version of the function), which should get single byte characters in the current code page. Since GUIDs don't have any non-ASCII characters, so there should be no high-bit characters and the code page should be irrelevant. I don't think any conversion should be necessary.

I'd love to step through this code to verify, but I can't seem to find a test that exercises this code.

[zturner]

Ahh you're right, weird choice of character type though. I'll commit this tomorrow morning.

lldb/trunk/cmake/LLDBDependencies.cmake

 
 # Windows-only libraries
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
   list(APPEND LLDB_USED_LIBS
     lldbPluginProcessWindows
     lldbPluginProcessElfCore
     lldbPluginJITLoaderGDB
     Ws2_32
+    Rpcrt4
     )
 endif ()
 
 # Linux-only libraries
 if ( CMAKE_SYSTEM_NAME MATCHES "Linux" )
   list(APPEND LLDB_USED_LIBS
     lldbPluginProcessLinux
     lldbPluginProcessPOSIX

lldb/trunk/source/Host/windows/PipeWindows.cpp

 #include "lldb/Host/windows/PipeWindows.h"
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <fcntl.h>
 #include <io.h>
+#include <rpc.h>
 
 #include <atomic>
 #include <string>
 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace
PipeWindows::CreateNew(llvm::StringRef name, bool child_process_inherit)
     return result;
 }
 
 Error
 PipeWindows::CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name)
 {
     llvm::SmallString<128> pipe_name;
     Error error;
-    do {
+    ::UUID unique_id;
+    RPC_CSTR unique_string;
+    RPC_STATUS status = ::UuidCreate(&unique_id);
+    if (status == RPC_S_OK || status == RPC_S_UUID_LOCAL_ONLY)
+        status = ::UuidToStringA(&unique_id, &unique_string);
+    if (status == RPC_S_OK)
+    {
         pipe_name = prefix;
         pipe_name += "-";
-        for (unsigned i = 0; i < 6; i++) {
-            pipe_name += "0123456789abcdef"[llvm::sys::Process::GetRandomNumber() & 15];
-        }
-        Error error = CreateNew(pipe_name, child_process_inherit);
-    } while (error.GetError() == ERROR_ALREADY_EXISTS);
+        pipe_name += reinterpret_cast<char *>(unique_string);
+        ::RpcStringFreeA(&unique_string);
+        error = CreateNew(pipe_name, child_process_inherit);
+    }
+    else
+    {
+        error.SetError(status, eErrorTypeWin32);
     if (error.Success())
         name = pipe_name;
     return error;
 }
 
 Error
 PipeWindows::OpenAsReader(llvm::StringRef name, bool child_process_inherit)
 {