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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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

> Hello labath, I am thinking maybe it is just because the windows command 'dir' can't interpret "\\" in the root directory path. For example,
>
> This is working
>  "C:\\WINDOWS\\system32\\cmd.exe" /C " dir **c:\**Users\\abc"
>
>   This is not working
>
> "C:\\WINDOWS\\system32\\cmd.exe" /C " dir **c:\\**Users\\abc"
>
> Other commands, like **ls** seems not have this problem.
>
> So could we just go ahead with  llvm::sys::flattenWindowsCommandLine but set a red light on 'dir' command only, at least handle it specifically?


Sorry about the delay.

I've done some experiments of my own, and my conclusion is that the "special" part here is not the "dir" command but cmd.exe itself. It handles quotes differently than typical windows applications. It behavior is documented like this:

  1.  If all of the following conditions are met, then quote characters
      on the command line are preserved:
  
      - no /S switch
      - exactly two quote characters
      - no special characters between the two quote characters,
        where special is one of: &<>()@^|
      - there are one or more whitespace characters between the
        the two quote characters
      - the string between the two quote characters is the name
        of an executable file.
  
  2.  Otherwise, old behavior is to see if the first character is
      a quote character and if so, strip the leading character and
      remove the last quote character on the command line, preserving
      any text after the last quote character.

So given these rules, and the lldb command `platform shell dir c:\`, the correct string to pass to CreateProcess would be `cmd.exe /c "dir c:\"`. Conceptually, that is easy to achieve, but it practice that's a bit hard because the road from "platform shell" to CreateProcess is long and winding:

1. first, the string `dir c:\` gets converted to a Args array in Host::RunShellCommand (the `const char *` version). This step uses the "lldb" quoting rules and results in something like `dir`, `c:\`.
2. then the Args array gets transmogrified in ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell. This uses some approximation of the posix shell quoting rules, but I believe that is not fully correct for posix shells either. The result is `cmd.exe`, `/c`, `dir c:\` (IIRC).
3. finally, we call `flattenWindowsCommandLine`. This uses the standard windows quoting rules, which gives us the string `cmd.exe /c "dir c:\\"

So, I think the proper solution here would be to find a way to skip steps 1. and 2. They're completely unnecessary even in the posix case -- the user types a string, the shell expects a string, we should be able to just pass that string from one place to the other and not play around with parsing and reconstituting it. Then, it will be easy to construct the proper command line for windows, or the right argv array in the posix case. This will probably involve having ProcessLaunchInfo be backed by either an argv array or a plain string.

However, this is getting pretty far from the original intention of your patch. For the time being I would be happy with checking in the present version of your patch, with the knowledge that this breaks some "platform shell" use cases. I don't think that's too bad, because "platform shell" is not the most important feature in lldb, and it's not fully correct now anyway. In return for that, we would get all tests in TestQuoting.py passing.


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

https://reviews.llvm.org/D56230