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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D56230#1354829 <https://reviews.llvm.org/D56230#1354829>, @Hui wrote:

> I think the key problem here is to make sure the argument will be treated as a single argument to the process launcher.
>
> To be specific to this case only, could we just provide a quote char to argument log file path and log channels on Windows?
>
> The downside is one more #if is introduced.
>
> #ifdef _WIN32
>  char quote_char= '"';
>  #else
>  char quote_char='\0';
>  #endif
>
>    std::string env_debugserver_log_channels =
>        host_env.lookup("LLDB_SERVER_LOG_CHANNELS");
>    if (!env_debugserver_log_channels.empty()) {
>      debugserver_args.AppendArgument(
>          llvm::formatv("--log-channels={0}", env_debugserver_log_channels)
>              .str(), quote_char);
>   }


I almost feel like the `Args` class could be made smarter here.  Because all of the proposed solutions will still not work correctly on Linux.  For example, why is this Windows specific?   `Args::GetCommandString()` is very dumb and just loops over each one appending to a string.  So if your log file name contains spaces on Linux, you would still have a problem no?

Currently the signature of `AppendArgument()` takes an argument string, and a quote char, which defaults to `'\0'` which means no quote.  But there is only one call-site of that entire function out of many hundreds of other calls to the function, most of which just pass nothing.  This means that all of those other call-sites are broken if their argument contains a space (many are string literals though, so it won't be a problem there).

But why don't we just split this into two functions:

  Args::QuoteAndAppendArgument(const Twine &arg);
  Args::AppendArgument(const Twine &arg);

and the first one will auto-detect the quoting style to use based on the host operating system.  This way we would fix the problem for all platforms, not just Windows, and we could also use the function to fix potentially many other bugs at all the callsites where we append unquoted arguments.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56230