[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Apparently closing a revision requires it to be accepted first.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.
Herald added a project: All.

Just for history's sake, this change was committed (738af7a6241c9).  I put in 
the Differential Revision line in the changelog, so I'm not sure why this 
didn't get marked as closed...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py:21
 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
-def test_breakpoint_command_sequence(self):
+def not_test_breakpoint_command_sequence(self):
 """Test a sequence of breakpoint command add, list, and delete."""

jingham wrote:
> wallace wrote:
> > I've been trying to track a regression in source maps between Android 
> > Studio versions and it seems that this test shouldn't have been disabled. :(
> Yup, my bad.  I sometimes disable tests this way when I'm only trying to run 
> one test in the set a bunch of times.  I must have forgotten to set it back.  
> Thanks for catching this!
No worries!! It was fun, though


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py:21
 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
-def test_breakpoint_command_sequence(self):
+def not_test_breakpoint_command_sequence(self):
 """Test a sequence of breakpoint command add, list, and delete."""

wallace wrote:
> I've been trying to track a regression in source maps between Android Studio 
> versions and it seems that this test shouldn't have been disabled. :(
Yup, my bad.  I sometimes disable tests this way when I'm only trying to run 
one test in the set a bunch of times.  I must have forgotten to set it back.  
Thanks for catching this!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.
Herald added a subscriber: dang.



Comment at: 
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py:21
 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
-def test_breakpoint_command_sequence(self):
+def not_test_breakpoint_command_sequence(self):
 """Test a sequence of breakpoint command add, list, and delete."""

I've been trying to track a regression in source maps between Android Studio 
versions and it seems that this test shouldn't have been disabled. :(


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

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

Re: [Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via lldb-commits]

Ack, sorry.  Thanks for fixing it.

Jim


> On Oct 25, 2019, at 3:52 PM, Adrian McCarthy via Phabricator 
>  wrote:
> 
> amccarth added a comment.
> 
> FYI:  This broke the build for me.
> 
> 
> 
> 
> Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:323
> +  const char *function_name,
> +  StructuredData::ObjectSP extra_args_sp) {}
> 
> 
> This breaks some builds because it doesn't return a value.
> 
> 
> Repository:
>  rLLDB LLDB
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D68671/new/
> 
> https://reviews.llvm.org/D68671
> 
> 
> 

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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

FYI:  This broke the build for me.




Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:323
+  const char *function_name,
+  StructuredData::ObjectSP extra_args_sp) {}
 

This breaks some builds because it doesn't return a value.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

