[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: llvm/lib/Support/MemoryBuffer.cpp:460
 
-  if (shouldUseMmap(FD, FileSize, MapSize, Offset, RequiresNullTerminator,
-PageSize, IsVolatile)) {
+  if (false) {
 std::error_code EC;

I guess this was unintended. Fixed in 03f1516d6075f42dce95bcf9fde3f6fde97abd35


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG127faae7529a: [lldb] Add -l/--language option to script 
command (authored by JDevlieghere).
Herald added subscribers: llvm-commits, hiraditya.
Herald added projects: LLDB, LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D86996?vs=290596=291960#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86996

Files:
  lldb/source/Commands/CommandObjectScript.cpp
  lldb/source/Commands/CommandObjectScript.h
  lldb/source/Commands/Options.td
  lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
  lldb/test/Shell/ScriptInterpreter/Lua/lua.test
  lldb/test/Shell/ScriptInterpreter/Python/python.test
  llvm/lib/Support/MemoryBuffer.cpp

Index: llvm/lib/Support/MemoryBuffer.cpp
===
--- llvm/lib/Support/MemoryBuffer.cpp
+++ llvm/lib/Support/MemoryBuffer.cpp
@@ -457,8 +457,7 @@
 MapSize = FileSize;
   }
 
-  if (shouldUseMmap(FD, FileSize, MapSize, Offset, RequiresNullTerminator,
-PageSize, IsVolatile)) {
+  if (false) {
 std::error_code EC;
 std::unique_ptr Result(
 new (NamedBufferAlloc(Filename)) MemoryBufferMMapFile(
Index: lldb/test/Shell/ScriptInterpreter/Python/python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/python.test
@@ -0,0 +1,13 @@
+# 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 -- 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: 
+
+# 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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua.test
===
--- lldb/test/Shell/ScriptInterpreter/Lua/lua.test
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua.test
@@ -1,3 +1,7 @@
 # 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 -- 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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
@@ -0,0 +1,17 @@
+# 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
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -717,6 +717,12 @@
 "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.">;
Index: lldb/source/Commands/CommandObjectScript.h
===
--- lldb/source/Commands/CommandObjectScript.h
+++ lldb/source/Commands/CommandObjectScript.h
@@ 

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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! :-)




Comment at: lldb/test/Shell/ScriptInterpreter/Lua/lua.test:2
 # 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

labath wrote:
> 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)
I think I meant to have both but updated the line without copying it first. 
I'll fix that before landing. 


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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




Comment at: lldb/test/Shell/ScriptInterpreter/Lua/lua.test:2
 # 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

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)


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 290596.

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

https://reviews.llvm.org/D86996

Files:
  lldb/source/Commands/CommandObjectScript.cpp
  lldb/source/Commands/CommandObjectScript.h
  lldb/source/Commands/Options.td
  lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
  lldb/test/Shell/ScriptInterpreter/Lua/lua.test
  lldb/test/Shell/ScriptInterpreter/Python/python.test

Index: lldb/test/Shell/ScriptInterpreter/Python/python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/python.test
@@ -0,0 +1,12 @@
+# 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: 
+
+# 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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua.test
===
--- lldb/test/Shell/ScriptInterpreter/Lua/lua.test
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua.test
@@ -1,3 +1,6 @@
 # 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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
@@ -0,0 +1,17 @@
+# 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
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -717,6 +717,12 @@
 "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.">;
Index: lldb/source/Commands/CommandObjectScript.h
===
--- lldb/source/Commands/CommandObjectScript.h
+++ lldb/source/Commands/CommandObjectScript.h
@@ -17,9 +17,24 @@
 public:
   CommandObjectScript(CommandInterpreter );
   ~CommandObjectScript() override;
+  Options *GetOptions() override { return _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 GetDefinitions() override;
+lldb::ScriptLanguage language = lldb::eScriptLanguageNone;
+  };
 
 protected:
   bool DoExecute(llvm::StringRef command, CommandReturnObject ) override;
+
+private:
+  CommandOptions m_options;
 };
 
 } // namespace lldb_private
Index: lldb/source/Commands/CommandObjectScript.cpp
===
--- lldb/source/Commands/CommandObjectScript.cpp
+++ lldb/source/Commands/CommandObjectScript.cpp
@@ -10,36 +10,107 @@
 #include "lldb/Core/Debugger.h"
 

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

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.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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:

   -- 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).


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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




Comment at: lldb/source/Commands/CommandObjectScript.cpp:99
+  } else {
+// When no code is provided We want to be able to launch the interactive
+// script interpreter without having to require the -- after the language.

`We` -> `we`.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

> 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  
` 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.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

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.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.



Comment at: lldb/source/Commands/CommandObjectScript.cpp:37
+"default",
+"The default scripting language.",
+},

kastiglione wrote:
> 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.
It's set dynamically as a debugger property, while the options here are static. 


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Commands/CommandObjectScript.cpp:37
+"default",
+"The default scripting language.",
+},

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.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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`.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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: :1:3: use of undeclared identifier 'show'
  --show-types
^
  error: :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).


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 289369.
JDevlieghere marked 2 inline comments as done.
JDevlieghere added a comment.
Herald added a subscriber: dang.

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


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

https://reviews.llvm.org/D86996

Files:
  lldb/source/Commands/CommandObjectScript.cpp
  lldb/source/Commands/CommandObjectScript.h
  lldb/source/Commands/Options.td
  lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
  lldb/test/Shell/ScriptInterpreter/Lua/lua.test
  lldb/test/Shell/ScriptInterpreter/Python/python.test

Index: lldb/test/Shell/ScriptInterpreter/Python/python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/python.test
@@ -0,0 +1,12 @@
+# 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: 
+
+# 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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua.test
===
--- lldb/test/Shell/ScriptInterpreter/Lua/lua.test
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua.test
@@ -1,3 +1,6 @@
 # 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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
