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