[lldb/Driver] Fix handling on positional arguments

[JDevlieghere]

Summary

Before the transition to libOption it was possible to specify arguments for the inferior without -- as long as they didn't start with a dash.

For example, the following invocations should all behave the same:

$ lldb inferior inferior-arg
$ lldb inferior -- inferior-arg
$ lldb -- inferior inferior-arg

This patch fixes that behavior and adds a test to cover the different combinations.

[JDevlieghere]

One thing the current approach allows is foo bar -o 'run' baz which will set target.run-args to "bar" "baz" . I guess that's not really desirable, but I'm not sure it outweighs the added complexity of tracking the beginning and thee end.

[clayborg]

So when talking about "prior to libOption", are we talking getopt()? I believe that getopt would have an issue with:

$ lldb inferior --inferior-arg 

As it would not understand the argument. Or was there an intermediate llvm option parser we were using after getopt()?

And BTW, getopt allows arguments and options to be interspersed:

$ lldb --lldb-option1 inferior --lldb-option2=123 inferior-arg1 -- --inferior-opt=444 inferior-arg2

[jingham]

Not sure why you need to track "beginning and end". Does libOption not pull out all the options (and their option values if they have them) before handing you the command line? If not, then this probably is not worth doing. But if it does, this should be straightforward since the only arguments lldb takes (prior to seeing a --) are meant to be arguments to the program when you run it.

[JDevlieghere]

Apparently I misremembered the old behavior. Jim refreshed my memory about what he expects the behavior to be link. I've reworked the patch to match that. The result is actually less complex than it was before.

Please ignore the old patch & description.

[JDevlieghere]

Add test for lldb inferior -b inferior-arg -- inferior-arg2 as Greg pointed out.

[JDevlieghere]

Document the behavior in the help and man page.

[jingham]

LGTM. These were useful behaviors, thanks for restoring them!

[clayborg]

Much better!

[labath]

Thanks for fixing this. I remember running into this a couple of times, but I never got around to do something about it...

lldb/test/Shell/Driver/TestPositionalArgs.test

27–28

Unrelated to this patch, but.. do we actually want this behavior? I don't think these were just "warnings" before libOption. Given that clang treats (at least some) unknown options as errors, I'm guessing there must be some libOption knob for controlling the behavior of this...

[JDevlieghere]

lldb/test/Shell/Driver/TestPositionalArgs.test

27–28

What behavior do you have in mind? Make these errors and exit early? It's just us reporting the warning, we can do anything we want with it.

[labath]

lldb/test/Shell/Driver/TestPositionalArgs.test

27–28

Yes, I think it would be more reasonable to report an error and exit in case we encounter an argument we don't recognise.

[jingham]

I agree with Pavel, reporting an error and exiting seems like a good behavior. When I mistype a command line option, I generally immediately quit lldb, up-arrow and fix and resubmit, so this would save keystrokes.

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

[labath]

In D80165#2044509, @jingham wrote:

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

[jingham]

In D80165#2045882, @labath wrote:

In D80165#2044509, @jingham wrote:

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Jonas said this wasn't part of libOption, but was done by hand in clang.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

You omitted the bit of text that would show whether it quit or not...

[labath]

In D80165#2047241, @jingham wrote:

In D80165#2045882, @labath wrote:

In D80165#2044509, @jingham wrote:

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Jonas said this wasn't part of libOption, but was done by hand in clang.

Oh, that's too bad.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

You omitted the bit of text that would show whether it quit or not...

It did quit. With exit code 1. :)

[jingham]

In D80165#2048458, @labath wrote:

In D80165#2047241, @jingham wrote:

In D80165#2045882, @labath wrote:

In D80165#2044509, @jingham wrote:

We should make sure if we do exit that we don't output any other text that would obscure the error message. It should be easy to spot the error both so you can easily fix it and to prevent you from typing lldb commands into your shell. If libOption had a "nearest option name to the one you typed" facility that would be useful in this case as well.

That facility exists. Clang uses it for command-line "fix-its". I am not sure what it takes to make use of it.

Jonas said this wasn't part of libOption, but was done by hand in clang.

Oh, that's too bad.

Does anybody remember what gdb does when you mistype a command-line option? We're not at all required to model their behavior, but it would be interesting to consider.