@@ -0,0 +1,17 @@
+# 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
Index: lldb/source/Commands/Options.td
===
--- lldb/source/Commands/Options.td
+++ lldb/source/Commands/Options.td
@@ -709,6 +709,12 @@
 "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.">;
Index: lldb/source/Commands/CommandObjectScript.h
===
--- lldb/source/Commands/CommandObjectScript.h
+++ lldb/source/Commands/CommandObjectScript.h
@@ -17,9 +17,24 @@
 public:
   CommandObjectScript(CommandInterpreter );
   ~CommandObjectScript() override;
+  Options *GetOptions() override { return _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 GetDefinitions() override;
+lldb::ScriptLanguage language = lldb::eScriptLanguageNone;
+  };
 
 protected:
   bool DoExecute(llvm::StringRef command, CommandReturnObject ) override;
+
+private:
+  CommandOptions m_options;
 };
 
 } // namespace lldb_private
Index: 

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.

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).


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere planned changes to this revision.
JDevlieghere marked 3 inline comments as done.
JDevlieghere added inline comments.



Comment at: lldb/source/Commands/CommandObjectScript.cpp:50-51
+
+  std::tie(head, tail) = tail.split(' ');
+  if (head != "-l" && head != "--language")
+return CommandParsed(command);

kastiglione wrote:
> 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?
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. 



Comment at: lldb/source/Commands/CommandObjectScript.cpp:65
+
+  return CommandParsed(command);
+}

kastiglione wrote:
> this is missing error handling for unsupported languages, such as `script -l 
> swift`
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. 



Comment at: lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test:7
+# RUN: cd %t
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o a.out
+# RUN: cat %s | %lldb 2>&1 | FileCheck %s

kastiglione wrote:
> is `a.out` not a universal default?
Not sure about Windows, but regardless I prefer to be explicit. 


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Commands/CommandObjectScript.cpp:50-51
+
+  std::tie(head, tail) = tail.split(' ');
+  if (head != "-l" && head != "--language")
+return CommandParsed(command);

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?


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/source/Commands/CommandObjectScript.cpp:65
+
+  return CommandParsed(command);
+}

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



Comment at: lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test:7
+# RUN: cd %t
+# RUN: echo "int main() { return 0; }" | %clang_host -x c - -o a.out
+# RUN: cat %s | %lldb 2>&1 | FileCheck %s

is `a.out` not a universal default?



Comment at: lldb/test/Shell/ScriptInterpreter/Python/python.test:8
+# RUN: %lldb -o 'script --language invalid print("{}".format(1000+100+10+1))' 
2>&1 | FileCheck %s --check-prefix INVALID
+# INVALID: SyntaxError
+# INVALID-NOT: 

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?


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

https://reviews.llvm.org/D86996

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86996: [lldb] Add -l/--language option to script command

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, mib, l.frisken, jingham.
JDevlieghere requested review of this revision.

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.


https://reviews.llvm.org/D86996

Files:
  lldb/source/Commands/CommandObjectScript.cpp
  lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
  lldb/test/Shell/ScriptInterpreter/Lua/lua.test
  lldb/test/Shell/ScriptInterpreter/None/none.test
  lldb/test/Shell/ScriptInterpreter/Python/python.test

Index: lldb/test/Shell/ScriptInterpreter/Python/python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Python/python.test
@@ -0,0 +1,9 @@
+# REQUIRES: python
+# RUN: %lldb --script-language python -o 'script 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 --language python print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s
+# CHECK: 
+
+# RUN: %lldb -o 'script --language invalid print("{}".format(1000+100+10+1))' 2>&1 | FileCheck %s --check-prefix INVALID
+# INVALID: SyntaxError
+# INVALID-NOT: 
Index: lldb/test/Shell/ScriptInterpreter/None/none.test
===
--- lldb/test/Shell/ScriptInterpreter/None/none.test
+++ lldb/test/Shell/ScriptInterpreter/None/none.test
@@ -1,2 +1,5 @@
 # RUN: %lldb --script-language none -o 'script' 2>&1 | FileCheck %s
 # CHECK: error: the script-lang setting is set to none - scripting not available
+
+# RUN: %lldb -o 'script -l none foo' 2>&1 | FileCheck %s --check-prefix ERROR
+# ERROR: error: the script-lang setting is set to none - scripting not available
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua.test
===
--- lldb/test/Shell/ScriptInterpreter/Lua/lua.test
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua.test
@@ -1,3 +1,5 @@
 # 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 -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: 
Index: lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
===
--- /dev/null
+++ lldb/test/Shell/ScriptInterpreter/Lua/lua-python.test
@@ -0,0 +1,17 @@
+# 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
Index: lldb/source/Commands/CommandObjectScript.cpp
===
--- lldb/source/Commands/CommandObjectScript.cpp
+++ lldb/source/Commands/CommandObjectScript.cpp
@@ -25,21 +25,63 @@
   interpreter, "script",
   "Invoke the script interpreter with provided code and display any "
   "results.  Start the interactive interpreter if no code is supplied.",
-  "script []") {}
+  "script [-l ] []") {}
 
 CommandObjectScript::~CommandObjectScript() {}
 
+struct CommandParsed {
+  CommandParsed(llvm::StringRef command)
+  : language(llvm::None), command(command) {}
+  CommandParsed(llvm::Optional language,
+llvm::StringRef command)
+  : language(language), command(command) {}
+  llvm::Optional language;
+  llvm::StringRef command;
+};
+
+/// Manually parse the -l  or --language 
+/// argument from the command. This is necessary because CommandObjectScript is
+/// an instance of CommandObjectRaw. We cannot use