[lldb/windows] Make "anonymous" pipe names more unique

[labath]

Using a "random" name for an "anonymous" pipe seems to be the state of the art on windows (according to stack overflow, new windows versions may have something better, but it involves calling kernel APIs directly and generally a lot of dark magic).

The problem with the current method was that is does not produce unique names if one has two copies of the pipe code in the same process, which is what happened with #120457 (because liblldb only exposes the public api, and we've started using the pipe code in lldb-dap as well).

This patch works around the problem by adding the address of the counter variable to the pipe name.

Replicating the multiple-copies setup in a test would be very difficult, which is why I'm not adding a test for this scenario.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

Using a "random" name for an "anonymous" pipe seems to be the state of the art on windows (according to stack overflow, new windows versions may have something better, but it involves calling kernel APIs directly and generally a lot of dark magic).

The problem with the current method was that is does not produce unique names if one has two copies of the pipe code in the same process, which is what happened with #120457 (because liblldb only exposes the public api, and we've started using the pipe code in lldb-dap as well).

This patch works around the problem by adding the address of the counter variable to the pipe name.

Replicating the multiple-copies setup in a test would be very difficult, which is why I'm not adding a test for this scenario.

---

Full diff: https://github.com/llvm/llvm-project/pull/123905.diff

1 Files Affected:

• (modified) lldb/source/Host/windows/PipeWindows.cpp (+2-3)

diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp
index 21e30f0ae87384..e95007ae8fd163 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -71,9 +71,8 @@ Status PipeWindows::CreateNew(bool child_process_inherit) {
   // cannot get overlapped i/o on Windows without using a named pipe.  So we
   // synthesize a unique name.
   uint32_t serial = g_pipe_serial.fetch_add(1);
-  std::string pipe_name;
-  llvm::raw_string_ostream pipe_name_stream(pipe_name);
-  pipe_name_stream << "lldb.pipe." << ::GetCurrentProcessId() << "." << serial;
+  std::string pipe_name = llvm::formatv(
+      "lldb.pipe.{0}.{1}.{2}", GetCurrentProcessId(), &g_pipe_serial, serial);
 
   return CreateNew(pipe_name.c_str(), child_process_inherit);
 }

[slydiman]

@labath

The problem with the current method was that is does not produce unique names if one has two copies of the pipe code in the same process

Do you mean PipeWindows::CreateNew(child_process_inherit) called from different threads may produce the same pipe_name?
Can you provide the sample values of pipe_name before and after this patch and explain the difference, please?

[slydiman]

If two copies of the pipe code means 2 instances of g_pipe_serial in different dlls, probably it is better to use GetTickCount() or such.

[labath]

Yes, the issue is two instances of the atomic counter in two libraries (well, one of them is an executable). I'm not sure why GetTickCount would be a better solution though. It returns time in milliseconds, which definitely too coarse-grained. Even if we used an API with greater resolution (I don't know which, but I assume Windows has one), I don't think we can completely rule out collisions.

I've recently been porting some code to aarch64, and one of the things I ran into was that (some) arm cpus have a (relatively) low frequency of the hardware clock, which broke code which assumes that functions like absl::Now() return monotonically increasing values (usually code which tried to measure the throughput of something, which then crashed due to division by zero).

So even if we went with something time-based, I would would still want to add some sort of a backup counter for an additional source of entropy. However, if we have that, I'm not sure if the time is useful. The new pipe name is lldb.pipe.<pid>.<address_of_atomic>.<value_of_atomic> (the old one was without the middle member), which seems unique enough (first value identifies a process, second one an address within that process and the last one a value at that address).

[slydiman]

Ah, I missed address_of_atomic. Sorry.
LGTM then.