$ gdb -foobar
gdb: unrecognized option '-foobar'
Use `gdb --help' for a complete list of options.

You omitted the bit of text that would show whether it quit or not...

It did quit. With exit code 1. :)

Great, then we all agree that exiting is the right thing to do!

lldb/docs/man/lldb.rst

 
  Tells the debugger to use the specified scripting language for user-defined scripts.
 
 EXAMPLES
 --------
 
 The debugger can be started in several modes.
 
-Passing an executable as a positional argument prepares :program:`lldb` to
-debug the given executable. Arguments passed after -- are considered arguments
-to the debugged executable.
+Passing an executable as a positional argument prepares lldb to debug the given
+executable. To disambiguate between arguments passed to lldb and arguments
+passed to the debugged executable, arguments starting with a - must be passed
+after --.
 
-  lldb --arch x86_64 /path/to/program -- --arch arvm7
+  lldb --arch x86_64 /path/to/program program argument -- --arch arvm7
+
+For convenience, passing the executable after -- is also supported.
+
+  lldb --arch x86_64 -- /path/to/program program argument --arch arvm7
 
 Passing one of the attach options causes :program:`lldb` to immediately attach
 to the given process.
 
   lldb -p <pid>
   lldb -n <process-name>
 
 Passing --repl starts :program:`lldb` in REPL mode.

lldb/test/Shell/Driver/TestNoUseColor.test

-# RUN: %lldb --no-use-color -s %s | FileCheck %s
+# RUN: %lldb --no-use-colors -s %s | FileCheck %s
 settings show use-color
 # CHECK: use-color (boolean) = false
 q

lldb/test/Shell/Driver/TestPositionalArgs.test

+RUN: echo "int main() { return 0; }" | %clang_host -x c - -o %t.foo
+
+RUN: %lldb -x -b %t.foo bar baz quux | FileCheck %s
+RUN: %lldb -x -b -- %t.foo bar baz quux | FileCheck %s
+RUN: %lldb -x -b %t.foo -- bar baz quux | FileCheck %s
+RUN: %lldb -x -b %t.foo bar -- baz quux | FileCheck %s
+RUN: %lldb -x -b %t.foo bar baz -- quux | FileCheck %s
+RUN: %lldb %t.foo -x bar -b baz -- quux | FileCheck %s
+RUN: %lldb -x -b -f %t.foo bar baz quux | FileCheck %s
+RUN: %lldb -x -b -f %t.foo -- bar baz quux | FileCheck %s
+RUN: %lldb -x -b -f %t.foo bar -- baz quux | FileCheck %s
+RUN: %lldb -x -b -f %t.foo bar baz -- quux | FileCheck %s
+
+CHECK: Current executable set to {{.*}}foo
+CHECK: target.run-args "bar" "baz" "quux"
+
+RUN: %lldb -x -b %t.foo -- bar -baz --quux | FileCheck %s --check-prefix DASH
+RUN: %lldb -x -b -- %t.foo bar -baz --quux | FileCheck %s --check-prefix DASH
+RUN: %lldb -x -b -f %t.foo -- bar -baz --quux | FileCheck %s --check-prefix DASH
+
+DASH: Current executable set to {{.*}}foo
+DASH: target.run-args "bar" "-baz" "--quux"
+
+RUN: %lldb -x -b %t.foo bar -baz --quux 2>&1 | FileCheck %s --check-prefix UNKNOWN
+RUN: %lldb -x -b -f %t.foo bar -baz --quux 2>&1 | FileCheck %s --check-prefix UNKNOWN
+
+UNKNOWN: warning: ignoring unknown option: -baz
+UNKNOWN: warning: ignoring unknown option: --quux
+UNKNOWN: Current executable set to {{.*}}foo
+UNKNOWN: target.run-args "bar"

lldb/tools/driver/Driver.cpp

if (arg->getOption().matches(OPT_one_line_before_file)) {
       if (error.Fail())
         return error;
     }
   }
 
   if (m_option_data.m_process_name.empty() &&
       m_option_data.m_process_pid == LLDB_INVALID_PROCESS_ID) {
 
-    // If the option data args array is empty that means the file was not
-    // specified with -f and we need to get it from the input args.
-    if (m_option_data.m_args.empty()) {
-      if (auto *arg = args.getLastArgNoClaim(OPT_INPUT)) {
-        m_option_data.m_args.push_back(arg->getAsString((args)));
-      }
-    }
+    for (auto *arg : args.filtered(OPT_INPUT))
 
     // Any argument following -- is an argument for the inferior.
     if (auto *arg = args.getLastArgNoClaim(OPT_REM)) {
       for (auto value : arg->getValues())
         m_option_data.m_args.emplace_back(value);
     }
   } else if (args.getLastArgNoClaim() != nullptr) {
     WithColor::warning() << "program arguments are ignored when attaching.\n";
static void printHelp(LLDBOptTable &table, llvm::StringRef tool_name) {
   std::string usage_str = tool_name.str() + " [options]";
   table.PrintHelp(llvm::outs(), usage_str.c_str(), "LLDB", false);
 
   std::string examples = R"___(
 EXAMPLES:
   The debugger can be started in several modes.
 
   Passing an executable as a positional argument prepares lldb to debug the
-  given executable. Arguments passed after -- are considered arguments to the
-  debugged executable.
+  given executable. To disambiguate between arguments passed to lldb and
+  arguments passed to the debugged executable, arguments starting with a - must
+  be passed after --.
+
+    lldb --arch x86_64 /path/to/program program argument -- --arch arvm7
+
+  For convenience, passing the executable after -- is also supported.
 
-    lldb --arch x86_64 /path/to/program -- --arch arvm7
+    lldb --arch x86_64 -- /path/to/program program argument --arch arvm7
 
   Passing one of the attach options causes lldb to immediately attach to the
   given process.
 
     lldb -p <pid>
     lldb -n <process-name>
 
   Passing --repl starts lldb in REPL mode.