[Lldb-commits] [PATCH] Fix warning about the use of mktemp and make platform agnostic by adding and using PipeBase::CreateWithUniqueName.

[Robert Flack]

In http://reviews.llvm.org/D7348#118521, @zturner wrote:

> Hi Robert, by any chance did you actually try compiling this on Windows?
>  The reason I ask is that it just occurred to me that this will now require
>  linking in rpcrt4.lib, which I don't think happens by default.  So this
>  will cause linker errors.  If you tested it and it compiled, then no
>  problem.  But if you didn't test it, then I think it's better to just go
>  back to the way it was before, and I will do the UuidToString stuff later.


I did, but it didn't get as far as the linking phase due to an unrelated error:

D:\src\ll\llvm\tools\lldb\source\Target\ProcessLaunchInfo.cpp(347) : error C2065: 'O_CLOEXEC' : undeclared identifier
[125/270] Building CXX object tools\ll...iles\lldbTarget.dir\ThreadPlan.cpp.obj
ninja: build stopped: subcommand failed.

So I'm not sure if there will be linker errors. I can go back to the way it was before on this.


REPOSITORY
  rL LLVM

================
Comment at: include/lldb/Host/posix/PipePosix.h:83
@@ -75,2 +82,3 @@
     int m_fds[2];
+    llvm::SmallString<PATH_MAX> m_name;
 };
----------------
zturner wrote:
> I think this would be better as an std::string.  The reason being that this means PipePosix is now going to take a huge amount of stack space, and it's not uncommon for pipes to be allocated on the stack.  Since the Pipe isn't used in any performance intensive code anyway, the extra heap allocation isn't really a big deal and it reduces the size of this class by a lot.
Makes sense, thanks. I've done this in the latest patch, although an alternate idea would be to add the generated name as an out parameter of CreateWithUniqueName avoiding storing it as a member at all.

================
Comment at: source/Host/windows/PipeWindows.cpp:97-107
@@ +96,13 @@
+{
+    llvm::SmallString<128> name;
+    Error error;
+    do {
+        name = prefix;
+        name += "-";
+        for (unsigned i = 0; i < 6; ++i) {
+            name += "0123456789abcdef"[llvm::sys::Process::GetRandomNumber() & 15];
+        }
+        Error error = CreateNew(name, child_process_inherit);
+    } while (error.GetError() == ERROR_ALREADY_EXISTS);
+    return error;
+}
----------------
zturner wrote:
> I would rather use UuidCreate [https://msdn.microsoft.com/en-us/library/windows/desktop/aa379205%28v=vs.85%29.aspx] and then UuidToString [https://msdn.microsoft.com/en-us/library/windows/desktop/aa379352(v=vs.85).aspx].  But if this compiles, and you don't feel comfortable / aren't able to make this change and test it on Windows, I can probably do that.
Done, thanks for the pointers! I got a Windows machine up and building and was able to verify that my changes in the latest patch compile however it looks like the compile on tip of tree is broken again so I haven't been able to test it yet.

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:785
@@ +784,3 @@
+                    return error;
+                strncpy(named_pipe_path, port_named_pipe.GetName().str().c_str(), PATH_MAX);
+                debugserver_args.AppendArgument("--named-pipe");
----------------
zturner wrote:
> Should this be strncpy?  I feel like we should just be appending the argument directly, regardless of how long it is.  So nuke this whole named_pipe_path variable, and instead just call AppendArgument(port_named_pipe.GetName())
Done.

http://reviews.llvm.org/D7348

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/