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

[Hui Huang via Phabricator via lldb-commits]

Hui added inline comments.


================
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());
----------------
labath wrote:
> 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.
It is not that applicable for the windows process launcher to determine which entry in the args needs to be quoted unless given very specific flag or option. I think this issue shall be documented and ask user to be responsible for setting the "right" variables. For example, 

on Windows PS,
$env:LLDB_SERVER_LOG_CHANNELS="""lldb all:windows all"""

instead of
$env:LLDB_SERVER_LOG_CHANNELS="lldb all:windows all"

on MSVC, 
LLDB_DEBUGSERVER_LOG_FILE="d:\\a b c\\log.txt" 

instead of
LLDB_DEBUGSERVER_LOG_FILE=d:\\a b c\\log.txt 
 




Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56230