Page MenuHomePhabricator

Use GUIDs to make pipe names unique on Windows.
ClosedPublic

Authored by amccarth on Feb 9 2015, 9:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth updated this revision to Diff 19592.Feb 9 2015, 9:47 AM
amccarth retitled this revision from to Use GUIDs to make pipe names unique on Windows..
amccarth updated this object.
amccarth edited the test plan for this revision. (Show Details)
amccarth added a reviewer: zturner.
amccarth added a subscriber: Unknown Object (MLST).
zturner edited edge metadata.Feb 9 2015, 10:32 AM

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 updated this revision to Diff 19614.Feb 9 2015, 2:08 PM
amccarth edited edge metadata.

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 added inline comments.Feb 9 2015, 2:37 PM
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 updated this revision to Diff 19624.Feb 9 2015, 4:04 PM

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.

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.

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.

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

This revision was automatically updated to reflect the committed changes.