labath wrote:
> jingham wrote:
> > labath wrote:
> > > jingham wrote:
> > > > jingham wrote:
> > > > > labath wrote:
> > > > > > labath wrote:
> > > > > > > lawrence_danna wrote:
> > > > > > > > jingham wrote:
> > > > > > > > > lawrence_danna wrote:
> > > > > > > > > > labath wrote:
> > > > > > > > > > > jingham wrote:
> > > > > > > > > > > > lawrence_danna wrote:
> > > > > > > > > > > > > labath wrote:
> > > > > > > > > > > > > > In light of varargs functions (`*args, **kwargs`), 
> > > > > > > > > > > > > > which are fairly popular in python, the concept of 
> > > > > > > > > > > > > > "number of arguments of a callable" does not seem 
> > > > > > > > > > > > > > that well defined. The current implementation seems 
> > > > > > > > > > > > > > to return the number of fixed arguments, which 
> > > > > > > > > > > > > > might be fine, but I think this behavior should be 
> > > > > > > > > > > > > > documented. Also, it would be good to modernize 
> > > > > > > > > > > > > > this function signature -- have it take a 
> > > > > > > > > > > > > > StringRef, and return a `Expected` -- 
> > > > > > > > > > > > > > ongoing work by @lawrence_danna will make it 
> > > > > > > > > > > > > > possible to return errors from the python 
> > > > > > > > > > > > > > interpreter, and this will make it possible to 
> > > > > > > > > > > > > > display those, instead of just guessing that this 
> > > > > > > > > > > > > > is because the callable was not found (it could in 
> > > > > > > > > > > > > > fact be because the named thing is not a callable, 
> > > > > > > > > > > > > > of because resolving the name produced an 
> > > > > > > > > > > > > > exception, ...).
> > > > > > > > > > > > > I just took a look at 
> > > > > > > > > > > > > PythonCallable::GetNumArguments() and it's horribly 
> > > > > > > > > > > > > broken.  
> > > > > > > > > > > > > 
> > > > > > > > > > > > > It doesn't even work for the simplest test case I 
> > > > > > > > > > > > > could think of.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ```  
> > > > > > > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > > > > > > auto hex = 
> > > > > > > > > > > > > As(builtins.get().GetAttribute("hex"));
> > > > > > > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > 
> > > > > > > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > > > > > > Interesting.  We use it for free functions (what you 
> > > > > > > > > > > > pass to the -F option of `breakpoint command add`) and 
> > > > > > > > > > > > for the __init__ and __call__ method in the little 
> > > > > > > > > > > > classes you can make up for scripted thread plans and 
> > > > > > > > > > > > for the class version of Python implemented 
> > > > > > > > > > > > command-line commands.  We have tests for telling 3 
> > > > > > > > > > > > vrs. 4 vrs. not enough or too many, and they all pass.  
> > > > > > > > > > > > So it seems to work in the cases we currently need it 
> > > > > > > > > > > > to work for...
> > > > > > > > > > > > 
> > > > > > > > > > > > "inspect.signature" is python 3 only, and the python 2 
> > > > > > > > > > > > equivalent is deprecated.  So it will take a little 
> > > > > > > > > > > > fancy footwork to use it in the transition period.
> > > > > > > > > > > lol :)
> > > > > > > > > > > 
> > > > > > > > > > > I would actually say that we should try not to use this 
> > > > > > > > > > > function(ality) wherever possible. Making decisions based 
> > > > > > > > > > > on the number of arguments the thing you're about to call 
> > > > > > > > > > > takes sounds weird. I don't want to get too involved in 
> > > > > > > > > > > this, but I was designing this, I'd just say that if one 
> > > > > > > > > > > tries to pass arguments to the callback then the callback 
> > > > > > > > > > > MUST take three arguments (or we'll abort processing the 
> > > > > > > > > > > breakpoint command). If he wants his function to be 
> > > > > > > > > > > called both with arguments and without, he can just add a 
> > > > > > > > > > > default value to the third argument. (And if his function 
> > > > > > > > > > > takes two arguments, but he still tries to pass 
> > > > > > > > > > > something... well, it's his own fault).
> > > > > > > > > > > 
> > > > > > > > > > > Anyway, feel free to ignore this comment, but I felt like 
> > > > > > > > > > > I had to say something. :)
> > > > > > > > > > I completely agree with Pavel.Inspecting a function 
> > > > > > > > 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

jingham wrote:
> labath wrote:
> > jingham wrote:
> > > jingham wrote:
> > > > labath wrote:
> > > > > labath wrote:
> > > > > > lawrence_danna wrote:
> > > > > > > jingham wrote:
> > > > > > > > lawrence_danna wrote:
> > > > > > > > > labath wrote:
> > > > > > > > > > jingham wrote:
> > > > > > > > > > > lawrence_danna wrote:
> > > > > > > > > > > > labath wrote:
> > > > > > > > > > > > > In light of varargs functions (`*args, **kwargs`), 
> > > > > > > > > > > > > which are fairly popular in python, the concept of 
> > > > > > > > > > > > > "number of arguments of a callable" does not seem 
> > > > > > > > > > > > > that well defined. The current implementation seems 
> > > > > > > > > > > > > to return the number of fixed arguments, which might 
> > > > > > > > > > > > > be fine, but I think this behavior should be 
> > > > > > > > > > > > > documented. Also, it would be good to modernize this 
> > > > > > > > > > > > > function signature -- have it take a StringRef, and 
> > > > > > > > > > > > > return a `Expected` -- ongoing work by 
> > > > > > > > > > > > > @lawrence_danna will make it possible to return 
> > > > > > > > > > > > > errors from the python interpreter, and this will 
> > > > > > > > > > > > > make it possible to display those, instead of just 
> > > > > > > > > > > > > guessing that this is because the callable was not 
> > > > > > > > > > > > > found (it could in fact be because the named thing is 
> > > > > > > > > > > > > not a callable, of because resolving the name 
> > > > > > > > > > > > > produced an exception, ...).
> > > > > > > > > > > > I just took a look at PythonCallable::GetNumArguments() 
> > > > > > > > > > > > and it's horribly broken.  
> > > > > > > > > > > > 
> > > > > > > > > > > > It doesn't even work for the simplest test case I could 
> > > > > > > > > > > > think of.
> > > > > > > > > > > > 
> > > > > > > > > > > > ```  
> > > > > > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > > > > > auto hex = 
> > > > > > > > > > > > As(builtins.get().GetAttribute("hex"));
> > > > > > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > > > > > ```
> > > > > > > > > > > > 
> > > > > > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > > > > > Interesting.  We use it for free functions (what you pass 
> > > > > > > > > > > to the -F option of `breakpoint command add`) and for the 
> > > > > > > > > > > __init__ and __call__ method in the little classes you 
> > > > > > > > > > > can make up for scripted thread plans and for the class 
> > > > > > > > > > > version of Python implemented command-line commands.  We 
> > > > > > > > > > > have tests for telling 3 vrs. 4 vrs. not enough or too 
> > > > > > > > > > > many, and they all pass.  So it seems to work in the 
> > > > > > > > > > > cases we currently need it to work for...
> > > > > > > > > > > 
> > > > > > > > > > > "inspect.signature" is python 3 only, and the python 2 
> > > > > > > > > > > equivalent is deprecated.  So it will take a little fancy 
> > > > > > > > > > > footwork to use it in the transition period.
> > > > > > > > > > lol :)
> > > > > > > > > > 
> > > > > > > > > > I would actually say that we should try not to use this 
> > > > > > > > > > function(ality) wherever possible. Making decisions based 
> > > > > > > > > > on the number of arguments the thing you're about to call 
> > > > > > > > > > takes sounds weird. I don't want to get too involved in 
> > > > > > > > > > this, but I was designing this, I'd just say that if one 
> > > > > > > > > > tries to pass arguments to the callback then the callback 
> > > > > > > > > > MUST take three arguments (or we'll abort processing the 
> > > > > > > > > > breakpoint command). If he wants his function to be called 
> > > > > > > > > > both with arguments and without, he can just add a default 
> > > > > > > > > > value to the third argument. (And if his function takes two 
> > > > > > > > > > arguments, but he still tries to pass something... well, 
> > > > > > > > > > it's his own fault).
> > > > > > > > > > 
> > > > > > > > > > Anyway, feel free to ignore this comment, but I felt like I 
> > > > > > > > > > had to say something. :)
> > > > > > > > > I completely agree with Pavel.Inspecting a function 
> > > > > > > > > signature before calling it is a big code smell in python.
> > > > > > > > > If there's a way to avoid doing that introspection, that 
> > > > > > > > > would be better.
> > > > > > > > Unfortunately, we originally designed this interface to take 
> > > > > > > > three arguments, 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

labath wrote:
> jingham wrote:
> > jingham wrote:
> > > labath wrote:
> > > > labath wrote:
> > > > > lawrence_danna wrote:
> > > > > > jingham wrote:
> > > > > > > lawrence_danna wrote:
> > > > > > > > labath wrote:
> > > > > > > > > jingham wrote:
> > > > > > > > > > lawrence_danna wrote:
> > > > > > > > > > > labath wrote:
> > > > > > > > > > > > In light of varargs functions (`*args, **kwargs`), 
> > > > > > > > > > > > which are fairly popular in python, the concept of 
> > > > > > > > > > > > "number of arguments of a callable" does not seem that 
> > > > > > > > > > > > well defined. The current implementation seems to 
> > > > > > > > > > > > return the number of fixed arguments, which might be 
> > > > > > > > > > > > fine, but I think this behavior should be documented. 
> > > > > > > > > > > > Also, it would be good to modernize this function 
> > > > > > > > > > > > signature -- have it take a StringRef, and return a 
> > > > > > > > > > > > `Expected` -- ongoing work by 
> > > > > > > > > > > > @lawrence_danna will make it possible to return errors 
> > > > > > > > > > > > from the python interpreter, and this will make it 
> > > > > > > > > > > > possible to display those, instead of just guessing 
> > > > > > > > > > > > that this is because the callable was not found (it 
> > > > > > > > > > > > could in fact be because the named thing is not a 
> > > > > > > > > > > > callable, of because resolving the name produced an 
> > > > > > > > > > > > exception, ...).
> > > > > > > > > > > I just took a look at PythonCallable::GetNumArguments() 
> > > > > > > > > > > and it's horribly broken.  
> > > > > > > > > > > 
> > > > > > > > > > > It doesn't even work for the simplest test case I could 
> > > > > > > > > > > think of.
> > > > > > > > > > > 
> > > > > > > > > > > ```  
> > > > > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > > > > auto hex = 
> > > > > > > > > > > As(builtins.get().GetAttribute("hex"));
> > > > > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > > > > ```
> > > > > > > > > > > 
> > > > > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > > > > Interesting.  We use it for free functions (what you pass 
> > > > > > > > > > to the -F option of `breakpoint command add`) and for the 
> > > > > > > > > > __init__ and __call__ method in the little classes you can 
> > > > > > > > > > make up for scripted thread plans and for the class version 
> > > > > > > > > > of Python implemented command-line commands.  We have tests 
> > > > > > > > > > for telling 3 vrs. 4 vrs. not enough or too many, and they 
> > > > > > > > > > all pass.  So it seems to work in the cases we currently 
> > > > > > > > > > need it to work for...
> > > > > > > > > > 
> > > > > > > > > > "inspect.signature" is python 3 only, and the python 2 
> > > > > > > > > > equivalent is deprecated.  So it will take a little fancy 
> > > > > > > > > > footwork to use it in the transition period.
> > > > > > > > > lol :)
> > > > > > > > > 
> > > > > > > > > I would actually say that we should try not to use this 
> > > > > > > > > function(ality) wherever possible. Making decisions based on 
> > > > > > > > > the number of arguments the thing you're about to call takes 
> > > > > > > > > sounds weird. I don't want to get too involved in this, but I 
> > > > > > > > > was designing this, I'd just say that if one tries to pass 
> > > > > > > > > arguments to the callback then the callback MUST take three 
> > > > > > > > > arguments (or we'll abort processing the breakpoint command). 
> > > > > > > > > If he wants his function to be called both with arguments and 
> > > > > > > > > without, he can just add a default value to the third 
> > > > > > > > > argument. (And if his function takes two arguments, but he 
> > > > > > > > > still tries to pass something... well, it's his own fault).
> > > > > > > > > 
> > > > > > > > > Anyway, feel free to ignore this comment, but I felt like I 
> > > > > > > > > had to say something. :)
> > > > > > > > I completely agree with Pavel.Inspecting a function 
> > > > > > > > signature before calling it is a big code smell in python.
> > > > > > > > If there's a way to avoid doing that introspection, that would 
> > > > > > > > be better.
> > > > > > > Unfortunately, we originally designed this interface to take 
> > > > > > > three arguments, the frame pointer, the breakpoint location and 
> > > > > > > the Python session dict.  Then it became clear that it would be 
> > > > > > > better 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

jingham wrote:
> jingham wrote:
> > labath wrote:
> > > labath wrote:
> > > > lawrence_danna wrote:
> > > > > jingham wrote:
> > > > > > lawrence_danna wrote:
> > > > > > > labath wrote:
> > > > > > > > jingham wrote:
> > > > > > > > > lawrence_danna wrote:
> > > > > > > > > > labath wrote:
> > > > > > > > > > > In light of varargs functions (`*args, **kwargs`), which 
> > > > > > > > > > > are fairly popular in python, the concept of "number of 
> > > > > > > > > > > arguments of a callable" does not seem that well defined. 
> > > > > > > > > > > The current implementation seems to return the number of 
> > > > > > > > > > > fixed arguments, which might be fine, but I think this 
> > > > > > > > > > > behavior should be documented. Also, it would be good to 
> > > > > > > > > > > modernize this function signature -- have it take a 
> > > > > > > > > > > StringRef, and return a `Expected` -- 
> > > > > > > > > > > ongoing work by @lawrence_danna will make it possible to 
> > > > > > > > > > > return errors from the python interpreter, and this will 
> > > > > > > > > > > make it possible to display those, instead of just 
> > > > > > > > > > > guessing that this is because the callable was not found 
> > > > > > > > > > > (it could in fact be because the named thing is not a 
> > > > > > > > > > > callable, of because resolving the name produced an 
> > > > > > > > > > > exception, ...).
> > > > > > > > > > I just took a look at PythonCallable::GetNumArguments() and 
> > > > > > > > > > it's horribly broken.  
> > > > > > > > > > 
> > > > > > > > > > It doesn't even work for the simplest test case I could 
> > > > > > > > > > think of.
> > > > > > > > > > 
> > > > > > > > > > ```  
> > > > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > > > auto hex = 
> > > > > > > > > > As(builtins.get().GetAttribute("hex"));
> > > > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > > > ```
> > > > > > > > > > 
> > > > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > > > Interesting.  We use it for free functions (what you pass to 
> > > > > > > > > the -F option of `breakpoint command add`) and for the 
> > > > > > > > > __init__ and __call__ method in the little classes you can 
> > > > > > > > > make up for scripted thread plans and for the class version 
> > > > > > > > > of Python implemented command-line commands.  We have tests 
> > > > > > > > > for telling 3 vrs. 4 vrs. not enough or too many, and they 
> > > > > > > > > all pass.  So it seems to work in the cases we currently need 
> > > > > > > > > it to work for...
> > > > > > > > > 
> > > > > > > > > "inspect.signature" is python 3 only, and the python 2 
> > > > > > > > > equivalent is deprecated.  So it will take a little fancy 
> > > > > > > > > footwork to use it in the transition period.
> > > > > > > > lol :)
> > > > > > > > 
> > > > > > > > I would actually say that we should try not to use this 
> > > > > > > > function(ality) wherever possible. Making decisions based on 
> > > > > > > > the number of arguments the thing you're about to call takes 
> > > > > > > > sounds weird. I don't want to get too involved in this, but I 
> > > > > > > > was designing this, I'd just say that if one tries to pass 
> > > > > > > > arguments to the callback then the callback MUST take three 
> > > > > > > > arguments (or we'll abort processing the breakpoint command). 
> > > > > > > > If he wants his function to be called both with arguments and 
> > > > > > > > without, he can just add a default value to the third argument. 
> > > > > > > > (And if his function takes two arguments, but he still tries to 
> > > > > > > > pass something... well, it's his own fault).
> > > > > > > > 
> > > > > > > > Anyway, feel free to ignore this comment, but I felt like I had 
> > > > > > > > to say something. :)
> > > > > > > I completely agree with Pavel.Inspecting a function signature 
> > > > > > > before calling it is a big code smell in python.If there's a 
> > > > > > > way to avoid doing that introspection, that would be better.
> > > > > > Unfortunately, we originally designed this interface to take three 
> > > > > > arguments, the frame pointer, the breakpoint location and the 
> > > > > > Python session dict.  Then it became clear that it would be better 
> > > > > > to add this extra args argument (and in the case of Python based 
> > > > > > commands the ExecutionContext pointer).  At that point we had three 
> > > > > > choices, abandon the improvement; switch to unconditionally passing 
> > > > > > the extra 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

jingham wrote:
> labath wrote:
> > labath wrote:
> > > lawrence_danna wrote:
> > > > jingham wrote:
> > > > > lawrence_danna wrote:
> > > > > > labath wrote:
> > > > > > > jingham wrote:
> > > > > > > > lawrence_danna wrote:
> > > > > > > > > labath wrote:
> > > > > > > > > > In light of varargs functions (`*args, **kwargs`), which 
> > > > > > > > > > are fairly popular in python, the concept of "number of 
> > > > > > > > > > arguments of a callable" does not seem that well defined. 
> > > > > > > > > > The current implementation seems to return the number of 
> > > > > > > > > > fixed arguments, which might be fine, but I think this 
> > > > > > > > > > behavior should be documented. Also, it would be good to 
> > > > > > > > > > modernize this function signature -- have it take a 
> > > > > > > > > > StringRef, and return a `Expected` -- ongoing 
> > > > > > > > > > work by @lawrence_danna will make it possible to return 
> > > > > > > > > > errors from the python interpreter, and this will make it 
> > > > > > > > > > possible to display those, instead of just guessing that 
> > > > > > > > > > this is because the callable was not found (it could in 
> > > > > > > > > > fact be because the named thing is not a callable, of 
> > > > > > > > > > because resolving the name produced an exception, ...).
> > > > > > > > > I just took a look at PythonCallable::GetNumArguments() and 
> > > > > > > > > it's horribly broken.  
> > > > > > > > > 
> > > > > > > > > It doesn't even work for the simplest test case I could think 
> > > > > > > > > of.
> > > > > > > > > 
> > > > > > > > > ```  
> > > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > > auto hex = 
> > > > > > > > > As(builtins.get().GetAttribute("hex"));
> > > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > > Interesting.  We use it for free functions (what you pass to 
> > > > > > > > the -F option of `breakpoint command add`) and for the __init__ 
> > > > > > > > and __call__ method in the little classes you can make up for 
> > > > > > > > scripted thread plans and for the class version of Python 
> > > > > > > > implemented command-line commands.  We have tests for telling 3 
> > > > > > > > vrs. 4 vrs. not enough or too many, and they all pass.  So it 
> > > > > > > > seems to work in the cases we currently need it to work for...
> > > > > > > > 
> > > > > > > > "inspect.signature" is python 3 only, and the python 2 
> > > > > > > > equivalent is deprecated.  So it will take a little fancy 
> > > > > > > > footwork to use it in the transition period.
> > > > > > > lol :)
> > > > > > > 
> > > > > > > I would actually say that we should try not to use this 
> > > > > > > function(ality) wherever possible. Making decisions based on the 
> > > > > > > number of arguments the thing you're about to call takes sounds 
> > > > > > > weird. I don't want to get too involved in this, but I was 
> > > > > > > designing this, I'd just say that if one tries to pass arguments 
> > > > > > > to the callback then the callback MUST take three arguments (or 
> > > > > > > we'll abort processing the breakpoint command). If he wants his 
> > > > > > > function to be called both with arguments and without, he can 
> > > > > > > just add a default value to the third argument. (And if his 
> > > > > > > function takes two arguments, but he still tries to pass 
> > > > > > > something... well, it's his own fault).
> > > > > > > 
> > > > > > > Anyway, feel free to ignore this comment, but I felt like I had 
> > > > > > > to say something. :)
> > > > > > I completely agree with Pavel.Inspecting a function signature 
> > > > > > before calling it is a big code smell in python.If there's a 
> > > > > > way to avoid doing that introspection, that would be better.
> > > > > Unfortunately, we originally designed this interface to take three 
> > > > > arguments, the frame pointer, the breakpoint location and the Python 
> > > > > session dict.  Then it became clear that it would be better to add 
> > > > > this extra args argument (and in the case of Python based commands 
> > > > > the ExecutionContext pointer).  At that point we had three choices, 
> > > > > abandon the improvement; switch to unconditionally passing the extra 
> > > > > arguments, and break everybody's extant uses; or switch off of the 
> > > > > number of arguments to decide whether the user had provided the old 
> > > > > or new forms.
> 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

labath wrote:
> labath wrote:
> > lawrence_danna wrote:
> > > jingham wrote:
> > > > lawrence_danna wrote:
> > > > > labath wrote:
> > > > > > jingham wrote:
> > > > > > > lawrence_danna wrote:
> > > > > > > > labath wrote:
> > > > > > > > > In light of varargs functions (`*args, **kwargs`), which are 
> > > > > > > > > fairly popular in python, the concept of "number of arguments 
> > > > > > > > > of a callable" does not seem that well defined. The current 
> > > > > > > > > implementation seems to return the number of fixed arguments, 
> > > > > > > > > which might be fine, but I think this behavior should be 
> > > > > > > > > documented. Also, it would be good to modernize this function 
> > > > > > > > > signature -- have it take a StringRef, and return a 
> > > > > > > > > `Expected` -- ongoing work by @lawrence_danna 
> > > > > > > > > will make it possible to return errors from the python 
> > > > > > > > > interpreter, and this will make it possible to display those, 
> > > > > > > > > instead of just guessing that this is because the callable 
> > > > > > > > > was not found (it could in fact be because the named thing is 
> > > > > > > > > not a callable, of because resolving the name produced an 
> > > > > > > > > exception, ...).
> > > > > > > > I just took a look at PythonCallable::GetNumArguments() and 
> > > > > > > > it's horribly broken.  
> > > > > > > > 
> > > > > > > > It doesn't even work for the simplest test case I could think 
> > > > > > > > of.
> > > > > > > > 
> > > > > > > > ```  
> > > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > > auto hex = 
> > > > > > > > As(builtins.get().GetAttribute("hex"));
> > > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > we should really re-write it to use inspect.signature.
> > > > > > > Interesting.  We use it for free functions (what you pass to the 
> > > > > > > -F option of `breakpoint command add`) and for the __init__ and 
> > > > > > > __call__ method in the little classes you can make up for 
> > > > > > > scripted thread plans and for the class version of Python 
> > > > > > > implemented command-line commands.  We have tests for telling 3 
> > > > > > > vrs. 4 vrs. not enough or too many, and they all pass.  So it 
> > > > > > > seems to work in the cases we currently need it to work for...
> > > > > > > 
> > > > > > > "inspect.signature" is python 3 only, and the python 2 equivalent 
> > > > > > > is deprecated.  So it will take a little fancy footwork to use it 
> > > > > > > in the transition period.
> > > > > > lol :)
> > > > > > 
> > > > > > I would actually say that we should try not to use this 
> > > > > > function(ality) wherever possible. Making decisions based on the 
> > > > > > number of arguments the thing you're about to call takes sounds 
> > > > > > weird. I don't want to get too involved in this, but I was 
> > > > > > designing this, I'd just say that if one tries to pass arguments to 
> > > > > > the callback then the callback MUST take three arguments (or we'll 
> > > > > > abort processing the breakpoint command). If he wants his function 
> > > > > > to be called both with arguments and without, he can just add a 
> > > > > > default value to the third argument. (And if his function takes two 
> > > > > > arguments, but he still tries to pass something... well, it's his 
> > > > > > own fault).
> > > > > > 
> > > > > > Anyway, feel free to ignore this comment, but I felt like I had to 
> > > > > > say something. :)
> > > > > I completely agree with Pavel.Inspecting a function signature 
> > > > > before calling it is a big code smell in python.If there's a way 
> > > > > to avoid doing that introspection, that would be better.
> > > > Unfortunately, we originally designed this interface to take three 
> > > > arguments, the frame pointer, the breakpoint location and the Python 
> > > > session dict.  Then it became clear that it would be better to add this 
> > > > extra args argument (and in the case of Python based commands the 
> > > > ExecutionContext pointer).  At that point we had three choices, abandon 
> > > > the improvement; switch to unconditionally passing the extra arguments, 
> > > > and break everybody's extant uses; or switch off of the number of 
> > > > arguments to decide whether the user had provided the old or new forms.
> > > > 
> > > > My feeling about lldb Python scripts/commands etc. that people use in 
> > > > the debugger is that a lot of users don't know how they work at all, 
> > > > they 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

labath wrote:
> lawrence_danna wrote:
> > jingham wrote:
> > > lawrence_danna wrote:
> > > > labath wrote:
> > > > > jingham wrote:
> > > > > > lawrence_danna wrote:
> > > > > > > labath wrote:
> > > > > > > > In light of varargs functions (`*args, **kwargs`), which are 
> > > > > > > > fairly popular in python, the concept of "number of arguments 
> > > > > > > > of a callable" does not seem that well defined. The current 
> > > > > > > > implementation seems to return the number of fixed arguments, 
> > > > > > > > which might be fine, but I think this behavior should be 
> > > > > > > > documented. Also, it would be good to modernize this function 
> > > > > > > > signature -- have it take a StringRef, and return a 
> > > > > > > > `Expected` -- ongoing work by @lawrence_danna 
> > > > > > > > will make it possible to return errors from the python 
> > > > > > > > interpreter, and this will make it possible to display those, 
> > > > > > > > instead of just guessing that this is because the callable was 
> > > > > > > > not found (it could in fact be because the named thing is not a 
> > > > > > > > callable, of because resolving the name produced an exception, 
> > > > > > > > ...).
> > > > > > > I just took a look at PythonCallable::GetNumArguments() and it's 
> > > > > > > horribly broken.  
> > > > > > > 
> > > > > > > It doesn't even work for the simplest test case I could think of.
> > > > > > > 
> > > > > > > ```  
> > > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > > auto hex = As(builtins.get().GetAttribute("hex"));
> > > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > > ```
> > > > > > > 
> > > > > > > we should really re-write it to use inspect.signature.
> > > > > > Interesting.  We use it for free functions (what you pass to the -F 
> > > > > > option of `breakpoint command add`) and for the __init__ and 
> > > > > > __call__ method in the little classes you can make up for scripted 
> > > > > > thread plans and for the class version of Python implemented 
> > > > > > command-line commands.  We have tests for telling 3 vrs. 4 vrs. not 
> > > > > > enough or too many, and they all pass.  So it seems to work in the 
> > > > > > cases we currently need it to work for...
> > > > > > 
> > > > > > "inspect.signature" is python 3 only, and the python 2 equivalent 
> > > > > > is deprecated.  So it will take a little fancy footwork to use it 
> > > > > > in the transition period.
> > > > > lol :)
> > > > > 
> > > > > I would actually say that we should try not to use this 
> > > > > function(ality) wherever possible. Making decisions based on the 
> > > > > number of arguments the thing you're about to call takes sounds 
> > > > > weird. I don't want to get too involved in this, but I was designing 
> > > > > this, I'd just say that if one tries to pass arguments to the 
> > > > > callback then the callback MUST take three arguments (or we'll abort 
> > > > > processing the breakpoint command). If he wants his function to be 
> > > > > called both with arguments and without, he can just add a default 
> > > > > value to the third argument. (And if his function takes two 
> > > > > arguments, but he still tries to pass something... well, it's his own 
> > > > > fault).
> > > > > 
> > > > > Anyway, feel free to ignore this comment, but I felt like I had to 
> > > > > say something. :)
> > > > I completely agree with Pavel.Inspecting a function signature 
> > > > before calling it is a big code smell in python.If there's a way to 
> > > > avoid doing that introspection, that would be better.
> > > Unfortunately, we originally designed this interface to take three 
> > > arguments, the frame pointer, the breakpoint location and the Python 
> > > session dict.  Then it became clear that it would be better to add this 
> > > extra args argument (and in the case of Python based commands the 
> > > ExecutionContext pointer).  At that point we had three choices, abandon 
> > > the improvement; switch to unconditionally passing the extra arguments, 
> > > and break everybody's extant uses; or switch off of the number of 
> > > arguments to decide whether the user had provided the old or new forms.
> > > 
> > > My feeling about lldb Python scripts/commands etc. that people use in the 
> > > debugger is that a lot of users don't know how they work at all, they 
> > > just got them from somebody else; and many more figured out how to write 
> > > them for some specific purpose, and then pretty much forgot how they 
> > > worked.  So suddenly breaking all these bits of functionality will result 
> > > in folks 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

lawrence_danna wrote:
> jingham wrote:
> > lawrence_danna wrote:
> > > labath wrote:
> > > > jingham wrote:
> > > > > lawrence_danna wrote:
> > > > > > labath wrote:
> > > > > > > In light of varargs functions (`*args, **kwargs`), which are 
> > > > > > > fairly popular in python, the concept of "number of arguments of 
> > > > > > > a callable" does not seem that well defined. The current 
> > > > > > > implementation seems to return the number of fixed arguments, 
> > > > > > > which might be fine, but I think this behavior should be 
> > > > > > > documented. Also, it would be good to modernize this function 
> > > > > > > signature -- have it take a StringRef, and return a 
> > > > > > > `Expected` -- ongoing work by @lawrence_danna will 
> > > > > > > make it possible to return errors from the python interpreter, 
> > > > > > > and this will make it possible to display those, instead of just 
> > > > > > > guessing that this is because the callable was not found (it 
> > > > > > > could in fact be because the named thing is not a callable, of 
> > > > > > > because resolving the name produced an exception, ...).
> > > > > > I just took a look at PythonCallable::GetNumArguments() and it's 
> > > > > > horribly broken.  
> > > > > > 
> > > > > > It doesn't even work for the simplest test case I could think of.
> > > > > > 
> > > > > > ```  
> > > > > > auto builtins = PythonModule::Import("builtins");
> > > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > > auto hex = As(builtins.get().GetAttribute("hex"));
> > > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > > ```
> > > > > > 
> > > > > > we should really re-write it to use inspect.signature.
> > > > > Interesting.  We use it for free functions (what you pass to the -F 
> > > > > option of `breakpoint command add`) and for the __init__ and __call__ 
> > > > > method in the little classes you can make up for scripted thread 
> > > > > plans and for the class version of Python implemented command-line 
> > > > > commands.  We have tests for telling 3 vrs. 4 vrs. not enough or too 
> > > > > many, and they all pass.  So it seems to work in the cases we 
> > > > > currently need it to work for...
> > > > > 
> > > > > "inspect.signature" is python 3 only, and the python 2 equivalent is 
> > > > > deprecated.  So it will take a little fancy footwork to use it in the 
> > > > > transition period.
> > > > lol :)
> > > > 
> > > > I would actually say that we should try not to use this function(ality) 
> > > > wherever possible. Making decisions based on the number of arguments 
> > > > the thing you're about to call takes sounds weird. I don't want to get 
> > > > too involved in this, but I was designing this, I'd just say that if 
> > > > one tries to pass arguments to the callback then the callback MUST take 
> > > > three arguments (or we'll abort processing the breakpoint command). If 
> > > > he wants his function to be called both with arguments and without, he 
> > > > can just add a default value to the third argument. (And if his 
> > > > function takes two arguments, but he still tries to pass something... 
> > > > well, it's his own fault).
> > > > 
> > > > Anyway, feel free to ignore this comment, but I felt like I had to say 
> > > > something. :)
> > > I completely agree with Pavel.Inspecting a function signature before 
> > > calling it is a big code smell in python.If there's a way to avoid 
> > > doing that introspection, that would be better.
> > Unfortunately, we originally designed this interface to take three 
> > arguments, the frame pointer, the breakpoint location and the Python 
> > session dict.  Then it became clear that it would be better to add this 
> > extra args argument (and in the case of Python based commands the 
> > ExecutionContext pointer).  At that point we had three choices, abandon the 
> > improvement; switch to unconditionally passing the extra arguments, and 
> > break everybody's extant uses; or switch off of the number of arguments to 
> > decide whether the user had provided the old or new forms.
> > 
> > My feeling about lldb Python scripts/commands etc. that people use in the 
> > debugger is that a lot of users don't know how they work at all, they just 
> > got them from somebody else; and many more figured out how to write them 
> > for some specific purpose, and then pretty much forgot how they worked.  So 
> > suddenly breaking all these bits of functionality will result in folks just 
> > deciding that this affordance is not reliable and not worth their time, 
> > which would be a shame.
> > 
> > So instead we accommodate both forms, which requires that we know which one 
> > the 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

jingham wrote:
> lawrence_danna wrote:
> > labath wrote:
> > > jingham wrote:
> > > > lawrence_danna wrote:
> > > > > labath wrote:
> > > > > > In light of varargs functions (`*args, **kwargs`), which are fairly 
> > > > > > popular in python, the concept of "number of arguments of a 
> > > > > > callable" does not seem that well defined. The current 
> > > > > > implementation seems to return the number of fixed arguments, which 
> > > > > > might be fine, but I think this behavior should be documented. 
> > > > > > Also, it would be good to modernize this function signature -- have 
> > > > > > it take a StringRef, and return a `Expected` -- 
> > > > > > ongoing work by @lawrence_danna will make it possible to return 
> > > > > > errors from the python interpreter, and this will make it possible 
> > > > > > to display those, instead of just guessing that this is because the 
> > > > > > callable was not found (it could in fact be because the named thing 
> > > > > > is not a callable, of because resolving the name produced an 
> > > > > > exception, ...).
> > > > > I just took a look at PythonCallable::GetNumArguments() and it's 
> > > > > horribly broken.  
> > > > > 
> > > > > It doesn't even work for the simplest test case I could think of.
> > > > > 
> > > > > ```  
> > > > > auto builtins = PythonModule::Import("builtins");
> > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > auto hex = As(builtins.get().GetAttribute("hex"));
> > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > ```
> > > > > 
> > > > > we should really re-write it to use inspect.signature.
> > > > Interesting.  We use it for free functions (what you pass to the -F 
> > > > option of `breakpoint command add`) and for the __init__ and __call__ 
> > > > method in the little classes you can make up for scripted thread plans 
> > > > and for the class version of Python implemented command-line commands.  
> > > > We have tests for telling 3 vrs. 4 vrs. not enough or too many, and 
> > > > they all pass.  So it seems to work in the cases we currently need it 
> > > > to work for...
> > > > 
> > > > "inspect.signature" is python 3 only, and the python 2 equivalent is 
> > > > deprecated.  So it will take a little fancy footwork to use it in the 
> > > > transition period.
> > > lol :)
> > > 
> > > I would actually say that we should try not to use this function(ality) 
> > > wherever possible. Making decisions based on the number of arguments the 
> > > thing you're about to call takes sounds weird. I don't want to get too 
> > > involved in this, but I was designing this, I'd just say that if one 
> > > tries to pass arguments to the callback then the callback MUST take three 
> > > arguments (or we'll abort processing the breakpoint command). If he wants 
> > > his function to be called both with arguments and without, he can just 
> > > add a default value to the third argument. (And if his function takes two 
> > > arguments, but he still tries to pass something... well, it's his own 
> > > fault).
> > > 
> > > Anyway, feel free to ignore this comment, but I felt like I had to say 
> > > something. :)
> > I completely agree with Pavel.Inspecting a function signature before 
> > calling it is a big code smell in python.If there's a way to avoid 
> > doing that introspection, that would be better.
> Unfortunately, we originally designed this interface to take three arguments, 
> the frame pointer, the breakpoint location and the Python session dict.  Then 
> it became clear that it would be better to add this extra args argument (and 
> in the case of Python based commands the ExecutionContext pointer).  At that 
> point we had three choices, abandon the improvement; switch to 
> unconditionally passing the extra arguments, and break everybody's extant 
> uses; or switch off of the number of arguments to decide whether the user had 
> provided the old or new forms.
> 
> My feeling about lldb Python scripts/commands etc. that people use in the 
> debugger is that a lot of users don't know how they work at all, they just 
> got them from somebody else; and many more figured out how to write them for 
> some specific purpose, and then pretty much forgot how they worked.  So 
> suddenly breaking all these bits of functionality will result in folks just 
> deciding that this affordance is not reliable and not worth their time, which 
> would be a shame.
> 
> So instead we accommodate both forms, which requires that we know which one 
> the user provided.  If you see a better way to do this, (and are willing to 
> implement it because so far as I can see this method is going to work just 
> fine) dig in, I'm not 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 2 inline comments as done.
jingham added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

lawrence_danna wrote:
> labath wrote:
> > In light of varargs functions (`*args, **kwargs`), which are fairly popular 
> > in python, the concept of "number of arguments of a callable" does not seem 
> > that well defined. The current implementation seems to return the number of 
> > fixed arguments, which might be fine, but I think this behavior should be 
> > documented. Also, it would be good to modernize this function signature -- 
> > have it take a StringRef, and return a `Expected` -- ongoing 
> > work by @lawrence_danna will make it possible to return errors from the 
> > python interpreter, and this will make it possible to display those, 
> > instead of just guessing that this is because the callable was not found 
> > (it could in fact be because the named thing is not a callable, of because 
> > resolving the name produced an exception, ...).
> I just took a look at PythonCallable::GetNumArguments() and it's horribly 
> broken.  
> 
> It doesn't even work for the simplest test case I could think of.
> 
> ```  
> auto builtins = PythonModule::Import("builtins");
> ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> auto hex = As(builtins.get().GetAttribute("hex"));
> ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> ```
> 
> we should really re-write it to use inspect.signature.
Interesting.  We use it for free functions (what you pass to the -F option of 
`breakpoint command add`) and for the __init__ and __call__ method in the 
little classes you can make up for scripted thread plans and for the class 
version of Python implemented command-line commands.  We have tests for telling 
3 vrs. 4 vrs. not enough or too many, and they all pass.  So it seems to work 
in the cases we currently need it to work for...

"inspect.signature" is python 3 only, and the python 2 equivalent is 
deprecated.  So it will take a little fancy footwork to use it in the 
transition period.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

lawrence_danna wrote:
> labath wrote:
> > jingham wrote:
> > > lawrence_danna wrote:
> > > > labath wrote:
> > > > > In light of varargs functions (`*args, **kwargs`), which are fairly 
> > > > > popular in python, the concept of "number of arguments of a callable" 
> > > > > does not seem that well defined. The current implementation seems to 
> > > > > return the number of fixed arguments, which might be fine, but I 
> > > > > think this behavior should be documented. Also, it would be good to 
> > > > > modernize this function signature -- have it take a StringRef, and 
> > > > > return a `Expected` -- ongoing work by @lawrence_danna 
> > > > > will make it possible to return errors from the python interpreter, 
> > > > > and this will make it possible to display those, instead of just 
> > > > > guessing that this is because the callable was not found (it could in 
> > > > > fact be because the named thing is not a callable, of because 
> > > > > resolving the name produced an exception, ...).
> > > > I just took a look at PythonCallable::GetNumArguments() and it's 
> > > > horribly broken.  
> > > > 
> > > > It doesn't even work for the simplest test case I could think of.
> > > > 
> > > > ```  
> > > > auto builtins = PythonModule::Import("builtins");
> > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > auto hex = As(builtins.get().GetAttribute("hex"));
> > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > ```
> > > > 
> > > > we should really re-write it to use inspect.signature.
> > > Interesting.  We use it for free functions (what you pass to the -F 
> > > option of `breakpoint command add`) and for the __init__ and __call__ 
> > > method in the little classes you can make up for scripted thread plans 
> > > and for the class version of Python implemented command-line commands.  
> > > We have tests for telling 3 vrs. 4 vrs. not enough or too many, and they 
> > > all pass.  So it seems to work in the cases we currently need it to work 
> > > for...
> > > 
> > > "inspect.signature" is python 3 only, and the python 2 equivalent is 
> > > deprecated.  So it will take a little fancy footwork to use it in the 
> > > transition period.
> > lol :)
> > 
> > I would actually say that we should try not to use this function(ality) 
> > wherever possible. Making decisions based on the number of arguments the 
> > thing you're about to call takes sounds weird. I don't want to get too 
> > involved in this, but I was designing this, I'd just say that 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

labath wrote:
> lawrence_danna wrote:
> > labath wrote:
> > > In light of varargs functions (`*args, **kwargs`), which are fairly 
> > > popular in python, the concept of "number of arguments of a callable" 
> > > does not seem that well defined. The current implementation seems to 
> > > return the number of fixed arguments, which might be fine, but I think 
> > > this behavior should be documented. Also, it would be good to modernize 
> > > this function signature -- have it take a StringRef, and return a 
> > > `Expected` -- ongoing work by @lawrence_danna will make it 
> > > possible to return errors from the python interpreter, and this will make 
> > > it possible to display those, instead of just guessing that this is 
> > > because the callable was not found (it could in fact be because the named 
> > > thing is not a callable, of because resolving the name produced an 
> > > exception, ...).
> > I just took a look at PythonCallable::GetNumArguments() and it's horribly 
> > broken.  
> > 
> > It doesn't even work for the simplest test case I could think of.
> > 
> > ```  
> > auto builtins = PythonModule::Import("builtins");
> > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > auto hex = As(builtins.get().GetAttribute("hex"));
> > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > ```
> > 
> > we should really re-write it to use inspect.signature.
> lol :)
> 
> I would actually say that we should try not to use this function(ality) 
> wherever possible. Making decisions based on the number of arguments the 
> thing you're about to call takes sounds weird. I don't want to get too 
> involved in this, but I was designing this, I'd just say that if one tries to 
> pass arguments to the callback then the callback MUST take three arguments 
> (or we'll abort processing the breakpoint command). If he wants his function 
> to be called both with arguments and without, he can just add a default value 
> to the third argument. (And if his function takes two arguments, but he still 
> tries to pass something... well, it's his own fault).
> 
> Anyway, feel free to ignore this comment, but I felt like I had to say 
> something. :)
I completely agree with Pavel.Inspecting a function signature before 
calling it is a big code smell in python.If there's a way to avoid doing 
that introspection, that would be better.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

lawrence_danna wrote:
> labath wrote:
> > In light of varargs functions (`*args, **kwargs`), which are fairly popular 
> > in python, the concept of "number of arguments of a callable" does not seem 
> > that well defined. The current implementation seems to return the number of 
> > fixed arguments, which might be fine, but I think this behavior should be 
> > documented. Also, it would be good to modernize this function signature -- 
> > have it take a StringRef, and return a `Expected` -- ongoing 
> > work by @lawrence_danna will make it possible to return errors from the 
> > python interpreter, and this will make it possible to display those, 
> > instead of just guessing that this is because the callable was not found 
> > (it could in fact be because the named thing is not a callable, of because 
> > resolving the name produced an exception, ...).
> I just took a look at PythonCallable::GetNumArguments() and it's horribly 
> broken.  
> 
> It doesn't even work for the simplest test case I could think of.
> 
> ```  
> auto builtins = PythonModule::Import("builtins");
> ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> auto hex = As(builtins.get().GetAttribute("hex"));
> ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
> EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> ```
> 
> we should really re-write it to use inspect.signature.
lol :)

I would actually say that we should try not to use this function(ality) 
wherever possible. Making decisions based on the number of arguments the thing 
you're about to call takes sounds weird. I don't want to get too involved in 
this, but I was designing this, I'd just say that if one tries to pass 
arguments to the callback then the callback MUST take three arguments (or we'll 
abort processing the breakpoint command). If he wants his function to be called 
both with arguments and without, he can just add a default value to the third 
argument. (And if his function takes two arguments, but he still tries to pass 
something... well, it's his own fault).

Anyway, feel free to ignore this comment, but I felt like I had to say 
something. :)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Lawrence D'Anna via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

labath wrote:
> In light of varargs functions (`*args, **kwargs`), which are fairly popular 
> in python, the concept of "number of arguments of a callable" does not seem 
> that well defined. The current implementation seems to return the number of 
> fixed arguments, which might be fine, but I think this behavior should be 
> documented. Also, it would be good to modernize this function signature -- 
> have it take a StringRef, and return a `Expected` -- ongoing 
> work by @lawrence_danna will make it possible to return errors from the 
> python interpreter, and this will make it possible to display those, instead 
> of just guessing that this is because the callable was not found (it could in 
> fact be because the named thing is not a callable, of because resolving the 
> name produced an exception, ...).
I just took a look at PythonCallable::GetNumArguments() and it's horribly 
broken.  

It doesn't even work for the simplest test case I could think of.

```  
auto builtins = PythonModule::Import("builtins");
ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
auto hex = As(builtins.get().GetAttribute("hex"));
ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());  
EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
```

we should really re-write it to use inspect.signature.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 224929.
jingham added a comment.

Addressed Pavel's comments.

I explicitly want the number of fixed arguments, so I changed the function name 
to GetNumFixedArguments.  The docs say explicitly that you have to make the 
function take either three or four fixed arguments, and that's what I have to 
check against.  I don't want to know about varargs and optional named arguments.

I added the Expected, though I didn't plumb the change into 
PythonCallable::GetNumArguments.  That's of marginal extra benefit, and 
orthogonal to the purposes of this patch.

Note also, the fact that I was now checking argument signatures when adding the 
command means you can no longer do:

  (lldb) break command add -F myfunc.function
  (lldb) command script import myfunc.py

which TestBreakpointCommand.py was doing.  You have to make the Python function 
available BEFORE adding it to the breakpoint.  I don't think this was an 
feature, and I don't see any harm in adding that requirement.  But it is a 
change in behavior so I thought I'd bring it up.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

Files:
  lldb/include/lldb/API/SBBreakpoint.h
  lldb/include/lldb/API/SBBreakpointLocation.h
  lldb/include/lldb/API/SBBreakpointName.h
  lldb/include/lldb/API/SBStructuredData.h
  lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommandsFromPython.py
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/bktptcmd.py
  lldb/scripts/Python/python-wrapper.swig
  lldb/scripts/interface/SBBreakpoint.i
  lldb/scripts/interface/SBBreakpointLocation.i
  lldb/scripts/interface/SBBreakpointName.i
  lldb/source/API/SBBreakpoint.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/API/SBBreakpointName.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Interpreter/OptionGroupPythonClassWithDict.cpp
  lldb/source/Interpreter/ScriptInterpreter.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -62,7 +62,8 @@
 extern "C" bool LLDBSwigPythonBreakpointCallbackFunction(
 const char *python_function_name, const char *session_dictionary_name,
 const lldb::StackFrameSP _frame,
-const lldb::BreakpointLocationSP _bp_loc) {
+const lldb::BreakpointLocationSP _bp_loc,
+StructuredDataImpl *args_impl) {
   return false;
 }
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -179,8 +179,10 @@
   Status GenerateFunction(const char *signature,
   const StringList ) override;
 
-  Status GenerateBreakpointCommandCallbackData(StringList ,
-   std::string ) override;
+  Status GenerateBreakpointCommandCallbackData(
+  StringList ,
+  std::string ,
+  bool has_extra_args) override;
 
   bool GenerateWatchpointCommandCallbackData(StringList ,
  std::string ) override;
@@ -244,14 +246,22 @@
   Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
   const char *callback_body) override;
 
-  void SetBreakpointCommandCallbackFunction(BreakpointOptions *bp_options,
-const char *function_name) override;
+  Status SetBreakpointCommandCallbackFunction(
+  BreakpointOptions *bp_options,
+  const char *function_name,
+  StructuredData::ObjectSP extra_args_sp) override;
 
   /// This one is for deserialization:
   Status SetBreakpointCommandCallback(
   BreakpointOptions *bp_options,
   std::unique_ptr _up) override;
 
+  Status SetBreakpointCommandCallback(
+  BreakpointOptions *bp_options, 
+   const char *command_body_text,
+   StructuredData::ObjectSP extra_args_sp,
+   bool 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Pavel Labath via Phabricator via lldb-commits]

labath added subscribers: lawrence_danna, labath.
labath added inline comments.



Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+return -1;

In light of varargs functions (`*args, **kwargs`), which are fairly popular in 
python, the concept of "number of arguments of a callable" does not seem that 
well defined. The current implementation seems to return the number of fixed 
arguments, which might be fine, but I think this behavior should be documented. 
Also, it would be good to modernize this function signature -- have it take a 
StringRef, and return a `Expected` -- ongoing work by 
@lawrence_danna will make it possible to return errors from the python 
interpreter, and this will make it possible to display those, instead of just 
guessing that this is because the callable was not found (it could in fact be 
because the named thing is not a callable, of because resolving the name 
produced an exception, ...).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 224719.
jingham added a comment.

I added ScriptInterpreter::GetNumArgumentsForCallable, so I can base all the 
decisions on how to call the function on whether I was passed a function with 3 
or 4 arguments.  That's cleaner.  I also plumbed an error through the callback 
setting, so I can report an error if you pass me a function with the wrong 
number of arguments, or provide extra_args when you are passing a function that 
doesn't take 4 arguments.

I added a check in the extant test to ensure that a function that takes 
extra_args will work with empty extra_args, since there's no reason to disallow 
that.

I also added a test for the error handling when you pass a function with the 
wrong number of arguments.

Now we don't depend on the SBStructuredData handed across the SB API's being 
any particular structure.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

Files:
  lldb/include/lldb/API/SBBreakpoint.h
  lldb/include/lldb/API/SBBreakpointLocation.h
  lldb/include/lldb/API/SBBreakpointName.h
  lldb/include/lldb/API/SBStructuredData.h
  lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommandsFromPython.py
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/bktptcmd.py
  lldb/scripts/Python/python-wrapper.swig
  lldb/scripts/interface/SBBreakpoint.i
  lldb/scripts/interface/SBBreakpointLocation.i
  lldb/scripts/interface/SBBreakpointName.i
  lldb/source/API/SBBreakpoint.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/API/SBBreakpointName.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Interpreter/OptionGroupPythonClassWithDict.cpp
  lldb/source/Interpreter/ScriptInterpreter.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -62,7 +62,8 @@
 extern "C" bool LLDBSwigPythonBreakpointCallbackFunction(
 const char *python_function_name, const char *session_dictionary_name,
 const lldb::StackFrameSP _frame,
-const lldb::BreakpointLocationSP _bp_loc) {
+const lldb::BreakpointLocationSP _bp_loc,
+StructuredDataImpl *args_impl) {
   return false;
 }
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -179,8 +179,10 @@
   Status GenerateFunction(const char *signature,
   const StringList ) override;
 
-  Status GenerateBreakpointCommandCallbackData(StringList ,
-   std::string ) override;
+  Status GenerateBreakpointCommandCallbackData(
+  StringList ,
+  std::string ,
+  bool has_extra_args) override;
 
   bool GenerateWatchpointCommandCallbackData(StringList ,
  std::string ) override;
@@ -244,14 +246,22 @@
   Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
   const char *callback_body) override;
 
-  void SetBreakpointCommandCallbackFunction(BreakpointOptions *bp_options,
-const char *function_name) override;
+  Status SetBreakpointCommandCallbackFunction(
+  BreakpointOptions *bp_options,
+  const char *function_name,
+  StructuredData::ObjectSP extra_args_sp) override;
 
   /// This one is for deserialization:
   Status SetBreakpointCommandCallback(
   BreakpointOptions *bp_options,
   std::unique_ptr _up) override;
 
+  Status SetBreakpointCommandCallback(
+  BreakpointOptions *bp_options, 
+   const char *command_body_text,
+   StructuredData::ObjectSP extra_args_sp,
+   bool uses_extra_args);
+
   /// Set a one-liner as the callback for the watchpoint.
   void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
 const char *oneliner) override;
@@ -366,6 +376,8 @@
   PythonDictionary ();
 
   PythonDictionary ();
+  
+  int GetNumArgumentsForCallable(const 

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

> The only place where I treat the extra_args as a Dictionary is when I use as 
> a way to tell whether the extra_args is empty, which I need to write the 
> right wrapper function in GenerateBreakpointCommandCallbackData. That 
> function is little annoying because it doesn't know whether it is calling a 
> function or some text gotten from the I/O handler, so I can't count the 
> function arguments there to decide which signature to use.
> 
> Let me see if I can make this cleaner so I don't have to rely on that. I 
> really don't intend to limit fancier uses of the extra args.

Let me know if you are going to look into making this cleaner for this patch. 
If so, I will hold off, else I can accept if you are out of time


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D68671#1704803 , @clayborg wrote:

> Looks good overall. A few questions:
>
> - should we do a global "s/extra_args/args/" edit in this patch? Not sure 
> what "extra_" is adding? Can we remove?


Since we still support the form that doesn't take extra_args, they do feel 
"extra" to me.  I also like the "extra" because it helps make it clear these 
are just free bits of data that the callback system just passes along.  After 
all, there are some fixed arguments to the call back that do have specific 
meanings.

Note, I called the equivalent feature "extra_args" for the scripted breakpoint 
resolvers (added last year) and scripted thread plans, so if we change it here, 
we need to change it in those places as well.  If we're going to do that, I'll 
do it as a follow on patch because I don't want to mix those changes in with 
this patch.  But as I said, I like the "extra".

> - Can the structured data not be a dictionary? Valid StructuredData could be 
> "malloc" or "[0x1000, 0x2000]" or "true", etc. If the data is required to be 
> a dictionary we should provide lldb::SBError support for the new API calls to 
> ensure we can let the user know what they did wrong. Be good to test this as 
> well.

The only place where I treat the extra_args as a Dictionary is when I use as a 
way to tell whether the extra_args is empty, which I need to write the right 
wrapper function in GenerateBreakpointCommandCallbackData.  That function is 
little annoying because it doesn't know whether it is calling a function or 
some text gotten from the I/O handler, so I can't count the function arguments 
there to decide which signature to use.

Let me see if I can make this cleaner so I don't have to rely on that.  I 
really don't intend to limit fancier uses of the extra args.

Note, the command-line doesn't allow you to make anything but a dictionary 
(through the -k -v option pairs).  I'm actually happy with that, it enforces 
some kind of structure on the breakpoint command callback that you write.  
You'll thank me when you want to add a second or third parameter...

Plus once you write something you are going to use frequently, you can do:

(lldb) command alias caller_break breakpoint command add -F 
MyCommands.break_when_parent -k func_name -v %1
(lldb) caller_break Caller -n Callee

That's how you would really want to use it.

> - is it possible to update the args for a callback function? That would seem 
> like a good thing you might want to do? And if so, to test?

There's no way to do this.  "breakpoint command add" is a bit poorly named, 
since it really overwrites the current command rather than adding to it.  But 
that's the way it has always worked.   So only giving you the option to provide 
a new pair {python function, extra_args} is in keeping with its behavior.

People have asked for a "breakpoint command modify" that could chain python 
callbacks, or incrementally add command-line commands.  That would also 
naturally allow you to change/add to the extra args in place.  But that's a 
whole other piece of work that I don't want to take on here.

> - If it is possible to update the structured data, and if the structured data 
> can be anything (array, string, boolean, dictionary), we might want an option 
> like: --json "..." to be able to set or update from the command line? The 
> current -k -v options seem to imply it must be a dict and only one level deep?

I thought about adding --json when making the OptionGroupPythonClassWithDict.  
The completionist side of me was sorely tempted, especially since it would be 
quite easy to do.  But the options for "break set" are already several pages 
long, so I don't want to add more options to it just "because I can".

For all the use cases I can think of, a simple one level dictionary will 
suffice.  If you want to do something fancier, you can make an arbitrarily 
complicated SBStructuredData in Python, and then use 
SBBreakpoint.SetScriptCallbackFunction to pass it in.  And if you want to do 
that something fancier a lot, you can make this into a python command.

One useful enhancement that wouldn't overburden "break set" would be to call 
ParseJSON on the -v option value, and if that succeeded add the resultant 
StructuredData::ObjectSP as the value.  That would allow you to pass lists in 
for the value, which does seem useful.  You can of course do this yourself by 
getting the string value out in your callback and passing it to 
SBStructuredData.ParseFromJSON().  So I don't think it's important to add this 
now. I'd also need to assure myself that this wouldn't surprise people by 
munging up legitimate string values.

But anyway, I'd prefer to do that as a follow on, since I'm running out of time 
to play with this this time round the wheel...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks good overall. A few questions:

- should we do a global "s/extra_args/args/" edit in this patch? Not sure what 
"extra_" is adding? Can we remove?
- Can the structured data not be a dictionary? Valid StructuredData could be 
"malloc" or "[0x1000, 0x2000]" or "true", etc. If the data is required to be a 
dictionary we should provide lldb::SBError support for the new API calls to 
ensure we can let the user know what they did wrong. Be good to test this as 
well.
- is it possible to update the args for a callback function? That would seem 
like a good thing you might want to do? And if so, to test?
  - If it is possible to update the structured data, and if the structured data 
can be anything (array, string, boolean, dictionary), we might want an option 
like: --json "..." to be able to set or update from the command line? The 
current -k -v options seem to imply it must be a dict and only one level deep?




Comment at: lldb/include/lldb/API/SBBreakpointLocation.h:59
+  void SetScriptCallbackFunction(const char *callback_function_name,
+ SBStructuredData _args);
+

shafik wrote:
> Could this be a `const &`
const doesn't mean anything for lldb::SB arguments. Since they contain shared 
pointers or unique pointers, so just leave as is.



Comment at: lldb/include/lldb/API/SBBreakpointName.h:89
+  void SetScriptCallbackFunction(const char *callback_function_name,
+ SBStructuredData _args);
+

shafik wrote:
> Could this be a `const &`
const doesn't mean anything for lldb::SB arguments. Since they contain shared 
pointers or unique pointers, so just leave as is.



Comment at: lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h:26-30
   OptionGroupPythonClassWithDict(const char *class_use,
+  bool is_class = true,
   int class_option = 'C',
   int key_option = 'k', 
+  int value_option = 'v');

clang-format? Indent seems off (it was before too I see).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: lldb/include/lldb/API/SBBreakpointLocation.h:59
+  void SetScriptCallbackFunction(const char *callback_function_name,
+ SBStructuredData _args);
+

Could this be a `const &`



Comment at: lldb/include/lldb/API/SBBreakpointName.h:89
+  void SetScriptCallbackFunction(const char *callback_function_name,
+ SBStructuredData _args);
+

Could this be a `const &`


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671



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

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

For example, it is pretty easy to write a breakpoint command that implements 
"stop when my caller is Foo", and it is pretty easy to write a breakpoint 
command that implements "stop when my caller is Bar".  But there's no way to 
write a generic "stop when my caller is..." function, and then specify the 
caller when you add the command.  With this patch, you can pass this data in a 
SBStructuredData dictionary.  That will get stored in the PythonCommandBaton 
for the breakpoint, and passed to the implementation function (if it has the 
right signature) when the breakpoint is hit.  Then in lldb, you can say:

(lldb) break com add -F caller_is -k caller_name -v Foo

More generally this will allow us to write reusable Python breakpoint commands.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D68671

Files:
  lldb/include/lldb/API/SBBreakpoint.h
  lldb/include/lldb/API/SBBreakpointLocation.h
  lldb/include/lldb/API/SBBreakpointName.h
  lldb/include/lldb/API/SBStructuredData.h
  lldb/include/lldb/Interpreter/OptionGroupPythonClassWithDict.h
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommandsFromPython.py
  
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/bktptcmd.py
  lldb/scripts/Python/python-wrapper.swig
  lldb/scripts/interface/SBBreakpoint.i
  lldb/scripts/interface/SBBreakpointLocation.i
  lldb/scripts/interface/SBBreakpointName.i
  lldb/source/API/SBBreakpoint.cpp
  lldb/source/API/SBBreakpointLocation.cpp
  lldb/source/API/SBBreakpointName.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Interpreter/OptionGroupPythonClassWithDict.cpp
  lldb/source/Interpreter/ScriptInterpreter.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -62,7 +62,8 @@
 extern "C" bool LLDBSwigPythonBreakpointCallbackFunction(
 const char *python_function_name, const char *session_dictionary_name,
 const lldb::StackFrameSP _frame,
-const lldb::BreakpointLocationSP _bp_loc) {
+const lldb::BreakpointLocationSP _bp_loc,
+StructuredDataImpl *args_impl) {
   return false;
 }
 
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -179,8 +179,10 @@
   Status GenerateFunction(const char *signature,
   const StringList ) override;
 
-  Status GenerateBreakpointCommandCallbackData(StringList ,
-   std::string ) override;
+  Status GenerateBreakpointCommandCallbackData(
+  StringList ,
+  std::string ,
+  StructuredData::ObjectSP extra_args_sp) override;
 
   bool GenerateWatchpointCommandCallbackData(StringList ,
  std::string ) override;
@@ -244,14 +246,21 @@
   Status SetBreakpointCommandCallback(BreakpointOptions *bp_options,
   const char *callback_body) override;
 
-  void SetBreakpointCommandCallbackFunction(BreakpointOptions *bp_options,
-const char *function_name) override;
+  void SetBreakpointCommandCallbackFunction(
+  BreakpointOptions *bp_options,
+  const char *function_name,
+  StructuredData::ObjectSP extra_args_sp) override;
 
   /// This one is for deserialization:
   Status SetBreakpointCommandCallback(
   BreakpointOptions *bp_options,
   std::unique_ptr _up) override;
 
+  Status SetBreakpointCommandCallback(
+  BreakpointOptions *bp_options, 
+   const char *command_body_text,
+   StructuredData::ObjectSP extra_args_sp);
+
   /// Set a one-liner as the callback for the watchpoint.
   void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
 const char *oneliner) override;
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===
---