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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
