[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added inline comments.
This revision now requires changes to proceed.


================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1138
+      // Double quote the string.
+      ::snprintf(arg_cstr, sizeof(arg_cstr), "--log-channels=\"%s\"",
+                 env_debugserver_log_channels.c_str());
----------------
zturner wrote:
> Hui wrote:
> > This change is essential others seem not that necessary but nice to have in order to support multiple systems.
> I would prefer to avoid using C-style formatting if possible.  LLVM has some nice formatting routines that are safer and easier to understand.
> 
> While we're here, perhaps we could change `char arg_cstr[PATH_MAX];` up at the top of this function to `std::string arg_str;`
> 
> Regardless, we can re-write this in a number of ways, such as:
> ```
> arg_str = llvm::formatv("--log-channels = \"{0}\", env_debugserver_log_channels).str();
> arg_str = (Twine("--log-channels = \"") + env_debugserver_log_channels + "\"").str();
> arg_str = llvm::format("--log-channels = \"%s\"", env_debugserver_log_channels);
> ```
> 
> to get rid of this snprintf call.
Actually, this shouldn't be done here at all. The assumption here is that the contents of the `Args` vector will get passed verbatim to the argc+argv of the called program. On unix systems we will do just that, so if you add the quotes here they will be passed the lldb-server, and then the parse there will fail because we will get extra quotes we didn't expect.

Since Windows needs special logic in order to ensure this, that logic should be inside windows-specific code, probably ProcessLauncherWindows. That will make all other instances of launching processes with spaces work, not just this particular case. It seems ProcessLauncherWindows uses Args::GetQuotedCommandString to do this job, but this function seems extremely under-equipped for this job. I'd suggest implementing a windows-specific version of this in ProcessLauncherWindows, which will do whatever fixups are necessary to make this happen.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56230/new/

https://reviews.llvm.org/D56230