[lldb] Add -l/--language option to script command

[JDevlieghere]

Summary

Make it possible to run the script command with a different language than currently selected.

$ ./bin/lldb -l python
(lldb) script -l lua
>>> io.stdout:write("Hello, Wolrd!\n")
Hello, Wolrd!

Because of the ability to pass anything after the script command, it is implemented as a CommandObjectRaw. In a first attempt I tried to change this to a CommandObjectParsed in order to have the existing infrastructure parse the language argument and have the script contents as the first argument. That didn't work out, first because it unconditionally removed quotes from within the script expression and second because it attempts to expand script commands between backticks. Given how easy it is to parse the language argument, I think it's warranted to keep the CommandObjectRaw and just do the parsing manually.

[kastiglione]

lldb/source/Commands/CommandObjectScript.cpp

122

this is missing error handling for unsupported languages, such as script -l swift

lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test

8

is a.out not a universal default?

lldb/test/Shell/ScriptInterpreter/Python/python.test

9

So this SyntaxError is from python because it's attempting to evaluate --language invalid print(...)? I think it'd be good to handle unknown languages, but maybe you have a reason for this?

[kastiglione]

lldb/source/Commands/CommandObjectScript.cpp

107–108

generally, lldb supports more than space delimited flags. For example I often type expr -lobjc -- ... with no space. A quick check shows --language=objc is also supported. Should these cases be handled too?

[JDevlieghere]

lldb/source/Commands/CommandObjectScript.cpp

107–108

Yes, if that works for other command it should work for this command too. I was already on the fence about this, but supporting that would definitely tip the scales beyond the complexity I'm willing to accept to work around the limitations of CommandObjectParsed. I'll need to come up with a different solution.

122

I actually did that on purpose, I don't think either Python or Lua support statements that begin with -l or --language but I didn't want to intercept this unless it meant something to LLDB. But I agree that's not great UX.

lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test

8

Not sure about Windows, but regardless I prefer to be explicit.

[teemperor]

