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

Pavel Labath via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 21 17:13:23 PST 2019


labath added inline comments.


================
Comment at: include/lldb/Utility/Args.h:121
 
+  bool GetFlattenWindowsCommandString(std::string &command) const;
+
----------------
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.


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

https://reviews.llvm.org/D56230





More information about the llvm-commits mailing list