Hui added inline comments.
================ Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string &command) const; + ---------------- labath wrote: > Hui wrote: > > labath wrote: > > > 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. > > I set the breakpoint through MSVC, triggered it by typing the command "dir > > c:\" on lldb console and the value I read was "dir c:\\\n". > Isn't MSVC doing the same thing here? Otherwise, how would you be able to > tell a line feed character ("\n") from a literal backslash followed by the > letter N? My understanding of the IOHandlerEditline::GetLine function is it calls fgets to get the input string which is stored as "dir c:\\\n" and the tailing char ('\n') is stripped out intentionally so a "dir c:\\" is the leftover. 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