So CommandObjectRaw does support arguments, but they way it works is that you need to have a delimiter for the 'raw' part which is --. If you have that, then you can just use invoke the normal option parsing like CommandObjectExpression does. The handcrafted implementation here adds a completely new lldb command syntax which has a 'raw' part without the -- which seems inconsistent (not saying that the syntax here is worse or better than script --language foo --, but it's just inconsistent with the way the rest of LLDB works). It's obviously also missing the other quirky things that are part of the command syntax (e.g., quoting arguments isn't supported and so on).

[JDevlieghere]

• Remove custom parsing and use the command options insofar possible.
• Require -- as a delimiter when language and code are specified together.

[JDevlieghere]

In D86996#2251485, @teemperor wrote:

So CommandObjectRaw does support arguments, but they way it works is that you need to have a delimiter for the 'raw' part which is --. If you have that, then you can just use invoke the normal option parsing like CommandObjectExpression does. The handcrafted implementation here adds a completely new lldb command syntax which has a 'raw' part without the -- which seems inconsistent (not saying that the syntax here is worse or better than script --language foo --, but it's just inconsistent with the way the rest of LLDB works). It's obviously also missing the other quirky things that are part of the command syntax (e.g., quoting arguments isn't supported and so on).

Yes, I really wanted to avoid that, and after spending (way too much time) trying to hack that up I concluded that with the lossy conversion to Args that really wasn't an option (pun intended?). So using the delimiter is indeed what I ended up settling for. I did add a special case for the situation where you pass a language but no code, because I thought it would be silly to require you to type script --language lua -- to launch the interactive Lua interpreter.

[labath]

In D86996#2251508, @JDevlieghere wrote:

because I thought it would be silly to require you to type script --language lua -- to launch the interactive Lua interpreter.

For better or worse, this is how expr works:

(lldb) expr --show-types
error: <user expression 0>:1:3: use of undeclared identifier 'show'
--show-types
  ^
error: <user expression 0>:1:8: use of undeclared identifier 'types'
--show-types
       ^
(lldb) expr --show-types --
Enter expressions, then terminate with an empty line to evaluate:
  1: ^D

I agree that silly, but maybe the same fix should then be applied to the expr command (and any other command with similar behavior).

[JDevlieghere]

In D86996#2252246, @labath wrote:

I agree that silly, but maybe the same fix should then be applied to the expr command (and any other command with similar behavior).

Sure, we can use the same trick in other places that use OptionsWithRaw. I'll do that in a separate patch.

[teemperor]

LGTM

[JDevlieghere]

In D86996#2252520, @JDevlieghere wrote:

In D86996#2252246, @labath wrote:

I agree that silly, but maybe the same fix should then be applied to the expr command (and any other command with similar behavior).

Sure, we can use the same trick in other places that use OptionsWithRaw. I'll do that in a separate patch.

Actually, after discussion this with @teemperor offline, for the expo command we can't use that trick because a valid option might also be a valid expression. expr --flag would parse correctly if flag was an option, but it might also be an expression that decrements --flag.

[kastiglione]

lldb/source/Commands/CommandObjectScript.cpp

37

is the default's name available in a define? If maybe it'd be good to include it here? I don't actually know when/where this string gets printed.

[JDevlieghere]

lldb/source/Commands/CommandObjectScript.cpp

37

It's set dynamically as a debugger property, while the options here are static.

[labath]

In D86996#2252561, @JDevlieghere wrote:

In D86996#2252520, @JDevlieghere wrote:

In D86996#2252246, @labath wrote:

I agree that silly, but maybe the same fix should then be applied to the expr command (and any other command with similar behavior).

Sure, we can use the same trick in other places that use OptionsWithRaw. I'll do that in a separate patch.

Actually, after discussion this with @teemperor offline, for the expo command we can't use that trick because a valid option might also be a valid expression. expr --flag would parse correctly if flag was an option, but it might also be an expression that decrements --flag.

That's actually a very good point. But... that also applies to the script command. With python script --flag returns the value of the flag variable (negated twice). With lua, it executes the command "--flag", which is a comment. Neither of these are as useful as the c++ --flag, but they still create ambiguities. And I think these commands should disambiguate in the same way. No matter what we choose as the primary interpretation, the "other" meaning can always be obtained by adding a -- to the appropriate place, so it's only a matter of choosing the best default. Given the expr status quo, I'd stick with that.

[kastiglione]

Given the expr status quo, I'd stick with that.

I want to give a +1 to not copying expr. I think the tradeoff is not not worth it. My user perspective is that lldb could be improved by making more decisions with the user in mind, not lldb's engineers (to give some context). At this time, there's only -l/--language and I don't think the cases of ambiguity have enough likelihood to force the use of --.

Taking --language as an example, it's only valid python if language is a defined variable, and once you consider that it takes an argument, it becomes invalid in python (--language python or --language=python). For Lua, I can't think of practical cases for evaluating a comment, unless Lua uses comments for semantics/metaprogramming?. The common case of -l <language> <code> is not valid for either python or Lua.

I think it's a practical tradeoff to not require -- at this point. Will there be more flags added, I don't know, but if there are I also think it's not a major problem to require -- at a later time if the balances shift.

[teemperor]

I have to agree with @kastiglione that this isn't very intuitive. I doubt that more than a handful of users are aware that "raw commands" are a thing, so I think this inconsistency is for once in favor of the user. LLDB commands are anyway not meant to be stable, so we can just fix this if this ever turns out to be a real issue.

Having said that, I think this shouldn't be done here but in its own patch with a less ad hoc implementation and some dedicated parsing tests. Good thing the command parsing implementation is so great, so that will be lots of fun for @JDevlieghere

lldb/source/Commands/CommandObjectScript.cpp

99

We -> we.

[labath]

I'm not married to the current way we process commands, but I do value their consistency (both between different commands, and between a command and their documentation). This would make script behave differently than expr even though they have identical modes of operation (raw input, special handling of empty argument, etc.), and differently from its documentation. The help script command contains this:

Invoke the script interpreter with provided code and display any results.  Start the interactive
interpreter if no code is supplied.  Expects 'raw' input (see 'help raw-input'.)

I'm not sure how this is patch coded, but probably after the argument are added, this additional snippet will appear:

Important Note: Because this command takes 'raw' input, if you use any command options you must use
 ' -- ' between the end of the command options and the beginning of the raw input.

help raw-input says this:

<raw-input> -- Free-form text passed to a command without prior interpretation, allowing spaces
               without requiring quotes.  To pass arguments and free form text put two dashes ' -- '
               between the last argument and any raw input.

If we do want to change that, we should at least make sure all of these things reflect that. And yeah, that should be a separate patch (maybe a small RFC even).

FWIW, if we do want to change the handling of raw-commands, I don't think changing that for _all_ raw commands (including expr) would be such a bad thing. Users mostly interact with the expression command via the p alias, which already includes the -- thingy in its expansion. And for the advanced uses of these commands, we could still tell a consistent story (e.g. option processing stops at the first token which does not look like an option, and everything after that is considered to be the "raw" part).

[JDevlieghere]

Sounds reasonable to me. I'll split it up in two patches. I'll update this one to use the double dashes so we can land the functionality and keep things consistent with the rest of LLDB. I'll create another patch to do the handling of raw commands to use a heuristic.

[labath]

This looks fine (sorry about the delay, I've been OOO).

lldb/test/Shell/ScriptInterpreter/Lua/lua.test

2

Did you add the -- here just for consistency? It believe it should not be necessary to use the double dash if the script command has no arguments (similar to how its possible to write expr 1+2 without any dashes (but expr -A -- 1+2 does require it)

[JDevlieghere]

In D86996#2273445, @labath wrote:

This looks fine (sorry about the delay, I've been OOO).

I hope you had a good time. Glad to have you back! :-)

lldb/test/Shell/ScriptInterpreter/Lua/lua.test

2

I think I meant to have both but updated the line without copying it first. I'll fix that before landing.

[MaskRay]

llvm/lib/Support/MemoryBuffer.cpp

460 ↗

(On Diff #291960)

I guess this was unintended. Fixed in 03f1516d6075f42dce95bcf9fde3f6fde97abd35

lldb/source/Commands/CommandObjectScript.h

 #include "lldb/Interpreter/CommandObject.h"
 
 namespace lldb_private {
 
 class CommandObjectScript : public CommandObjectRaw {
 public:
   CommandObjectScript(CommandInterpreter &interpreter);
   ~CommandObjectScript() override;
+  Options *GetOptions() override { return &m_options; }
+
+  class CommandOptions : public Options {
+  public:
+    CommandOptions() : Options() {}
+    ~CommandOptions() override = default;
+    Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+                          ExecutionContext *execution_context) override;
+    void OptionParsingStarting(ExecutionContext *execution_context) override;
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
+    lldb::ScriptLanguage language = lldb::eScriptLanguageNone;
+  };
 
 protected:
   bool DoExecute(llvm::StringRef command, CommandReturnObject &result) override;
+
+private:
+  CommandOptions m_options;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_INTERPRETER_COMMANDOBJECTSCRIPT_H

lldb/source/Commands/CommandObjectScript.cpp

 //===-- CommandObjectScript.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CommandObjectScript.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/Host/Config.h"
+#include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/Utility/Args.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-// CommandObjectScript
+static constexpr OptionEnumValueElement g_script_option_enumeration[] = {
+    {
+        eScriptLanguagePython,
+        "python",
+        "Python",
+    },
+    {
+        eScriptLanguageLua,
+        "lua",
+        "Lua",
+    },
+    {
+        eScriptLanguageNone,
+        "default",
+        "The default scripting language.",
+    },
+};
+
+static constexpr OptionEnumValues ScriptOptionEnum() {
+  return OptionEnumValues(g_script_option_enumeration);
+}
+
+#define LLDB_OPTIONS_script
+#include "CommandOptions.inc"
+
+Status CommandObjectScript::CommandOptions::SetOptionValue(
+    uint32_t option_idx, llvm::StringRef option_arg,
+    ExecutionContext *execution_context) {
+  Status error;
+  const int short_option = m_getopt_table[option_idx].val;
+
+  switch (short_option) {
+  case 'l':
+    language = (lldb::ScriptLanguage)OptionArgParser::ToOptionEnum(
+        option_arg, GetDefinitions()[option_idx].enum_values,
+        eScriptLanguageNone, error);
+    if (!error.Success())
+      error.SetErrorStringWithFormat("unrecognized value for language '%s'",
+                                     option_arg.str().c_str());
+    break;
+  default:
+    llvm_unreachable("Unimplemented option");
+  }
+
+  return error;
+}
+
+void CommandObjectScript::CommandOptions::OptionParsingStarting(
+    ExecutionContext *execution_context) {
+  language = lldb::eScriptLanguageNone;
+}
+
+llvm::ArrayRef<OptionDefinition>
+CommandObjectScript::CommandOptions::GetDefinitions() {
+  return llvm::makeArrayRef(g_script_options);
+}
 
 CommandObjectScript::CommandObjectScript(CommandInterpreter &interpreter)
     : CommandObjectRaw(
           interpreter, "script",
           "Invoke the script interpreter with provided code and display any "
           "results.  Start the interactive interpreter if no code is supplied.",
-          "script [<script-code>]") {}
+          "script [--language <scripting-language> --] [<script-code>]") {}
 
 CommandObjectScript::~CommandObjectScript() {}
 
 bool CommandObjectScript::DoExecute(llvm::StringRef command,
                                     CommandReturnObject &result) {
-  if (m_interpreter.GetDebugger().GetScriptLanguage() ==
-      lldb::eScriptLanguageNone) {
+  // Try parsing the language option but when the command contains a raw part
+  // separated by the -- delimiter.
+  OptionsWithRaw raw_args(command);
+  if (raw_args.HasArgs()) {
+    if (!ParseOptions(raw_args.GetArgs(), result))
+      return false;
+    command = raw_args.GetRawPart();
+  }
+
+  lldb::ScriptLanguage language =
+      (m_options.language == lldb::eScriptLanguageNone)
+          ? m_interpreter.GetDebugger().GetScriptLanguage()
+          : m_options.language;
+
+  if (language == lldb::eScriptLanguageNone) {
     result.AppendError(
         "the script-lang setting is set to none - scripting not available");
     result.SetStatus(eReturnStatusFailed);
     return false;
   }
 
-  ScriptInterpreter *script_interpreter = GetDebugger().GetScriptInterpreter();
+  ScriptInterpreter *script_interpreter =
+      GetDebugger().GetScriptInterpreter(true, language);
 
   if (script_interpreter == nullptr) {
     result.AppendError("no script interpreter");
     result.SetStatus(eReturnStatusFailed);
     return false;
   }
 
   // Script might change Python code we use for formatting. Make sure we keep
   // up to date with it.
   DataVisualization::ForceUpdate();
 
   if (command.empty()) {
     script_interpreter->ExecuteInterpreterLoop();
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
     return result.Succeeded();
   }
 
   // We can do better when reporting the status of one-liner script execution.
   if (script_interpreter->ExecuteOneLine(command, &result))
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
   else
     result.SetStatus(eReturnStatusFailed);
 
   return result.Succeeded();
 }

lldb/source/Commands/Options.td

let Command = "script add" in {
   def script_add_help : Option<"help", "h">, Group<1>, Arg<"HelpText">,
   Desc<"The help text to display for this command.">;
   def script_add_synchronicity : Option<"synchronicity", "s">,
     EnumArg<"ScriptedCommandSynchronicity", "ScriptSynchroType()">,
     Desc<"Set the synchronicity of this command's executions with regard to "
     "LLDB event system.">;
 }
 
+let Command = "script" in {
+  def script_language : Option<"language", "l">,
+    EnumArg<"ScriptLang", "ScriptOptionEnum()">, Desc<"Specify the scripting "
+    " language. If none is specific the default scripting language is used.">;
+}
+
 let Command = "source info" in {
   def source_info_count : Option<"count", "c">, Arg<"Count">,
     Desc<"The number of line entries to display.">;
   def source_info_shlib : Option<"shlib", "s">, Groups<[1,2]>, Arg<"ShlibName">,
     Completion<"Module">, Desc<"Look up the source in the given module or "
     "shared library (can be specified more than once).">;
   def source_info_file : Option<"file", "f">, Group<1>, Arg<"Filename">,
     Completion<"SourceFile">, Desc<"The file from which to display source.">;

lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test

+# REQUIRES: lua
+# REQUIRES: python
+# UNSUPPORTED: lldb-repro
+
+# RUN: mkdir -p %t
+# RUN: cd %t
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o a.out
+# RUN: cat %s | %lldb 2>&1 | FileCheck %s
+script -l lua --
+target = lldb.debugger:CreateTarget("a.out")
+print("target is valid:", tostring(target:IsValid()))
+lldb.debugger:SetSelectedTarget(target)
+quit
+# CHECK: target is valid: true
+script -l python --
+print("selected target: {}".format(lldb.debugger.GetSelectedTarget()))
+# CHECK: selected target: a.out

lldb/test/Shell/ScriptInterpreter/Lua/lua.test

 # REQUIRES: lua
-# RUN: %lldb --script-language lua -o 'script print(1000+100+10+1)' 2>&1 | FileCheck %s
+# RUN: %lldb --script-language lua -o 'script -- io.stdout:write(1000+100+10+1, "\n")' 2>&1 | FileCheck %s
+# RUN: %lldb --script-language lua -o 'script --language default -- io.stdout:write(1000+100+10+1, "\n")' 2>&1 | FileCheck %s
+# RUN: %lldb -o 'script -l lua -- io.stdout:write(1000+100+10+1, "\n")' 2>&1 | FileCheck %s
+# RUN: %lldb -o 'script --language lua -- io.stdout:write(1000+100+10+1, "\n")' 2>&1 | FileCheck %s
 # CHECK: 1111

lldb/test/Shell/ScriptInterpreter/Python/python.test

+# REQUIRES: python
+# RUN: %lldb --script-language python -o 'script -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# RUN: %lldb --script-language python -o 'script --language default -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# RUN: %lldb -o 'script -l python -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# RUN: %lldb -o 'script -lpython -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# RUN: %lldb -o 'script --language python -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# RUN: %lldb -o 'script --language=python -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# CHECK: 1111
+
+# RUN: %lldb -o 'script --language invalid -- print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s --check-prefix INVALID
+# INVALID: error: unrecognized value for language 'invalid'
+# INVALID-NOT: 1111