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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Utility/Args.h:121
 
+  bool GetFlattenWindowsCommandString(std::string &command) const;
+
----------------
Hui wrote:
> labath wrote:
> > labath wrote:
> > > Hui wrote:
> > > > labath wrote:
> > > > > I am sorry for being such a pain, but I don't think this is grammatically correct, as `get` and `flatten` are both verbs. I'd call this either GetFlatten**ed**WindowsCommandLine or just plain FlattenWindowsCommandLine.
> > > > There are  two kinds of sources of args in this discussion:
> > > > 
> > > >   - from lldb console through stdin which is not raw.
> > > >   - from LLDB_SERVER_LOG_FILE environment variables which are raw
> > > > 
> > > > We expect to call a GetFlattenedWindowsCommandString for raw input. However it seems not the case for now. 
> > > > 
> > > > What about adding a TODO in the following in ProcessWindowsLauncher. 
> > > > 
> > > > Would like a solution to proceed.
> > > > 
> > > > +
> > > > +bool GetFlattenedWindowsCommandString(Args args, std::string &command) {
> > > > +  if (args.empty())
> > > > +    return false;
> > > > +
> > > > +  std::vector<llvm::StringRef> args_ref;
> > > > +  for (auto &entry : args.entries())
> > > > +     args_ref.push_back(entry.ref);
> > > > +
> > > > +  // TODO: only flatten raw input.
> > > > +  // Inputs from lldb console through the stdin are not raw, for example,
> > > > +  // A command line like "dir c:\" is attained as "dir c":\\". Trying to flatten such
> > > > +  // input will result in unexpected errors. In this case, the flattend string will be
> > > > +  // interpreted as "dir c:\\ which is a wrong usage of `dir` command.
> > > > +
> > > > +  command = llvm::sys::flattenWindowsCommandLine(args_ref);
> > > > +  return true;
> > > >  }
> > > > +} // namespace
> > > > 
> > > I am afraid you lost me here. By the time something gets to this function, it has already been parsed into individual argv strings. I am not sure which ones do you consider raw, or why should it make a difference. (lldb builtin interpreter has a concept of "raw" commands, but I don't think that's what you mean here)
> > > 
> > > This function should take those argv strings, no matter what they are, and recompose them into a single string. If those strings are wrong, then it's output will also be wrong, but that's not a problem of this function -- it's a problem of whoever parsed those strings.
> > > 
> > > I won't have access to a windows machine this week to check this out, but I can take a look at that next week. In the mean time, I would be fine with just xfailing the single test which fails because of this. I think that's a good tradeoff, as this would fix a bunch of other tests as well as unblock you on your lldb-server work.
> > I can't test this out, but the place I'd expect the problem to be is in `ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell`. This function seems to be blissfully unaware of the windows quoting rules, and yet it's the one that turns `platform shell dir c:\` into `cmd`, `/c` `whatever`. The flatten function then gets this argv array and flattens it one more time. I am not sure if that's the right thing to do on windows.
> It is observed when you type "dir c:\" on lldb console, the IOHandlerEditline::GetLine will yield "dir c:\\" for you through the standard input (I skipped the new line char '\n'). And the CommandInterpreter::ResolveCommandImpl won't get the raw command line string, i mean "dir c:\", even I force WantsRawCommandString() true to get one.
How are you observing that? Are you sure that's what happens, and it's not just the lldb formatting of c strings (when lldb displays a `const char *` variable, it will automatically escape any backslashes within, which means the backslashes will appear doubled).

I'd be surprised if extra backslashes appear at this level (they certainly don't on linux), as plenty of other things would fail if this didn't work.


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

https://reviews.llvm.org/D56230