[python] Allow explicit locations in breakpoints.

[python] Allow explicit locations in breakpoints.

[Phil Muldoon]

Hello,

This minor patch allows for explicit locations to be specified in the
gdb.Breakpoint constructor.  It's a minor tweak, and the patch largely
consists of tests to make sure the existing explicit locations
mechanisms work in Python.

Cheers

Phil

--

2017-08-23  Phil Muldoon  <pmuldoon@redhat.com>

       * python/py-breakpoint.c (bppy_init): Use string_to_event_location
       over basic location code.

2017-08-23  Phil Muldoon  <pmuldoon@redhat.com>

       * python.texi (Breakpoints In Python): Add text relating
       to allowed explicit location in gdb.Breakpoints.

2017-08-23  Phil Muldoon  <pmuldoon@redhat.com>

       * gdb.python/py-breakpoint.exp (test_bkpt_explicit_loc): Add new
       tests for explicit locations.

--

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 32d7939e66..a214c2a9e7 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4825,7 +4825,8 @@ class.
 Create a new breakpoint according to @var{spec}, which is a string
 naming the location of the breakpoint, or an expression that defines a
 watchpoint.  The contents can be any location recognized by the
-@code{break} command, or in the case of a watchpoint, by the
+@code{break} command, including those set as explicit locations
+(@pxref{Explicit Locations}), or in the case of a watchpoint, by the
 @code{watch} command.  The optional @var{type} denotes the breakpoint
 to create from the types defined later in this chapter.  This argument
 can be either @code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}; it
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 6156eb6179..8431bed939 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -681,7 +681,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	case bp_breakpoint:
 	  {
 	    event_location_up location
-	      = string_to_event_location_basic (&copy, current_language);
+	      = string_to_event_location (&copy, current_language);
 	    create_breakpoint (python_gdbarch,
 			       location.get (), NULL, -1, NULL,
 			       0,
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
index df6163e53a..562cab35f7 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.c
+++ b/gdb/testsuite/gdb.python/py-breakpoint.c
@@ -24,7 +24,7 @@ int multiply (int i)
 
 int add (int i)
 {
-  return i + i; 
+  return i + i;  /* Break at function add.  */
 }
 
 
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index bd138ac3d2..228489f74e 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -547,6 +547,72 @@ proc test_bkpt_events {} {
     check_last_event breakpoint_deleted
 }
 
+proc test_bkpt_explicit_loc {} {
+    global srcfile testfile
+
+    with_test_prefix test_bkpt_invisible {
+	# Start with a fresh gdb.
+	clean_restart ${testfile}
+
+	if ![runto_main] then {
+	    fail "cannot run to main."
+	    return 0
+	}
+
+	delete_breakpoints
+
+	set bp_location1 [gdb_get_line_number "Break at multiply."]
+	set bp_location2 [gdb_get_line_number "Break at add."]
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-li $bp_location1\")" \
+	    "Set explicit breakpoint by line" 0
+	gdb_continue_to_breakpoint "Break at multiply" \
+	    ".*Break at multiply.*"
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-li +1\")" \
+	    "Set explicit breakpoint by relative line" 0
+	gdb_continue_to_breakpoint "Break at add" \
+	    ".*Break at add.*"
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-li -1\")" \
+	    "Set explicit breakpoint by relative negative line" 0
+	gdb_continue_to_breakpoint "Break at multiply" \
+	    ".*Break at multiply.*"
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-function add\")" \
+	    "Set explicit breakpoint by function" 0
+	gdb_continue_to_breakpoint "Break at function add" \
+	    ".*Break at function add.*"
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -function add\")" \
+	    "Set explicit breakpoint by source file and function" 0
+	gdb_continue_to_breakpoint "Break at function add" \
+	    ".*Break at function add.*"
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -li $bp_location2\")" \
+	    "Set explicit breakpoint by source file and line number" 0
+	gdb_continue_to_breakpoint "Break at add" \
+	    ".*Break at add.*"
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (\"-source $srcfile\")" \
+	    "RuntimeError: Source filename requires function, label, or line offset.*" \
+	    "Set invalid explicit breakpoint by source only"
+	# The below will print a warning but set pending breakpoints.
+	gdb_test "python bp1 = gdb.Breakpoint (\"-source foo.c -li 5\")" \
+	    "No source file named foo.*" \
+	    "Set invalid explicit breakpoint by missing source and line"
+	gdb_test "python bp1 = gdb.Breakpoint (\"-source $srcfile -li 900\")" \
+	    "No line 900 in file \"$srcfile\".*" \
+	    "Set invalid explicit breakpoint by source and invalid line."
+	gdb_test "python bp1 = gdb.Breakpoint (\"-function blah\")" \
+	    "Function \"blah\" not defined.*" \
+	    "Set invalid explicit breakpoint by missing function."
+	# Invalid explicit location flags.
+	gdb_test "python bp1 = gdb.Breakpoint (\"-foo -li 5\")" \
+	    "RuntimeError: invalid explicit location argument, \"-foo\".*" \
+	    "Set invalid explicit breakpoint by wrong flag"
+    }
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -558,3 +624,4 @@ test_bkpt_temporary
 test_bkpt_address
 test_bkpt_pending
 test_bkpt_events
+test_bkpt_explicit_loc

Re: [python] Allow explicit locations in breakpoints.

[Keith Seitz]

On 08/23/2017 06:58 AM, Phil Muldoon wrote:

> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 6156eb6179..8431bed939 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -681,7 +681,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  	case bp_breakpoint:
>  	  {
>  	    event_location_up location
> -	      = string_to_event_location_basic (&copy, current_language);
> +	      = string_to_event_location (&copy, current_language);
>  	    create_breakpoint (python_gdbarch,
>  			       location.get (), NULL, -1, NULL,
>  			       0,

This binds python interfaces to the CLI, and I don't think we want that. I would have expected (perhaps naively) to see explicit locations supported using a more natural python convention, such as using PyArg_ParseTupleAndKeywords.

For example, in MI (mi_cmd_break_insert_1) , we do not use string_to_event_location. We support MI-centric calling conventions by using mi_getopt for argument processing. While MI does use the same option names, they don't (or didn't) have to be. The comments for string_to_event_location should be clearer that this is a CLI-specific implementation. [Perhaps that entire function could be moved to somewhere in cli/?]

I admit, like the MI case, it is almost busywork, but it does (at least) isolate those interfaces from any internal API churn that GDB might undergo.

WDYT?

Keith

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 23/08/17 18:51, Keith Seitz wrote:
> On 08/23/2017 06:58 AM, Phil Muldoon wrote:
> 
>> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
>> index 6156eb6179..8431bed939 100644
>> --- a/gdb/python/py-breakpoint.c
>> +++ b/gdb/python/py-breakpoint.c
>> @@ -681,7 +681,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>>  	case bp_breakpoint:
>>  	  {
>>  	    event_location_up location
>> -	      = string_to_event_location_basic (&copy, current_language);
>> +	      = string_to_event_location (&copy, current_language);
>>  	    create_breakpoint (python_gdbarch,
>>  			       location.get (), NULL, -1, NULL,
>>  			       0,
> 
> This binds python interfaces to the CLI, and I don't think we want
> that. I would have expected (perhaps naively) to see explicit
> locations supported using a more natural python convention, such as
> using PyArg_ParseTupleAndKeywords.

My original implementation was that. I had function="foo" etc as
parameters to the constructor. The problem is with the -line
parameters and relative parameters. So line=+3 won't work in the
Python parameter sense. So it would have to be line="+3" in the
constructor. If keywords are all strings, I'm not sure I see the point
of parsing them as keywords when you can just specify the whole string
(i.e.  gdb.Breakpoint("-source=foo.c -line=28"). This is what we do,
more less, with the current constructor: IE

foo = gdb.Breakpoint("bar.c:23")
or
foo = gdb.Breakpoint("functionName").

So this patch allows the addition of explicit location parsing in much
the same vein as we handle regular breakpoints. (That is, with a
CLI-like interface). The gdb.Breakpoint class, for better or for
worse, is based pretty much on the interface to create_breakpoint. I'm
not adverse to implementing keywords; it's a little extra string
wrangling, but I'm not clear on the benefit? The gdb.Breakpoint class
has always handed over the interpretation of breakpoints to GDB using
the CLI like syntax. I can see why MI separates it out, and I take
your point well, in a machine context.

Cheers

Phil

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 23/08/17 14:58, Phil Muldoon wrote:
> 
> Hello,
> 
> This minor patch allows for explicit locations to be specified in the
> gdb.Breakpoint constructor.  It's a minor tweak, and the patch largely
> consists of tests to make sure the existing explicit locations
> mechanisms work in Python.

Ping on this.

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 12/09/17 11:03, Phil Muldoon wrote:
> On 23/08/17 14:58, Phil Muldoon wrote:
>>
>> Hello,
>>
>> This minor patch allows for explicit locations to be specified in the
>> gdb.Breakpoint constructor.  It's a minor tweak, and the patch largely
>> consists of tests to make sure the existing explicit locations
>> mechanisms work in Python.
> 
> Ping on this.
> 

Ping.

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 02/10/17 16:18, Phil Muldoon wrote:
> On 12/09/17 11:03, Phil Muldoon wrote:
>> On 23/08/17 14:58, Phil Muldoon wrote:
>>>
>>> Hello,
>>>
>>> This minor patch allows for explicit locations to be specified in the
>>> gdb.Breakpoint constructor.  It's a minor tweak, and the patch largely
>>> consists of tests to make sure the existing explicit locations
>>> mechanisms work in Python.
>>
>> Ping on this.
>>
> 
> Ping.
> 

Ping

Cheers

Phil

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-08-23 02:30 PM, Phil Muldoon wrote:
> On 23/08/17 18:51, Keith Seitz wrote:
>> On 08/23/2017 06:58 AM, Phil Muldoon wrote:
>>
>>> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
>>> index 6156eb6179..8431bed939 100644
>>> --- a/gdb/python/py-breakpoint.c
>>> +++ b/gdb/python/py-breakpoint.c
>>> @@ -681,7 +681,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>>>  	case bp_breakpoint:
>>>  	  {
>>>  	    event_location_up location
>>> -	      = string_to_event_location_basic (&copy, current_language);
>>> +	      = string_to_event_location (&copy, current_language);
>>>  	    create_breakpoint (python_gdbarch,
>>>  			       location.get (), NULL, -1, NULL,
>>>  			       0,
>>
>> This binds python interfaces to the CLI, and I don't think we want
>> that. I would have expected (perhaps naively) to see explicit
>> locations supported using a more natural python convention, such as
>> using PyArg_ParseTupleAndKeywords.
> 
> My original implementation was that. I had function="foo" etc as
> parameters to the constructor. The problem is with the -line
> parameters and relative parameters. So line=+3 won't work in the
> Python parameter sense. So it would have to be line="+3" in the
> constructor. If keywords are all strings, I'm not sure I see the point
> of parsing them as keywords when you can just specify the whole string
> (i.e.  gdb.Breakpoint("-source=foo.c -line=28"). This is what we do,
> more less, with the current constructor: IE
> 
> foo = gdb.Breakpoint("bar.c:23")
> or
> foo = gdb.Breakpoint("functionName").
> 
> So this patch allows the addition of explicit location parsing in much
> the same vein as we handle regular breakpoints. (That is, with a
> CLI-like interface). The gdb.Breakpoint class, for better or for
> worse, is based pretty much on the interface to create_breakpoint. I'm
> not adverse to implementing keywords; it's a little extra string
> wrangling, but I'm not clear on the benefit? The gdb.Breakpoint class
> has always handed over the interpretation of breakpoints to GDB using
> the CLI like syntax. I can see why MI separates it out, and I take
> your point well, in a machine context.

I think for Python it would make sense to support the two paradigms.  If you
are writing a Python command that ends up installing a breakpoint, it would
be nice if you could directly pass what you received to the gdb.Breakpoint
constructor and have it parse it (including explicit locations).  For example,

  (gdb) special-break -file foo.c -line 17

But it would also be nice to have a keywords based API, for when the line/file/function
information is already split.  It would avoid having to build an explicit linespec
string just to have GDB parse it after.

In terms of API, I think the "spec" argument could be mutually exclusive with
the function/file/line/etc keywork arguments, which would be added.  An error
would be thrown if you try to use both ways at the same time.

About the line="+3" issue, because this is Python, the line keyword could
probably accept integers and strings.  And if it's a string, there could
be some validation on the format.

Simon

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index bd138ac3d2..228489f74e 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -547,6 +547,72 @@ proc test_bkpt_events {} {
>      check_last_event breakpoint_deleted
>  }
>  
> +proc test_bkpt_explicit_loc {} {
> +    global srcfile testfile
> +
> +    with_test_prefix test_bkpt_invisible {

This should be test_bkpt_explicit_loc.  But I think I'll do a pass
and make all these procs use "proc_with_prefix", to avoid having to
repeat the proc name.

> +	# Start with a fresh gdb.
> +	clean_restart ${testfile}
> +
> +	if ![runto_main] then {
> +	    fail "cannot run to main."
> +	    return 0
> +	}
> +
> +	delete_breakpoints
> +
> +	set bp_location1 [gdb_get_line_number "Break at multiply."]
> +	set bp_location2 [gdb_get_line_number "Break at add."]
> +
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-li $bp_location1\")" \
> +	    "Set explicit breakpoint by line" 0> +	gdb_continue_to_breakpoint "Break at multiply" \
> +	    ".*Break at multiply.*"
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-li +1\")" \
> +	    "Set explicit breakpoint by relative line" 0
> +	gdb_continue_to_breakpoint "Break at add" \
> +	    ".*Break at add.*"
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-li -1\")" \
> +	    "Set explicit breakpoint by relative negative line" 0
> +	gdb_continue_to_breakpoint "Break at multiply" \
> +	    ".*Break at multiply.*"
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-function add\")" \
> +	    "Set explicit breakpoint by function" 0
> +	gdb_continue_to_breakpoint "Break at function add" \
> +	    ".*Break at function add.*"
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -function add\")" \
> +	    "Set explicit breakpoint by source file and function" 0
> +	gdb_continue_to_breakpoint "Break at function add" \
> +	    ".*Break at function add.*"
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -li $bp_location2\")" \
> +	    "Set explicit breakpoint by source file and line number" 0
> +	gdb_continue_to_breakpoint "Break at add" \
> +	    ".*Break at add.*"
> +	delete_breakpoints
> +	gdb_test "python bp1 = gdb.Breakpoint (\"-source $srcfile\")" \
> +	    "RuntimeError: Source filename requires function, label, or line offset.*" \
> +	    "Set invalid explicit breakpoint by source only"
> +	# The below will print a warning but set pending breakpoints.
> +	gdb_test "python bp1 = gdb.Breakpoint (\"-source foo.c -li 5\")" \
> +	    "No source file named foo.*" \
> +	    "Set invalid explicit breakpoint by missing source and line"
> +	gdb_test "python bp1 = gdb.Breakpoint (\"-source $srcfile -li 900\")" \
> +	    "No line 900 in file \"$srcfile\".*" \
> +	    "Set invalid explicit breakpoint by source and invalid line."
> +	gdb_test "python bp1 = gdb.Breakpoint (\"-function blah\")" \
> +	    "Function \"blah\" not defined.*" \
> +	    "Set invalid explicit breakpoint by missing function."
> +	# Invalid explicit location flags.
> +	gdb_test "python bp1 = gdb.Breakpoint (\"-foo -li 5\")" \
> +	    "RuntimeError: invalid explicit location argument, \"-foo\".*" \
> +	    "Set invalid explicit breakpoint by wrong flag"

For readability, could you add some empty lines between the logical blocks above?

> +    }
> +}
> +
>  test_bkpt_basic
>  test_bkpt_deletion
>  test_bkpt_cond_and_cmds
> @@ -558,3 +624,4 @@ test_bkpt_temporary
>  test_bkpt_address
>  test_bkpt_pending
>  test_bkpt_events
> +test_bkpt_explicit_loc
> 

Simon

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-10-16 02:23 PM, Simon Marchi wrote:
> I think for Python it would make sense to support the two paradigms.  If you
> are writing a Python command that ends up installing a breakpoint, it would
> be nice if you could directly pass what you received to the gdb.Breakpoint
> constructor and have it parse it (including explicit locations).  For example,
> 
>   (gdb) special-break -file foo.c -line 17
> 
> But it would also be nice to have a keywords based API, for when the line/file/function
> information is already split.  It would avoid having to build an explicit linespec
> string just to have GDB parse it after.
> 
> In terms of API, I think the "spec" argument could be mutually exclusive with
> the function/file/line/etc keywork arguments, which would be added.  An error
> would be thrown if you try to use both ways at the same time.
> 
> About the line="+3" issue, because this is Python, the line keyword could
> probably accept integers and strings.  And if it's a string, there could
> be some validation on the format.
> 
> Simon

Btw, if we went with the idea described above, I think your patch would be
acceptable as-is, and the work to do add keyword arguments could be done separately
(by you or somebody else).

Simon

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 16/10/17 19:23, Simon Marchi wrote:
> On 2017-08-23 02:30 PM, Phil Muldoon wrote:
>> On 23/08/17 18:51, Keith Seitz wrote:
>>> On 08/23/2017 06:58 AM, Phil Muldoon wrote:

> I think for Python it would make sense to support the two paradigms.  If you
> are writing a Python command that ends up installing a breakpoint, it would
> be nice if you could directly pass what you received to the gdb.Breakpoint
> constructor and have it parse it (including explicit locations).  For example,
> 
>   (gdb) special-break -file foo.c -line 17
> 
> But it would also be nice to have a keywords based API, for when the line/file/function
> information is already split.  It would avoid having to build an explicit linespec
> string just to have GDB parse it after.
> 
> In terms of API, I think the "spec" argument could be mutually exclusive with
> the function/file/line/etc keywork arguments, which would be added.  An error
> would be thrown if you try to use both ways at the same time.
> 
> About the line="+3" issue, because this is Python, the line keyword could
> probably accept integers and strings.  And if it's a string, there could
> be some validation on the format.
> 

Simon,

Thanks for the review. For the record I have no objection to the
keywords API in addition to the spec line.

But I'm not sure what you mean about the line argument taking an
integer or a string. So line is a problem; it can be:

- line=3 (at line three in the source code)
- line=+3 (plus three lines from current source location)
- line=-3 (minus three lines from current source location)

I'm not sure how I could write a ParseTupleAndKeyword to accept that
in any form other than a string? The -3 will be a minus three, the +3
will just be a 3, and the =3 will be a 3 too. The problem is the
relative "+" information gets lost. Did you have something else in
mind? I guess I could use the O& in the format string to invoke a
converter function? I'm not quite sure what you intend though?

For now, though, I'll add the keywords (as strings) in. This really
prompts me to think we should rewrite the gdb.Breakpoint constructor
to not use create_breakpoint and be more MI-like in the creation of
breakpoints.

Cheers

Phil

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-10-16 04:24 PM, Phil Muldoon wrote:
> On 16/10/17 19:23, Simon Marchi wrote:
>> On 2017-08-23 02:30 PM, Phil Muldoon wrote:
>>> On 23/08/17 18:51, Keith Seitz wrote:
>>>> On 08/23/2017 06:58 AM, Phil Muldoon wrote:
> 
>> I think for Python it would make sense to support the two paradigms.  If you
>> are writing a Python command that ends up installing a breakpoint, it would
>> be nice if you could directly pass what you received to the gdb.Breakpoint
>> constructor and have it parse it (including explicit locations).  For example,
>>
>>   (gdb) special-break -file foo.c -line 17
>>
>> But it would also be nice to have a keywords based API, for when the line/file/function
>> information is already split.  It would avoid having to build an explicit linespec
>> string just to have GDB parse it after.
>>
>> In terms of API, I think the "spec" argument could be mutually exclusive with
>> the function/file/line/etc keywork arguments, which would be added.  An error
>> would be thrown if you try to use both ways at the same time.
>>
>> About the line="+3" issue, because this is Python, the line keyword could
>> probably accept integers and strings.  And if it's a string, there could
>> be some validation on the format.
>>
> 
> Simon,
> 
> Thanks for the review. For the record I have no objection to the
> keywords API in addition to the spec line.
> 
> But I'm not sure what you mean about the line argument taking an
> integer or a string. So line is a problem; it can be:
> 
> - line=3 (at line three in the source code)
> - line=+3 (plus three lines from current source location)
> - line=-3 (minus three lines from current source location)
> 
> I'm not sure how I could write a ParseTupleAndKeyword to accept that
> in any form other than a string? The -3 will be a minus three, the +3
> will just be a 3, and the =3 will be a 3 too. The problem is the
> relative "+" information gets lost. Did you have something else in
> mind? I guess I could use the O& in the format string to invoke a
> converter function? I'm not quite sure what you intend though?

I think we could support all of these:

  line=3
  line='3'
  line='+3'
  line='-3'

I was thinking about using the O modifier, to get a plain PyObject*, and
then check what type it really is (int or string).  But I didn't know
about O&, which looks like a good fit.  The converter function could
make use of linespec_parse_line_offset if the passed argument is a string.

> For now, though, I'll add the keywords (as strings) in. This really
> prompts me to think we should rewrite the gdb.Breakpoint constructor
> to not use create_breakpoint and be more MI-like in the creation of
> breakpoints.
I'm not sure what you mean, MI uses create_breakpoint in mi_cmd_break_insert_1.

Simon

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 16/10/17 22:25, Simon Marchi wrote:

> 
>> For now, though, I'll add the keywords (as strings) in. This really
>> prompts me to think we should rewrite the gdb.Breakpoint constructor
>> to not use create_breakpoint and be more MI-like in the creation of
>> breakpoints.
> I'm not sure what you mean, MI uses create_breakpoint in mi_cmd_break_insert_1.
> 
> Simon
> 
Simon,

My apologies, on reading back I see I was pretty vague. I meant to
create an explicit location using "new_explicit_location" function as
MI does in that function you mentioned instead of
"string_to_event_location". Keith mentioned it in the original email,
I think, and that "string_to_event_location" was designed expressly
for the command-line invocation. I wanted to see if Keith's comment
would work in a gdb.Breakpoint.  The downside is, if we do that (use
new_explicit_location), we won't be able to accept explicit locations
in the spec keyword and only via specific line, function, source-file,
etc keyword based instantiation.  I'll hack on the patch tomorrow and
try to decide which.  I'll repost the patch soon.

Cheers

Phil

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-10-16 06:01 PM, Phil Muldoon wrote:
> On 16/10/17 22:25, Simon Marchi wrote:
> 
>>
>>> For now, though, I'll add the keywords (as strings) in. This really
>>> prompts me to think we should rewrite the gdb.Breakpoint constructor
>>> to not use create_breakpoint and be more MI-like in the creation of
>>> breakpoints.
>> I'm not sure what you mean, MI uses create_breakpoint in mi_cmd_break_insert_1.
>>
>> Simon
>>
> Simon,
> 
> My apologies, on reading back I see I was pretty vague. I meant to
> create an explicit location using "new_explicit_location" function as
> MI does in that function you mentioned instead of
> "string_to_event_location". Keith mentioned it in the original email,
> I think, and that "string_to_event_location" was designed expressly
> for the command-line invocation. I wanted to see if Keith's comment
> would work in a gdb.Breakpoint.  The downside is, if we do that (use
> new_explicit_location), we won't be able to accept explicit locations
> in the spec keyword and only via specific line, function, source-file,
> etc keyword based instantiation.  I'll hack on the patch tomorrow and
> try to decide which.  I'll repost the patch soon.

But why can't we support both modes?

If "spec" is set, it's a CLI-like location, so you can feed it to
string_to_event_location.

If the keywords line/function/file are set (some of them), use them
with new_explicit_location.

If both spec and line/function/file are used, throw an error.

Simon

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 16/10/17 23:25, Simon Marchi wrote:

> But why can't we support both modes?
> 
> If "spec" is set, it's a CLI-like location, so you can feed it to
> string_to_event_location.
> 
> If the keywords line/function/file are set (some of them), use them
> with new_explicit_location.
> 
> If both spec and line/function/file are used, throw an error.
> 
> Simon

All,

Here's an updated patch. ChangeLog remains the same (except for
additional NEWS entry)

I just realised this needs a doc review also.

Cheers

Phil

diff --git a/gdb/NEWS b/gdb/NEWS
index 9246659bfb..592fe70156 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -24,6 +24,9 @@
      gdb.new_thread are emitted.  See the manual for further
      description of these.
 
+  ** Python breakpoints can now accept explicit locations.  See the
+     manual for a further description of this feature.
+
 * New features in the GDB remote stub, GDBserver
 
   ** New "--selftest" command line option runs some GDBserver self
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index f661e489bb..25cee3f93e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4861,27 +4861,30 @@ represented as Python @code{Long} values.
 Python code can manipulate breakpoints via the @code{gdb.Breakpoint}
 class.
 
-@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[},internal @r{[},temporary@r{]]]]})
+@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[}, internal @r{[}, temporary @r{]}, source @r{]}, function @r{]}, label @r{]}, line @r{]]]]]]]]})
 Create a new breakpoint according to @var{spec}, which is a string
 naming the location of the breakpoint, or an expression that defines a
-watchpoint.  The contents can be any location recognized by the
-@code{break} command, or in the case of a watchpoint, by the
-@code{watch} command.  The optional @var{type} denotes the breakpoint
-to create from the types defined later in this chapter.  This argument
-can be either @code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}; it
-defaults to @code{gdb.BP_BREAKPOINT}.  The optional @var{internal}
-argument allows the breakpoint to become invisible to the user.  The
-breakpoint will neither be reported when created, nor will it be
-listed in the output from @code{info breakpoints} (but will be listed
-with the @code{maint info breakpoints} command).  The optional
-@var{temporary} argument makes the breakpoint a temporary breakpoint.
-Temporary breakpoints are deleted after they have been hit.  Any
-further access to the Python breakpoint after it has been hit will
-result in a runtime error (as that breakpoint has now been
-automatically deleted).  The optional @var{wp_class} argument defines
-the class of watchpoint to create, if @var{type} is
-@code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not provided, it
-is assumed to be a @code{gdb.WP_WRITE} class.
+watchpoint. The contents can be any location recognized by the
+@code{break} command or, in the case of a watchpoint, by the
+@code{watch} command.  Alternatively, create a new a explicit location
+breakpoint (@pxref{Explicit Locations}) according to the
+specifications contained in the key words @var{source},
+@var{function}, @var{label} and @var{line}.  The optional @var{type}
+denotes the breakpoint to create from the types defined later in this
+chapter.  This argument can be either @code{gdb.BP_BREAKPOINT} or
+@code{gdb.BP_WATCHPOINT}; it defaults to @code{gdb.BP_BREAKPOINT}.
+The optional @var{internal} argument allows the breakpoint to become
+invisible to the user.  The breakpoint will neither be reported when
+created, nor will it be listed in the output from @code{info
+breakpoints} (but will be listed with the @code{maint info
+breakpoints} command).  The optional @var{temporary} argument makes
+the breakpoint a temporary breakpoint.  Temporary breakpoints are
+deleted after they have been hit.  Any further access to the Python
+breakpoint after it has been hit will result in a runtime error (as
+that breakpoint has now been automatically deleted).  The optional
+@var{wp_class} argument defines the class of watchpoint to create, if
+@var{type} is @code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not
+provided, it is assumed to be a @code{gdb.WP_WRITE} class.
 @end defun
 
 The available types are represented by constants defined in the @code{gdb}
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index d57c2fa05e..85d514f57a 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -32,6 +32,7 @@
 #include "language.h"
 #include "location.h"
 #include "py-event.h"
+#include "linespec.h"
 
 /* Number of live breakpoints.  */
 static int bppy_live;
@@ -638,20 +639,73 @@ static int
 bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
   static const char *keywords[] = { "spec", "type", "wp_class", "internal",
-				    "temporary", NULL };
-  const char *spec;
+				    "temporary","source", "function",
+				    "label", "line", NULL };
+  const char *spec = NULL;
   int type = bp_breakpoint;
   int access_type = hw_write;
   PyObject *internal = NULL;
   PyObject *temporary = NULL;
   int internal_bp = 0;
   int temporary_bp = 0;
+  char *line = NULL;
+  char *label = NULL;
+  char *source = NULL;
+  char *function = NULL;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiOO", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOssss", keywords,
 					&spec, &type, &access_type,
-					&internal, &temporary))
+					&internal,
+					&temporary, &source,
+					&function, &label, &line))
     return -1;
 
+  /* If spec is defined, ensure that none of the explicit location
+     keywords are also defined.  */
+  if (spec != NULL)
+    {
+      if (source != NULL || function != NULL || label != NULL || line != NULL)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Breakpoints specified with spec cannot "
+			     "have source, function, label or line defined."));
+	  return -1;
+	}
+    }
+  else
+    {
+      /* If spec isn't defined, ensure that the user is not trying to
+	 define a watchpoint with an explicit location.  */
+      if (type == bp_watchpoint)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Watchpoints cannot be set by explicit "
+			     "location parameters."));
+	  return -1;
+	}
+      else
+	{
+	  /* Otherwise, ensure some explicit locations are defined.  */
+	  if (source == NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Neither spec nor explicit location set."));
+	      return -1;
+	    }
+	  /* Finally, if source is specified, ensure that line, label
+	     or function are specified too.  */
+	  if (source != NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Specifying a source must also include a "
+				 "line, label or function."));
+	      return -1;
+	    }
+	}
+    }
+
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
@@ -672,16 +726,37 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 
   TRY
     {
-      gdb::unique_xmalloc_ptr<char>
-	copy_holder (xstrdup (skip_spaces (spec)));
-      char *copy = copy_holder.get ();
-
       switch (type)
 	{
 	case bp_breakpoint:
 	  {
-	    event_location_up location
-	      = string_to_event_location_basic (&copy, current_language);
+	    event_location_up location;
+
+	    if (spec != NULL)
+	      {
+		gdb::unique_xmalloc_ptr<char>
+		  copy_holder (xstrdup (skip_spaces (spec)));
+		char *copy = copy_holder.get ();
+
+		location  = string_to_event_location (&copy,
+						      current_language);
+	      }
+	    else
+	      {
+		struct explicit_location explicit_loc;
+
+		initialize_explicit_location (&explicit_loc);
+		explicit_loc.source_filename = source;
+		explicit_loc.function_name = function;
+		explicit_loc.label_name = label;
+
+		if (line != NULL)
+		  explicit_loc.line_offset =
+		    linespec_parse_line_offset (line);
+
+		location = new_explicit_location (&explicit_loc);
+	      }
+
 	    create_breakpoint (python_gdbarch,
 			       location.get (), NULL, -1, NULL,
 			       0,
@@ -692,8 +767,12 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 			       0, 1, internal_bp, 0);
 	    break;
 	  }
-        case bp_watchpoint:
+	case bp_watchpoint:
 	  {
+	    gdb::unique_xmalloc_ptr<char>
+	      copy_holder (xstrdup (skip_spaces (spec)));
+	    char *copy = copy_holder.get ();
+
 	    if (access_type == hw_write)
 	      watch_command_wrapper (copy, 0, internal_bp);
 	    else if (access_type == hw_access)
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
index df6163e53a..562cab35f7 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.c
+++ b/gdb/testsuite/gdb.python/py-breakpoint.c
@@ -24,7 +24,7 @@ int multiply (int i)
 
 int add (int i)
 {
-  return i + i; 
+  return i + i;  /* Break at function add.  */
 }
 
 
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index bd138ac3d2..d91e5deb50 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -547,6 +547,74 @@ proc test_bkpt_events {} {
     check_last_event breakpoint_deleted
 }
 
+proc test_bkpt_explicit_loc {} {
+    global srcfile testfile
+
+    with_test_prefix test_bkpt_invisible {
+	# Start with a fresh gdb.
+	clean_restart ${testfile}
+
+	if ![runto_main] then {
+	    fail "cannot run to main."
+	    return 0
+	}
+
+	delete_breakpoints
+
+	set bp_location1 [gdb_get_line_number "Break at multiply."]
+	set bp_location2 [gdb_get_line_number "Break at add."]
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"$bp_location1\")" \
+	    "set explicit breakpoint by line" 0
+	gdb_continue_to_breakpoint "Break at multiply" \
+	    ".*Break at multiply.*"
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
+	    "set explicit breakpoint by relative line" 0
+	gdb_continue_to_breakpoint "Break at add" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
+	    "set explicit breakpoint by relative negative line" 0
+	gdb_continue_to_breakpoint "Break at multiply" \
+	    ".*Break at multiply.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
+	    "set explicit breakpoint by function" 0
+	gdb_continue_to_breakpoint "Break at function add" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
+	    "set explicit breakpoint by source file and function" 0
+	gdb_continue_to_breakpoint "Break at function add" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
+	    "set explicit breakpoint by source file and line number" 0
+	gdb_continue_to_breakpoint "Break at add" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
+	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
+	    "set invalid explicit breakpoint by source only"
+
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
+	    "No source file named foo.*" \
+	    "set invalid explicit breakpoint by missing source and line"
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
+	    "No line 900 in file \"$srcfile\".*" \
+	    "set invalid explicit breakpoint by source and invalid line"
+	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
+	    "Function \"blah\" not defined.*" \
+	    "set invalid explicit breakpoint by missing function"
+    }
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -558,3 +626,4 @@ test_bkpt_temporary
 test_bkpt_address
 test_bkpt_pending
 test_bkpt_events
+test_bkpt_explicit_loc

Re: [python] Allow explicit locations in breakpoints.

[Eli Zaretskii]

> From: Phil Muldoon <pmuldoon@redhat.com>
> Date: Fri, 17 Nov 2017 11:02:08 +0000
> 
> I just realised this needs a doc review also.

Doc review coming up.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 9246659bfb..592fe70156 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -24,6 +24,9 @@
>       gdb.new_thread are emitted.  See the manual for further
>       description of these.
>  
> +  ** Python breakpoints can now accept explicit locations.  See the
> +     manual for a further description of this feature.

I think "a further" should lose the "a" part.  Also, how about
mentioning the node name in the manual where this is described?

> +watchpoint. The contents can be any location recognized by the
             ^^
Two spaces here, please.

> +@code{break} command or, in the case of a watchpoint, by the
> +@code{watch} command.  Alternatively, create a new a explicit location
                                         ^^^^^^^^^^^^^^^^^^^^^^^
The second "a" should be deleted.

> +@var{function}, @var{label} and @var{line}.  The optional @var{type}
> +denotes the breakpoint to create from the types defined later in this
> +chapter.  This argument can be either @code{gdb.BP_BREAKPOINT} or

I think "denotes the breakpoint to create from the types" is at least
confusing.  What did you mean here?

> +@code{gdb.BP_WATCHPOINT}; it defaults to @code{gdb.BP_BREAKPOINT}.
> +The optional @var{internal} argument allows the breakpoint to become
> +invisible to the user.  The breakpoint will neither be reported when
                        ^
I think you want a colon here, not a period, because the next sentence
explains what does it mean for a breakpoint to be "invisible".
Alternatively, start the next sentence with "Invisible breakpoints
will neither be reported ...".

> +created, nor will it be listed in the output from @code{info
> +breakpoints} (but will be listed with the @code{maint info
                                    ^^^^
"by", I think, not "with".

> +breakpoints} command).  The optional @var{temporary} argument makes
> +the breakpoint a temporary breakpoint.  Temporary breakpoints are
> +deleted after they have been hit.  Any further access to the Python
> +breakpoint after it has been hit will result in a runtime error (as

I think you want to say "to the temporary breakpoint" here.  The fact
that it is a Python breakpoint is not really relevant.

> +that breakpoint has now been automatically deleted).  The optional
> +@var{wp_class} argument defines the class of watchpoint to create, if
> +@var{type} is @code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not
> +provided, it is assumed to be a @code{gdb.WP_WRITE} class.

What are the other supported classes?  Either state that here or point
to where that is described.

Thanks.

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 17/11/17 13:30, Eli Zaretskii wrote:
>> From: Phil Muldoon <pmuldoon@redhat.com>
>> Date: Fri, 17 Nov 2017 11:02:08 +0000
>>
>> I just realised this needs a doc review also.
> 
> Doc review coming up.
> 
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index 9246659bfb..592fe70156 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -24,6 +24,9 @@
>>       gdb.new_thread are emitted.  See the manual for further
>>       description of these.
>>  
>> +  ** Python breakpoints can now accept explicit locations.  See the
>> +     manual for a further description of this feature.
> 
> I think "a further" should lose the "a" part.  Also, how about
> mentioning the node name in the manual where this is described?

OK.

>> +@code{break} command or, in the case of a watchpoint, by the
>> +@code{watch} command.  Alternatively, create a new a explicit location
>                                          ^^^^^^^^^^^^^^^^^^^^^^^
> The second "a" should be deleted.

OK. The other comments refer to existing documentation that I have not
actually changed. I formatted the paragraph with the additional
sentence I added and re-formatted (the wrapping affected the whole
paragraph). I have no problem making the changes you suggest, they are
good, but can I please make them in another patchset so the changes
made in this one stay related?

For the purposes of clarity, the related changes in this patch are:

+@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[}, internal @r{[}, temporary @r{]}, source @r{]}, function @r{]}, label @r{]}, line @r{]]]]]]]]})

and

+Alternatively, create a new a explicit location
+breakpoint (@pxref{Explicit Locations}) according to the
+specifications contained in the key words @var{source},
+@var{function}, @var{label} and @var{line}. 

Cheers

Phil

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-11-17 06:02 AM, Phil Muldoon wrote:
> All,
> 
> Here's an updated patch. ChangeLog remains the same (except for
> additional NEWS entry)
> 
> I just realised this needs a doc review also.
> 
> Cheers
> 
> Phil

Hi Phil,

That

> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -32,6 +32,7 @@
>  #include "language.h"
>  #include "location.h"
>  #include "py-event.h"
> +#include "linespec.h"
>  
>  /* Number of live breakpoints.  */
>  static int bppy_live;
> @@ -638,20 +639,73 @@ static int
>  bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  {
>    static const char *keywords[] = { "spec", "type", "wp_class", "internal",
> -				    "temporary", NULL };
> -  const char *spec;
> +				    "temporary","source", "function",
> +				    "label", "line", NULL };
> +  const char *spec = NULL;
>    int type = bp_breakpoint;
>    int access_type = hw_write;
>    PyObject *internal = NULL;
>    PyObject *temporary = NULL;
>    int internal_bp = 0;
>    int temporary_bp = 0;
> +  char *line = NULL;
> +  char *label = NULL;
> +  char *source = NULL;
> +  char *function = NULL;
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiOO", keywords,
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOssss", keywords,
>  					&spec, &type, &access_type,
> -					&internal, &temporary))
> +					&internal,
> +					&temporary, &source,
> +					&function, &label, &line))
>      return -1;
>  
> +  /* If spec is defined, ensure that none of the explicit location
> +     keywords are also defined.  */
> +  if (spec != NULL)
> +    {
> +      if (source != NULL || function != NULL || label != NULL || line != NULL)
> +	{
> +	  PyErr_SetString (PyExc_RuntimeError,
> +			   _("Breakpoints specified with spec cannot "
> +			     "have source, function, label or line defined."));
> +	  return -1;
> +	}
> +    }
> +  else
> +    {
> +      /* If spec isn't defined, ensure that the user is not trying to
> +	 define a watchpoint with an explicit location.  */
> +      if (type == bp_watchpoint)
> +	{
> +	  PyErr_SetString (PyExc_RuntimeError,
> +			   _("Watchpoints cannot be set by explicit "
> +			     "location parameters."));
> +	  return -1;
> +	}
> +      else
> +	{
> +	  /* Otherwise, ensure some explicit locations are defined.  */
> +	  if (source == NULL && function == NULL && label == NULL
> +	      && line == NULL)
> +	    {
> +	      PyErr_SetString (PyExc_RuntimeError,
> +			       _("Neither spec nor explicit location set."));
> +	      return -1;
> +	    }
> +	  /* Finally, if source is specified, ensure that line, label
> +	     or function are specified too.  */
> +	  if (source != NULL && function == NULL && label == NULL
> +	      && line == NULL)
> +	    {
> +	      PyErr_SetString (PyExc_RuntimeError,
> +			       _("Specifying a source must also include a "
> +				 "line, label or function."));
> +	      return -1;
> +	    }
> +	}
> +    }
> +

I would suggest putting the argument validation code in its own function (e.g.
bppy_init_validate_args), to keep bppy_init of a reasonnable size.

>    if (internal)
>      {
>        internal_bp = PyObject_IsTrue (internal);
> @@ -672,16 +726,37 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  
>    TRY
>      {
> -      gdb::unique_xmalloc_ptr<char>
> -	copy_holder (xstrdup (skip_spaces (spec)));
> -      char *copy = copy_holder.get ();
> -
>        switch (type)
>  	{
>  	case bp_breakpoint:
>  	  {
> -	    event_location_up location
> -	      = string_to_event_location_basic (&copy, current_language);
> +	    event_location_up location;
> +
> +	    if (spec != NULL)
> +	      {
> +		gdb::unique_xmalloc_ptr<char>
> +		  copy_holder (xstrdup (skip_spaces (spec)));
> +		char *copy = copy_holder.get ();
> +
> +		location  = string_to_event_location (&copy,
> +						      current_language);
> +	      }
> +	    else
> +	      {
> +		struct explicit_location explicit_loc;
> +
> +		initialize_explicit_location (&explicit_loc);
> +		explicit_loc.source_filename = source;
> +		explicit_loc.function_name = function;
> +		explicit_loc.label_name = label;
> +
> +		if (line != NULL)
> +		  explicit_loc.line_offset =
> +		    linespec_parse_line_offset (line);

For convenience, I think it would be nice to accept integers for line.  So perhaps
something like this:

From d0e3122bca8b4fcb2d6f303bdc66f68929bdc14f Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 23 Nov 2017 16:58:35 -0500
Subject: [PATCH] suggestion

---
 gdb/python/py-breakpoint.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 7fb35ce..5ee0fed 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -648,23 +648,23 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   PyObject *temporary = NULL;
   int internal_bp = 0;
   int temporary_bp = 0;
-  char *line = NULL;
+  PyObject *lineobj = NULL;
   char *label = NULL;
   char *source = NULL;
   char *function = NULL;

-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOssss", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssO", keywords,
 					&spec, &type, &access_type,
 					&internal,
 					&temporary, &source,
-					&function, &label, &line))
+					&function, &label, &lineobj))
     return -1;

   /* If spec is defined, ensure that none of the explicit location
      keywords are also defined.  */
   if (spec != NULL)
     {
-      if (source != NULL || function != NULL || label != NULL || line != NULL)
+      if (source != NULL || function != NULL || label != NULL || lineobj != NULL)
 	{
 	  PyErr_SetString (PyExc_RuntimeError,
 			   _("Breakpoints specified with spec cannot "
@@ -687,7 +687,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	{
 	  /* Otherwise, ensure some explicit locations are defined.  */
 	  if (source == NULL && function == NULL && label == NULL
-	      && line == NULL)
+	      && lineobj == NULL)
 	    {
 	      PyErr_SetString (PyExc_RuntimeError,
 			       _("Neither spec nor explicit location set."));
@@ -696,7 +696,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	  /* Finally, if source is specified, ensure that line, label
 	     or function are specified too.  */
 	  if (source != NULL && function == NULL && label == NULL
-	      && line == NULL)
+	      && lineobj == NULL)
 	    {
 	      PyErr_SetString (PyExc_RuntimeError,
 			       _("Specifying a source must also include a "
@@ -706,6 +706,13 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	}
     }

+  if (lineobj != NULL && !PyInt_Check (lineobj) && !PyString_Check (lineobj))
+    {
+      PyErr_SetString (PyExc_TypeError,
+		       _ ("line must be an integer or a string."));
+      return -1;
+    }
+
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
@@ -750,9 +757,23 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 		explicit_loc.function_name = function;
 		explicit_loc.label_name = label;

-		if (line != NULL)
-		  explicit_loc.line_offset =
-		    linespec_parse_line_offset (line);
+		if (lineobj != NULL)
+		  {
+		    gdb::unique_xmalloc_ptr<char> line;
+
+		    if (PyInt_Check (lineobj))
+		      line.reset (xstrprintf ("%ld", PyInt_AsLong (lineobj)));
+		    else if (PyString_Check (lineobj))
+		      line = python_string_to_host_string (lineobj);
+		    else
+		      {
+			/* We should have validated the type up there... */
+			gdb_assert_not_reached ("Unexpected type of lineobj.");
+		      }
+
+		    explicit_loc.line_offset
+		      = linespec_parse_line_offset (line.get ());
+		  }

 		location = new_explicit_location (&explicit_loc);
 	      }
-- 
2.7.4


> +
> +		location = new_explicit_location (&explicit_loc);
> +	      }
> +
>  	    create_breakpoint (python_gdbarch,
>  			       location.get (), NULL, -1, NULL,
>  			       0,
> @@ -692,8 +767,12 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>  			       0, 1, internal_bp, 0);
>  	    break;
>  	  }
> -        case bp_watchpoint:
> +	case bp_watchpoint:
>  	  {
> +	    gdb::unique_xmalloc_ptr<char>
> +	      copy_holder (xstrdup (skip_spaces (spec)));
> +	    char *copy = copy_holder.get ();
> +
>  	    if (access_type == hw_write)
>  	      watch_command_wrapper (copy, 0, internal_bp);
>  	    else if (access_type == hw_access)
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
> index df6163e53a..562cab35f7 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.c
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.c
> @@ -24,7 +24,7 @@ int multiply (int i)
>  
>  int add (int i)
>  {
> -  return i + i; 
> +  return i + i;  /* Break at function add.  */
>  }
>  
>  
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index bd138ac3d2..d91e5deb50 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -547,6 +547,74 @@ proc test_bkpt_events {} {
>      check_last_event breakpoint_deleted
>  }
>  
> +proc test_bkpt_explicit_loc {} {
> +    global srcfile testfile
> +
> +    with_test_prefix test_bkpt_invisible {
> +	# Start with a fresh gdb.
> +	clean_restart ${testfile}
> +
> +	if ![runto_main] then {
> +	    fail "cannot run to main."
> +	    return 0
> +	}
> +
> +	delete_breakpoints
> +
> +	set bp_location1 [gdb_get_line_number "Break at multiply."]
> +	set bp_location2 [gdb_get_line_number "Break at add."]
> +
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"$bp_location1\")" \
> +	    "set explicit breakpoint by line" 0
> +	gdb_continue_to_breakpoint "Break at multiply" \
> +	    ".*Break at multiply.*"
> +
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
> +	    "set explicit breakpoint by relative line" 0
> +	gdb_continue_to_breakpoint "Break at add" \
> +	    ".*Break at add.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
> +	    "set explicit breakpoint by relative negative line" 0
> +	gdb_continue_to_breakpoint "Break at multiply" \
> +	    ".*Break at multiply.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
> +	    "set explicit breakpoint by function" 0
> +	gdb_continue_to_breakpoint "Break at function add" \
> +	    ".*Break at function add.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
> +	    "set explicit breakpoint by source file and function" 0
> +	gdb_continue_to_breakpoint "Break at function add" \
> +	    ".*Break at function add.*"
> +
> +	delete_breakpoints
> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
> +	    "set explicit breakpoint by source file and line number" 0
> +	gdb_continue_to_breakpoint "Break at add" \
> +	    ".*Break at add.*"
> +
> +	delete_breakpoints
> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
> +	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
> +	    "set invalid explicit breakpoint by source only"
> +
> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
> +	    "No source file named foo.*" \
> +	    "set invalid explicit breakpoint by missing source and line"
> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
> +	    "No line 900 in file \"$srcfile\".*" \
> +	    "set invalid explicit breakpoint by source and invalid line"
> +	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
> +	    "Function \"blah\" not defined.*" \
> +	    "set invalid explicit breakpoint by missing function"
> +    }

The test names should start with a lower case letter:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention

Also make sure that test names are not duplicated.  You can use this command to quickly
find duplicated names in gdb.sum:

$ cat testsuite/gdb.sum | grep -e PASS -e FAIL | sort | uniq -c | sort -n

Would it be good to test that explicit breakpoint locations through the spec
argument work as well?  Things like

  gdb.Breakpoint('-line 5')

?

Thanks,

Simon

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 23/11/17 22:17, Simon Marchi wrote:
> On 2017-11-17 06:02 AM, Phil Muldoon wrote:

> 
> I would suggest putting the argument validation code in its own function (e.g.
> bppy_init_validate_args), to keep bppy_init of a reasonnable size.

Okay, will do.
 
> For convenience, I think it would be nice to accept integers for line.  So perhaps
> something like this:
> 
> From d0e3122bca8b4fcb2d6f303bdc66f68929bdc14f Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Thu, 23 Nov 2017 16:58:35 -0500
> Subject: [PATCH] suggestion

Yeah good idea. I remember we talked about this, but I must have
forgotten about it. I'll add your patch in.

>> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
>> index bd138ac3d2..d91e5deb50 100644
>> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
>> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
>> @@ -547,6 +547,74 @@ proc test_bkpt_events {} {
>>      check_last_event breakpoint_deleted
>>  }
>>  
>> +proc test_bkpt_explicit_loc {} {
>> +    global srcfile testfile
>> +
>> +    with_test_prefix test_bkpt_invisible {
>> +	# Start with a fresh gdb.
>> +	clean_restart ${testfile}
>> +
>> +	if ![runto_main] then {
>> +	    fail "cannot run to main."
>> +	    return 0
>> +	}
>> +
>> +	delete_breakpoints
>> +
>> +	set bp_location1 [gdb_get_line_number "Break at multiply."]
>> +	set bp_location2 [gdb_get_line_number "Break at add."]
>> +
>> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"$bp_location1\")" \
>> +	    "set explicit breakpoint by line" 0
>> +	gdb_continue_to_breakpoint "Break at multiply" \
>> +	    ".*Break at multiply.*"
>> +
>> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
>> +	    "set explicit breakpoint by relative line" 0
>> +	gdb_continue_to_breakpoint "Break at add" \
>> +	    ".*Break at add.*"
>> +
>> +	delete_breakpoints
>> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
>> +	    "set explicit breakpoint by relative negative line" 0
>> +	gdb_continue_to_breakpoint "Break at multiply" \
>> +	    ".*Break at multiply.*"
>> +
>> +	delete_breakpoints
>> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
>> +	    "set explicit breakpoint by function" 0
>> +	gdb_continue_to_breakpoint "Break at function add" \
>> +	    ".*Break at function add.*"
>> +
>> +	delete_breakpoints
>> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
>> +	    "set explicit breakpoint by source file and function" 0
>> +	gdb_continue_to_breakpoint "Break at function add" \
>> +	    ".*Break at function add.*"
>> +
>> +	delete_breakpoints
>> +	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
>> +	    "set explicit breakpoint by source file and line number" 0
>> +	gdb_continue_to_breakpoint "Break at add" \
>> +	    ".*Break at add.*"
>> +
>> +	delete_breakpoints
>> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
>> +	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
>> +	    "set invalid explicit breakpoint by source only"
>> +
>> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
>> +	    "No source file named foo.*" \
>> +	    "set invalid explicit breakpoint by missing source and line"
>> +	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
>> +	    "No line 900 in file \"$srcfile\".*" \
>> +	    "set invalid explicit breakpoint by source and invalid line"
>> +	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
>> +	    "Function \"blah\" not defined.*" \
>> +	    "set invalid explicit breakpoint by missing function"
>> +    }
> 
> The test names should start with a lower case letter:
> 
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Follow_the_test_name_convention
> 
> Also make sure that test names are not duplicated.  You can use this command to quickly
> find duplicated names in gdb.sum:
> 
> $ cat testsuite/gdb.sum | grep -e PASS -e FAIL | sort | uniq -c | sort -n

I thought they all were. Regardless, I'll check again before resubmitting.
 
> Would it be good to test that explicit breakpoint locations through the spec
> argument work as well?  Things like
> 
>   gdb.Breakpoint('-line 5')

Yeah, good idea. They all originally were but I converted them to the
keywords without thinking of the spec case

Cheers

Phil

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

On 23/11/17 22:17, Simon Marchi wrote:
> On 2017-11-17 06:02 AM, Phil Muldoon wrote:
>> All,
>>
>> Here's an updated patch. ChangeLog remains the same (except for
>> additional NEWS entry)
>>
>> I just realised this needs a doc review also.
>>
>> Cheers
>>
>> Phil
> 
> Hi Phil,

Here's a new patch. I think I've addressed all of the issues.

Cheers

Phil

--

diff --git a/gdb/NEWS b/gdb/NEWS
index 9246659bfb..592fe70156 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -24,6 +24,9 @@
      gdb.new_thread are emitted.  See the manual for further
      description of these.
 
+  ** Python breakpoints can now accept explicit locations.  See the
+     manual for a further description of this feature.
+
 * New features in the GDB remote stub, GDBserver
 
   ** New "--selftest" command line option runs some GDBserver self
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index f661e489bb..25cee3f93e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4861,27 +4861,30 @@ represented as Python @code{Long} values.
 Python code can manipulate breakpoints via the @code{gdb.Breakpoint}
 class.
 
-@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[},internal @r{[},temporary@r{]]]]})
+@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[}, internal @r{[}, temporary @r{]}, source @r{]}, function @r{]}, label @r{]}, line @r{]]]]]]]]})
 Create a new breakpoint according to @var{spec}, which is a string
 naming the location of the breakpoint, or an expression that defines a
-watchpoint.  The contents can be any location recognized by the
-@code{break} command, or in the case of a watchpoint, by the
-@code{watch} command.  The optional @var{type} denotes the breakpoint
-to create from the types defined later in this chapter.  This argument
-can be either @code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}; it
-defaults to @code{gdb.BP_BREAKPOINT}.  The optional @var{internal}
-argument allows the breakpoint to become invisible to the user.  The
-breakpoint will neither be reported when created, nor will it be
-listed in the output from @code{info breakpoints} (but will be listed
-with the @code{maint info breakpoints} command).  The optional
-@var{temporary} argument makes the breakpoint a temporary breakpoint.
-Temporary breakpoints are deleted after they have been hit.  Any
-further access to the Python breakpoint after it has been hit will
-result in a runtime error (as that breakpoint has now been
-automatically deleted).  The optional @var{wp_class} argument defines
-the class of watchpoint to create, if @var{type} is
-@code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not provided, it
-is assumed to be a @code{gdb.WP_WRITE} class.
+watchpoint. The contents can be any location recognized by the
+@code{break} command or, in the case of a watchpoint, by the
+@code{watch} command.  Alternatively, create a new a explicit location
+breakpoint (@pxref{Explicit Locations}) according to the
+specifications contained in the key words @var{source},
+@var{function}, @var{label} and @var{line}.  The optional @var{type}
+denotes the breakpoint to create from the types defined later in this
+chapter.  This argument can be either @code{gdb.BP_BREAKPOINT} or
+@code{gdb.BP_WATCHPOINT}; it defaults to @code{gdb.BP_BREAKPOINT}.
+The optional @var{internal} argument allows the breakpoint to become
+invisible to the user.  The breakpoint will neither be reported when
+created, nor will it be listed in the output from @code{info
+breakpoints} (but will be listed with the @code{maint info
+breakpoints} command).  The optional @var{temporary} argument makes
+the breakpoint a temporary breakpoint.  Temporary breakpoints are
+deleted after they have been hit.  Any further access to the Python
+breakpoint after it has been hit will result in a runtime error (as
+that breakpoint has now been automatically deleted).  The optional
+@var{wp_class} argument defines the class of watchpoint to create, if
+@var{type} is @code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not
+provided, it is assumed to be a @code{gdb.WP_WRITE} class.
 @end defun
 
 The available types are represented by constants defined in the @code{gdb}
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index d57c2fa05e..417f02d612 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -32,6 +32,7 @@
 #include "language.h"
 #include "location.h"
 #include "py-event.h"
+#include "linespec.h"
 
 /* Number of live breakpoints.  */
 static int bppy_live;
@@ -633,25 +634,103 @@ bppy_get_ignore_count (PyObject *self, void *closure)
   return PyInt_FromLong (self_bp->bp->ignore_count);
 }
 
+/* Internal function to validate the Python parameters/keywords
+   provided to bppy_init.  */
+
+static int
+bppy_init_validate_args (const char *spec, char *source,
+			 char *function, char *label,
+			 char *line, enum bptype type)
+{
+  /* If spec is defined, ensure that none of the explicit location
+     keywords are also defined.  */
+  if (spec != NULL)
+    {
+      if (source != NULL || function != NULL || label != NULL || line != NULL)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Breakpoints specified with spec cannot "
+			     "have source, function, label or line defined."));
+	  return -1;
+	}
+    }
+  else
+    {
+      /* If spec isn't defined, ensure that the user is not trying to
+	 define a watchpoint with an explicit location.  */
+      if (type == bp_watchpoint)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Watchpoints cannot be set by explicit "
+			     "location parameters."));
+	  return -1;
+	}
+      else
+	{
+	  /* Otherwise, ensure some explicit locations are defined.  */
+	  if (source == NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Neither spec nor explicit location set."));
+	      return -1;
+	    }
+	  /* Finally, if source is specified, ensure that line, label
+	     or function are specified too.  */
+	  if (source != NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Specifying a source must also include a "
+				 "line, label or function."));
+	      return -1;
+	    }
+	}
+    }
+  return 1;
+}
+
 /* Python function to create a new breakpoint.  */
 static int
 bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
   static const char *keywords[] = { "spec", "type", "wp_class", "internal",
-				    "temporary", NULL };
-  const char *spec;
-  int type = bp_breakpoint;
+				    "temporary","source", "function",
+				    "label", "line", NULL };
+  const char *spec = NULL;
+  enum bptype type = bp_breakpoint;
   int access_type = hw_write;
   PyObject *internal = NULL;
   PyObject *temporary = NULL;
+  PyObject *lineobj = NULL;;
   int internal_bp = 0;
   int temporary_bp = 0;
+  gdb::unique_xmalloc_ptr<char> line;
+  char *label = NULL;
+  char *source = NULL;
+  char *function = NULL;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiOO", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssO", keywords,
 					&spec, &type, &access_type,
-					&internal, &temporary))
+					&internal,
+					&temporary, &source,
+					&function, &label, &lineobj))
     return -1;
 
+
+  if (lineobj != NULL)
+    {
+      if (PyInt_Check (lineobj))
+	line.reset (xstrprintf ("%ld", PyInt_AsLong (lineobj)));
+      else if (PyString_Check (lineobj))
+	line = python_string_to_host_string (lineobj);
+      else
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Line keyword should be an integer or a string. "));
+	  return -1;
+	}
+    }
+
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
@@ -666,22 +745,47 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	return -1;
     }
 
+  if (bppy_init_validate_args (spec, source, function, label, line.get (),
+			       type) == -1)
+    return -1;
+
   bppy_pending_object = (gdbpy_breakpoint_object *) self;
   bppy_pending_object->number = -1;
   bppy_pending_object->bp = NULL;
 
   TRY
     {
-      gdb::unique_xmalloc_ptr<char>
-	copy_holder (xstrdup (skip_spaces (spec)));
-      char *copy = copy_holder.get ();
-
       switch (type)
 	{
 	case bp_breakpoint:
 	  {
-	    event_location_up location
-	      = string_to_event_location_basic (&copy, current_language);
+	    event_location_up location;
+
+	    if (spec != NULL)
+	      {
+		gdb::unique_xmalloc_ptr<char>
+		  copy_holder (xstrdup (skip_spaces (spec)));
+		char *copy = copy_holder.get ();
+
+		location  = string_to_event_location (&copy,
+						      current_language);
+	      }
+	    else
+	      {
+		struct explicit_location explicit_loc;
+
+		initialize_explicit_location (&explicit_loc);
+		explicit_loc.source_filename = source;
+		explicit_loc.function_name = function;
+		explicit_loc.label_name = label;
+
+		if (line != NULL)
+		  explicit_loc.line_offset =
+		    linespec_parse_line_offset (line.get ());
+
+		location = new_explicit_location (&explicit_loc);
+	      }
+
 	    create_breakpoint (python_gdbarch,
 			       location.get (), NULL, -1, NULL,
 			       0,
@@ -692,8 +796,12 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 			       0, 1, internal_bp, 0);
 	    break;
 	  }
-        case bp_watchpoint:
+	case bp_watchpoint:
 	  {
+	    gdb::unique_xmalloc_ptr<char>
+	      copy_holder (xstrdup (skip_spaces (spec)));
+	    char *copy = copy_holder.get ();
+
 	    if (access_type == hw_write)
 	      watch_command_wrapper (copy, 0, internal_bp);
 	    else if (access_type == hw_access)
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
index df6163e53a..562cab35f7 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.c
+++ b/gdb/testsuite/gdb.python/py-breakpoint.c
@@ -24,7 +24,7 @@ int multiply (int i)
 
 int add (int i)
 {
-  return i + i; 
+  return i + i;  /* Break at function add.  */
 }
 
 
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index bd138ac3d2..2eca7f366a 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -547,6 +547,85 @@ proc test_bkpt_events {} {
     check_last_event breakpoint_deleted
 }
 
+proc test_bkpt_explicit_loc {} {
+    global srcfile testfile
+
+    with_test_prefix test_bkpt_invisible {
+	# Start with a fresh gdb.
+	clean_restart ${testfile}
+
+	if ![runto_main] then {
+	    fail "cannot run to main."
+	    return 0
+	}
+
+	delete_breakpoints
+
+	set bp_location1 [gdb_get_line_number "Break at multiply."]
+	set bp_location2 [gdb_get_line_number "Break at add."]
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=$bp_location1)" \
+	    "set explicit breakpoint by line" 0
+	gdb_continue_to_breakpoint "break at multiply for explicit line" \
+	    ".*Break at multiply.*"
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
+	    "set explicit breakpoint by relative line" 0
+	gdb_continue_to_breakpoint "break at add for relative line" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
+	    "set explicit breakpoint by relative negative line" 0
+	gdb_continue_to_breakpoint "break at multiply for negative line" \
+	    ".*Break at multiply.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (line=bp1)" \
+	    "RuntimeError: Line keyword should be an integer or a string.*" \
+	    "set explicit breakpoint by invalid line type"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
+	    "set explicit breakpoint by function" 0
+	gdb_continue_to_breakpoint "break at function add for function" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
+	    "set explicit breakpoint by source file and function" 0
+	gdb_continue_to_breakpoint "break at function add for source and function" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
+	    "set explicit breakpoint by source file and line number" 0
+	gdb_continue_to_breakpoint "break at add for source and line" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -line $bp_location2\")" \
+	    "set explicit breakpoint by source file and line number in spec" 0
+	gdb_continue_to_breakpoint "break at add for source and line in spec" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
+	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
+	    "set invalid explicit breakpoint by source only"
+
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
+	    "No source file named foo.*" \
+	    "set invalid explicit breakpoint by missing source and line"
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
+	    "No line 900 in file \"$srcfile\".*" \
+	    "set invalid explicit breakpoint by source and invalid line"
+	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
+	    "Function \"blah\" not defined.*" \
+	    "set invalid explicit breakpoint by missing function"
+    }
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -558,3 +637,4 @@ test_bkpt_temporary
 test_bkpt_address
 test_bkpt_pending
 test_bkpt_events
+test_bkpt_explicit_loc

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On 07/12/17 10:02, Phil Muldoon wrote:
> On 23/11/17 22:17, Simon Marchi wrote:
>> On 2017-11-17 06:02 AM, Phil Muldoon wrote:
>>> All,
>>>
>>> Here's an updated patch. ChangeLog remains the same (except for
>>> additional NEWS entry)
>>>
>>> I just realised this needs a doc review also.
>>>
>>> Cheers
>>>
>>> Phil
>>
>> Hi Phil,
> 
> Here's a new patch. I think I've addressed all of the issues.
> 
> Cheers
> 
> Phil

As my Christmas vacation is rapidly approaching and Pedro mentioned
he'd like to see this for 8.1, he asked me to format the patch to be
git am'able in case he would have to shepherd the last bits of this
patch in. I've included the patch, duly formatted, as an attachment
to this email 

Cheers,

Phil

[-- Attachment #2: 0001-Implement-explicit-locations-for-Python-breakpoints.patch --]
[-- Type: text/x-patch, Size: 14915 bytes --]

From 319d9e802995aafcdd14ce8cf63aec1320519079 Mon Sep 17 00:00:00 2001
From: Phil Muldoon <pmuldoon@redhat.com>
Date: Tue, 07 Dec 2017 12:09:08 +0100
Subject: [PATCH] Implement explicit locations for Python breakpoints.
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

This introduces several new keywords to the bppy_init constructor.
The spec parameter is now optional but mutually exclusive to the
explicit keywords source, label, function and line.

gdb/ChangeLog

2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>

       * python/py-breakpoint.c (bppy_init): Use string_to_event_location
       over basic location code. Implement explicit location keywords.
       (bppy_init_validate_args): New function.
       * NEWS: Document Python explicit breakpoint locations.

doc/ChangeLog

2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>

       * python.texi (Breakpoints In Python): Add text relating
       to allowed explicit locations and keywords in gdb.Breakpoints.

testsuite/ChangeLog

2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>

       * gdb.python/py-breakpoint.exp (test_bkpt_explicit_loc): Add new
       tests for explicit locations.
---
 gdb/NEWS                                   |   3 +
 gdb/doc/python.texi                        |  41 ++++-----
 gdb/python/py-breakpoint.c                 | 132 ++++++++++++++++++++++++++---
 gdb/testsuite/gdb.python/py-breakpoint.c   |   2 +-
 gdb/testsuite/gdb.python/py-breakpoint.exp |  80 +++++++++++++++++
 5 files changed, 226 insertions(+), 32 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 9246659bfb..592fe70156 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -24,6 +24,9 @@
      gdb.new_thread are emitted.  See the manual for further
      description of these.
 
+  ** Python breakpoints can now accept explicit locations.  See the
+     manual for a further description of this feature.
+
 * New features in the GDB remote stub, GDBserver
 
   ** New "--selftest" command line option runs some GDBserver self
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index f661e489bb..25cee3f93e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4861,27 +4861,30 @@ represented as Python @code{Long} values.
 Python code can manipulate breakpoints via the @code{gdb.Breakpoint}
 class.
 
-@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[},internal @r{[},temporary@r{]]]]})
+@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[}, internal @r{[}, temporary @r{]}, source @r{]}, function @r{]}, label @r{]}, line @r{]]]]]]]]})
 Create a new breakpoint according to @var{spec}, which is a string
 naming the location of the breakpoint, or an expression that defines a
-watchpoint.  The contents can be any location recognized by the
-@code{break} command, or in the case of a watchpoint, by the
-@code{watch} command.  The optional @var{type} denotes the breakpoint
-to create from the types defined later in this chapter.  This argument
-can be either @code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}; it
-defaults to @code{gdb.BP_BREAKPOINT}.  The optional @var{internal}
-argument allows the breakpoint to become invisible to the user.  The
-breakpoint will neither be reported when created, nor will it be
-listed in the output from @code{info breakpoints} (but will be listed
-with the @code{maint info breakpoints} command).  The optional
-@var{temporary} argument makes the breakpoint a temporary breakpoint.
-Temporary breakpoints are deleted after they have been hit.  Any
-further access to the Python breakpoint after it has been hit will
-result in a runtime error (as that breakpoint has now been
-automatically deleted).  The optional @var{wp_class} argument defines
-the class of watchpoint to create, if @var{type} is
-@code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not provided, it
-is assumed to be a @code{gdb.WP_WRITE} class.
+watchpoint. The contents can be any location recognized by the
+@code{break} command or, in the case of a watchpoint, by the
+@code{watch} command.  Alternatively, create a new a explicit location
+breakpoint (@pxref{Explicit Locations}) according to the
+specifications contained in the key words @var{source},
+@var{function}, @var{label} and @var{line}.  The optional @var{type}
+denotes the breakpoint to create from the types defined later in this
+chapter.  This argument can be either @code{gdb.BP_BREAKPOINT} or
+@code{gdb.BP_WATCHPOINT}; it defaults to @code{gdb.BP_BREAKPOINT}.
+The optional @var{internal} argument allows the breakpoint to become
+invisible to the user.  The breakpoint will neither be reported when
+created, nor will it be listed in the output from @code{info
+breakpoints} (but will be listed with the @code{maint info
+breakpoints} command).  The optional @var{temporary} argument makes
+the breakpoint a temporary breakpoint.  Temporary breakpoints are
+deleted after they have been hit.  Any further access to the Python
+breakpoint after it has been hit will result in a runtime error (as
+that breakpoint has now been automatically deleted).  The optional
+@var{wp_class} argument defines the class of watchpoint to create, if
+@var{type} is @code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not
+provided, it is assumed to be a @code{gdb.WP_WRITE} class.
 @end defun
 
 The available types are represented by constants defined in the @code{gdb}
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index d57c2fa05e..417f02d612 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -32,6 +32,7 @@
 #include "language.h"
 #include "location.h"
 #include "py-event.h"
+#include "linespec.h"
 
 /* Number of live breakpoints.  */
 static int bppy_live;
@@ -633,25 +634,103 @@ bppy_get_ignore_count (PyObject *self, void *closure)
   return PyInt_FromLong (self_bp->bp->ignore_count);
 }
 
+/* Internal function to validate the Python parameters/keywords
+   provided to bppy_init.  */
+static int
+bppy_init_validate_args (const char *spec, char *source,
+			 char *function, char *label,
+			 char *line, enum bptype type)
+{
+  /* If spec is defined, ensure that none of the explicit location
+     keywords are also defined.  */
+  if (spec != NULL)
+    {
+      if (source != NULL || function != NULL || label != NULL || line != NULL)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Breakpoints specified with spec cannot "
+			     "have source, function, label or line defined."));
+	  return -1;
+	}
+    }
+  else
+    {
+      /* If spec isn't defined, ensure that the user is not trying to
+	 define a watchpoint with an explicit location.  */
+      if (type == bp_watchpoint)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Watchpoints cannot be set by explicit "
+			     "location parameters."));
+	  return -1;
+	}
+      else
+	{
+	  /* Otherwise, ensure some explicit locations are defined.  */
+	  if (source == NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Neither spec nor explicit location set."));
+	      return -1;
+	    }
+	  /* Finally, if source is specified, ensure that line, label
+	     or function are specified too.  */
+	  if (source != NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Specifying a source must also include a "
+				 "line, label or function."));
+	      return -1;
+	    }
+	}
+    }
+  return 1;
+}
+
 /* Python function to create a new breakpoint.  */
 static int
 bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
   static const char *keywords[] = { "spec", "type", "wp_class", "internal",
-				    "temporary", NULL };
-  const char *spec;
-  int type = bp_breakpoint;
+				    "temporary","source", "function",
+				    "label", "line", NULL };
+  const char *spec = NULL;
+  enum bptype type = bp_breakpoint;
   int access_type = hw_write;
   PyObject *internal = NULL;
   PyObject *temporary = NULL;
+  PyObject *lineobj = NULL;;
   int internal_bp = 0;
   int temporary_bp = 0;
+  gdb::unique_xmalloc_ptr<char> line;
+  char *label = NULL;
+  char *source = NULL;
+  char *function = NULL;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiOO", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssO", keywords,
 					&spec, &type, &access_type,
-					&internal, &temporary))
+					&internal,
+					&temporary, &source,
+					&function, &label, &lineobj))
     return -1;
 
+
+  if (lineobj != NULL)
+    {
+      if (PyInt_Check (lineobj))
+	line.reset (xstrprintf ("%ld", PyInt_AsLong (lineobj)));
+      else if (PyString_Check (lineobj))
+	line = python_string_to_host_string (lineobj);
+      else
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Line keyword should be an integer or a string. "));
+	  return -1;
+	}
+    }
+
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
@@ -666,22 +745,47 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	return -1;
     }
 
+  if (bppy_init_validate_args (spec, source, function, label, line.get (),
+			       type) == -1)
+    return -1;
+
   bppy_pending_object = (gdbpy_breakpoint_object *) self;
   bppy_pending_object->number = -1;
   bppy_pending_object->bp = NULL;
 
   TRY
     {
-      gdb::unique_xmalloc_ptr<char>
-	copy_holder (xstrdup (skip_spaces (spec)));
-      char *copy = copy_holder.get ();
-
       switch (type)
 	{
 	case bp_breakpoint:
 	  {
-	    event_location_up location
-	      = string_to_event_location_basic (&copy, current_language);
+	    event_location_up location;
+
+	    if (spec != NULL)
+	      {
+		gdb::unique_xmalloc_ptr<char>
+		  copy_holder (xstrdup (skip_spaces (spec)));
+		char *copy = copy_holder.get ();
+
+		location  = string_to_event_location (&copy,
+						      current_language);
+	      }
+	    else
+	      {
+		struct explicit_location explicit_loc;
+
+		initialize_explicit_location (&explicit_loc);
+		explicit_loc.source_filename = source;
+		explicit_loc.function_name = function;
+		explicit_loc.label_name = label;
+
+		if (line != NULL)
+		  explicit_loc.line_offset =
+		    linespec_parse_line_offset (line.get ());
+
+		location = new_explicit_location (&explicit_loc);
+	      }
+
 	    create_breakpoint (python_gdbarch,
 			       location.get (), NULL, -1, NULL,
 			       0,
@@ -692,8 +796,12 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 			       0, 1, internal_bp, 0);
 	    break;
 	  }
-        case bp_watchpoint:
+	case bp_watchpoint:
 	  {
+	    gdb::unique_xmalloc_ptr<char>
+	      copy_holder (xstrdup (skip_spaces (spec)));
+	    char *copy = copy_holder.get ();
+
 	    if (access_type == hw_write)
 	      watch_command_wrapper (copy, 0, internal_bp);
 	    else if (access_type == hw_access)
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
index df6163e53a..562cab35f7 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.c
+++ b/gdb/testsuite/gdb.python/py-breakpoint.c
@@ -24,7 +24,7 @@ int multiply (int i)
 
 int add (int i)
 {
-  return i + i; 
+  return i + i;  /* Break at function add.  */
 }
 
 
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index bd138ac3d2..2eca7f366a 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -547,6 +547,85 @@ proc test_bkpt_events {} {
     check_last_event breakpoint_deleted
 }
 
+proc test_bkpt_explicit_loc {} {
+    global srcfile testfile
+
+    with_test_prefix test_bkpt_invisible {
+	# Start with a fresh gdb.
+	clean_restart ${testfile}
+
+	if ![runto_main] then {
+	    fail "cannot run to main."
+	    return 0
+	}
+
+	delete_breakpoints
+
+	set bp_location1 [gdb_get_line_number "Break at multiply."]
+	set bp_location2 [gdb_get_line_number "Break at add."]
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=$bp_location1)" \
+	    "set explicit breakpoint by line" 0
+	gdb_continue_to_breakpoint "break at multiply for explicit line" \
+	    ".*Break at multiply.*"
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
+	    "set explicit breakpoint by relative line" 0
+	gdb_continue_to_breakpoint "break at add for relative line" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
+	    "set explicit breakpoint by relative negative line" 0
+	gdb_continue_to_breakpoint "break at multiply for negative line" \
+	    ".*Break at multiply.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (line=bp1)" \
+	    "RuntimeError: Line keyword should be an integer or a string.*" \
+	    "set explicit breakpoint by invalid line type"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
+	    "set explicit breakpoint by function" 0
+	gdb_continue_to_breakpoint "break at function add for function" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
+	    "set explicit breakpoint by source file and function" 0
+	gdb_continue_to_breakpoint "break at function add for source and function" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
+	    "set explicit breakpoint by source file and line number" 0
+	gdb_continue_to_breakpoint "break at add for source and line" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -line $bp_location2\")" \
+	    "set explicit breakpoint by source file and line number in spec" 0
+	gdb_continue_to_breakpoint "break at add for source and line in spec" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
+	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
+	    "set invalid explicit breakpoint by source only"
+
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
+	    "No source file named foo.*" \
+	    "set invalid explicit breakpoint by missing source and line"
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
+	    "No line 900 in file \"$srcfile\".*" \
+	    "set invalid explicit breakpoint by source and invalid line"
+	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
+	    "Function \"blah\" not defined.*" \
+	    "set invalid explicit breakpoint by missing function"
+    }
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -558,3 +637,4 @@ test_bkpt_temporary
 test_bkpt_address
 test_bkpt_pending
 test_bkpt_events
+test_bkpt_explicit_loc
-- 
2.14.3

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-12-07 07:15, Phil Muldoon wrote:
> On 07/12/17 10:02, Phil Muldoon wrote:
>> On 23/11/17 22:17, Simon Marchi wrote:
>>> On 2017-11-17 06:02 AM, Phil Muldoon wrote:
>>>> All,
>>>> 
>>>> Here's an updated patch. ChangeLog remains the same (except for
>>>> additional NEWS entry)
>>>> 
>>>> I just realised this needs a doc review also.
>>>> 
>>>> Cheers
>>>> 
>>>> Phil
>>> 
>>> Hi Phil,
>> 
>> Here's a new patch. I think I've addressed all of the issues.
>> 
>> Cheers
>> 
>> Phil
> 
> As my Christmas vacation is rapidly approaching and Pedro mentioned
> he'd like to see this for 8.1, he asked me to format the patch to be
> git am'able in case he would have to shepherd the last bits of this
> patch in. I've included the patch, duly formatted, as an attachment
> to this email
> 
> Cheers,
> 
> Phil

Thanks for the git-am format, it's usually easier to apply.  However, I 
can't seem to find the right baseline.  Which commit did you base this 
on?  If it is old-ish, would it be possible for you to rebase it to 
current master?

Thanks,

Simon

Re: [python] Allow explicit locations in breakpoints.

[Phil Muldoon]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On 07/12/17 14:54, Simon Marchi wrote:
> On 2017-12-07 07:15, Phil Muldoon wrote:
>> On 07/12/17 10:02, Phil Muldoon wrote:
>>> On 23/11/17 22:17, Simon Marchi wrote:
>>>> On 2017-11-17 06:02 AM, Phil Muldoon wrote:

>> Phil
> 
> Thanks for the git-am format, it's usually easier to apply.  However, I can't seem to find the right baseline.  Which commit did you base this on?  If it is old-ish, would it be possible for you to rebase it to current master?
> 
> Thanks,
> 
> Simon

Apologies, my github branch was more behind master than I at first
thought. I've attached the update patch.

Cheers,

Phil


[-- Attachment #2: 0001-Implement-explicit-locations-for-Python-breakpoints.patch --]
[-- Type: text/x-patch, Size: 14956 bytes --]

From: Phil Muldoon <pmuldoon@redhat.com>
Date: Thu, 7 Dec 2017 15:05:51 +0000
Subject: [PATCH] Implement explicit locations for Python breakpoints.
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

This introduces several new keywords to the bppy_init constructor.
The spec parameter is now optional but mutually exclusive to the
explicit keywords source, label, function and line.

gdb/ChangeLog

2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>

       * python/py-breakpoint.c (bppy_init): Use string_to_event_location
       over basic location code. Implement explicit location keywords.
       (bppy_init_validate_args): New function.
       * NEWS: Document Python explicit breakpoint locations.

doc/ChangeLog

2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>

       * python.texi (Breakpoints In Python): Add text relating
       to allowed explicit locations and keywords in gdb.Breakpoints.

testsuite/ChangeLog

2017-12-07  Phil Muldoon  <pmuldoon@redhat.com>

       * gdb.python/py-breakpoint.exp (test_bkpt_explicit_loc): Add new
       tests for explicit locations.
---
 gdb/NEWS                                   |   4 +
 gdb/doc/python.texi                        |  41 +++++----
 gdb/python/py-breakpoint.c                 | 134 ++++++++++++++++++++++++++---
 gdb/testsuite/gdb.python/py-breakpoint.c   |   2 +-
 gdb/testsuite/gdb.python/py-breakpoint.exp |  80 +++++++++++++++++
 5 files changed, 228 insertions(+), 33 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 2262b259a8..72b4057f4d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -115,6 +115,10 @@
      command allows the setting of a large number of breakpoints via a
      regex pattern in Python.  See the manual for further details.
 
+  ** Python breakpoints can now accept explicit locations.  See the
+     manual for a further description of this feature.
+
+
 * New features in the GDB remote stub, GDBserver
 
   ** GDBserver is now able to start inferior processes with a
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index f411f60d7e..28a7a1a9f5 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4878,27 +4878,30 @@ represented as Python @code{Long} values.
 Python code can manipulate breakpoints via the @code{gdb.Breakpoint}
 class.
 
-@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[},internal @r{[},temporary@r{]]]]})
+@defun Breakpoint.__init__ (spec @r{[}, type @r{[}, wp_class @r{[}, internal @r{[}, temporary @r{]}, source @r{]}, function @r{]}, label @r{]}, line @r{]]]]]]]]})
 Create a new breakpoint according to @var{spec}, which is a string
 naming the location of the breakpoint, or an expression that defines a
-watchpoint.  The contents can be any location recognized by the
-@code{break} command, or in the case of a watchpoint, by the
-@code{watch} command.  The optional @var{type} denotes the breakpoint
-to create from the types defined later in this chapter.  This argument
-can be either @code{gdb.BP_BREAKPOINT} or @code{gdb.BP_WATCHPOINT}; it
-defaults to @code{gdb.BP_BREAKPOINT}.  The optional @var{internal}
-argument allows the breakpoint to become invisible to the user.  The
-breakpoint will neither be reported when created, nor will it be
-listed in the output from @code{info breakpoints} (but will be listed
-with the @code{maint info breakpoints} command).  The optional
-@var{temporary} argument makes the breakpoint a temporary breakpoint.
-Temporary breakpoints are deleted after they have been hit.  Any
-further access to the Python breakpoint after it has been hit will
-result in a runtime error (as that breakpoint has now been
-automatically deleted).  The optional @var{wp_class} argument defines
-the class of watchpoint to create, if @var{type} is
-@code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not provided, it
-is assumed to be a @code{gdb.WP_WRITE} class.
+watchpoint. The contents can be any location recognized by the
+@code{break} command or, in the case of a watchpoint, by the
+@code{watch} command.  Alternatively, create a new a explicit location
+breakpoint (@pxref{Explicit Locations}) according to the
+specifications contained in the key words @var{source},
+@var{function}, @var{label} and @var{line}.  The optional @var{type}
+denotes the breakpoint to create from the types defined later in this
+chapter.  This argument can be either @code{gdb.BP_BREAKPOINT} or
+@code{gdb.BP_WATCHPOINT}; it defaults to @code{gdb.BP_BREAKPOINT}.
+The optional @var{internal} argument allows the breakpoint to become
+invisible to the user.  The breakpoint will neither be reported when
+created, nor will it be listed in the output from @code{info
+breakpoints} (but will be listed with the @code{maint info
+breakpoints} command).  The optional @var{temporary} argument makes
+the breakpoint a temporary breakpoint.  Temporary breakpoints are
+deleted after they have been hit.  Any further access to the Python
+breakpoint after it has been hit will result in a runtime error (as
+that breakpoint has now been automatically deleted).  The optional
+@var{wp_class} argument defines the class of watchpoint to create, if
+@var{type} is @code{gdb.BP_WATCHPOINT}.  If a watchpoint class is not
+provided, it is assumed to be a @code{gdb.WP_WRITE} class.
 @end defun
 
 The available types are represented by constants defined in the @code{gdb}
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 2574683ed4..f865317ab3 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -32,6 +32,7 @@
 #include "language.h"
 #include "location.h"
 #include "py-event.h"
+#include "linespec.h"
 
 /* Number of live breakpoints.  */
 static int bppy_live;
@@ -631,25 +632,104 @@ bppy_get_ignore_count (PyObject *self, void *closure)
   return PyInt_FromLong (self_bp->bp->ignore_count);
 }
 
+/* Internal function to validate the Python parameters/keywords
+   provided to bppy_init.  */
+
+static int
+bppy_init_validate_args (const char *spec, char *source,
+			 char *function, char *label,
+			 char *line, enum bptype type)
+{
+  /* If spec is defined, ensure that none of the explicit location
+     keywords are also defined.  */
+  if (spec != NULL)
+    {
+      if (source != NULL || function != NULL || label != NULL || line != NULL)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Breakpoints specified with spec cannot "
+			     "have source, function, label or line defined."));
+	  return -1;
+	}
+    }
+  else
+    {
+      /* If spec isn't defined, ensure that the user is not trying to
+	 define a watchpoint with an explicit location.  */
+      if (type == bp_watchpoint)
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Watchpoints cannot be set by explicit "
+			     "location parameters."));
+	  return -1;
+	}
+      else
+	{
+	  /* Otherwise, ensure some explicit locations are defined.  */
+	  if (source == NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Neither spec nor explicit location set."));
+	      return -1;
+	    }
+	  /* Finally, if source is specified, ensure that line, label
+	     or function are specified too.  */
+	  if (source != NULL && function == NULL && label == NULL
+	      && line == NULL)
+	    {
+	      PyErr_SetString (PyExc_RuntimeError,
+			       _("Specifying a source must also include a "
+				 "line, label or function."));
+	      return -1;
+	    }
+	}
+    }
+  return 1;
+}
+
 /* Python function to create a new breakpoint.  */
 static int
 bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
   static const char *keywords[] = { "spec", "type", "wp_class", "internal",
-				    "temporary", NULL };
-  const char *spec;
-  int type = bp_breakpoint;
+				    "temporary","source", "function",
+				    "label", "line", NULL };
+  const char *spec = NULL;
+  enum bptype type = bp_breakpoint;
   int access_type = hw_write;
   PyObject *internal = NULL;
   PyObject *temporary = NULL;
+  PyObject *lineobj = NULL;;
   int internal_bp = 0;
   int temporary_bp = 0;
+  gdb::unique_xmalloc_ptr<char> line;
+  char *label = NULL;
+  char *source = NULL;
+  char *function = NULL;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|iiOO", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssO", keywords,
 					&spec, &type, &access_type,
-					&internal, &temporary))
+					&internal,
+					&temporary, &source,
+					&function, &label, &lineobj))
     return -1;
 
+
+  if (lineobj != NULL)
+    {
+      if (PyInt_Check (lineobj))
+	line.reset (xstrprintf ("%ld", PyInt_AsLong (lineobj)));
+      else if (PyString_Check (lineobj))
+	line = python_string_to_host_string (lineobj);
+      else
+	{
+	  PyErr_SetString (PyExc_RuntimeError,
+			   _("Line keyword should be an integer or a string. "));
+	  return -1;
+	}
+    }
+
   if (internal)
     {
       internal_bp = PyObject_IsTrue (internal);
@@ -664,23 +744,47 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	return -1;
     }
 
+  if (bppy_init_validate_args (spec, source, function, label, line.get (),
+			       type) == -1)
+    return -1;
+
   bppy_pending_object = (gdbpy_breakpoint_object *) self;
   bppy_pending_object->number = -1;
   bppy_pending_object->bp = NULL;
 
   TRY
     {
-      gdb::unique_xmalloc_ptr<char>
-	copy_holder (xstrdup (skip_spaces (spec)));
-      const char *copy = copy_holder.get ();
-
       switch (type)
 	{
 	case bp_breakpoint:
 	  {
-	    event_location_up location
-	      = string_to_event_location_basic (&copy, current_language,
-						symbol_name_match_type::WILD);
+	    event_location_up location;
+
+	    if (spec != NULL)
+	      {
+		gdb::unique_xmalloc_ptr<char>
+		  copy_holder (xstrdup (skip_spaces (spec)));
+		const char *copy = copy_holder.get ();
+
+		location  = string_to_event_location (&copy,
+						      current_language);
+	      }
+	    else
+	      {
+		struct explicit_location explicit_loc;
+
+		initialize_explicit_location (&explicit_loc);
+		explicit_loc.source_filename = source;
+		explicit_loc.function_name = function;
+		explicit_loc.label_name = label;
+
+		if (line != NULL)
+		  explicit_loc.line_offset =
+		    linespec_parse_line_offset (line.get ());
+
+		location = new_explicit_location (&explicit_loc);
+	      }
+
 	    create_breakpoint (python_gdbarch,
 			       location.get (), NULL, -1, NULL,
 			       0,
@@ -691,8 +795,12 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 			       0, 1, internal_bp, 0);
 	    break;
 	  }
-        case bp_watchpoint:
+	case bp_watchpoint:
 	  {
+	    gdb::unique_xmalloc_ptr<char>
+	      copy_holder (xstrdup (skip_spaces (spec)));
+	    char *copy = copy_holder.get ();
+
 	    if (access_type == hw_write)
 	      watch_command_wrapper (copy, 0, internal_bp);
 	    else if (access_type == hw_access)
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
index df6163e53a..562cab35f7 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.c
+++ b/gdb/testsuite/gdb.python/py-breakpoint.c
@@ -24,7 +24,7 @@ int multiply (int i)
 
 int add (int i)
 {
-  return i + i; 
+  return i + i;  /* Break at function add.  */
 }
 
 
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index 6c39f13696..e89b9b8446 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -531,6 +531,85 @@ proc_with_prefix test_bkpt_events {} {
     check_last_event breakpoint_deleted
 }
 
+proc test_bkpt_explicit_loc {} {
+    global srcfile testfile
+
+    with_test_prefix test_bkpt_invisible {
+	# Start with a fresh gdb.
+	clean_restart ${testfile}
+
+	if ![runto_main] then {
+	    fail "cannot run to main."
+	    return 0
+	}
+
+	delete_breakpoints
+
+	set bp_location1 [gdb_get_line_number "Break at multiply."]
+	set bp_location2 [gdb_get_line_number "Break at add."]
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=$bp_location1)" \
+	    "set explicit breakpoint by line" 0
+	gdb_continue_to_breakpoint "break at multiply for explicit line" \
+	    ".*Break at multiply.*"
+
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"+1\")" \
+	    "set explicit breakpoint by relative line" 0
+	gdb_continue_to_breakpoint "break at add for relative line" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (line=\"-1\")" \
+	    "set explicit breakpoint by relative negative line" 0
+	gdb_continue_to_breakpoint "break at multiply for negative line" \
+	    ".*Break at multiply.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (line=bp1)" \
+	    "RuntimeError: Line keyword should be an integer or a string.*" \
+	    "set explicit breakpoint by invalid line type"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (function=\"add\")" \
+	    "set explicit breakpoint by function" 0
+	gdb_continue_to_breakpoint "break at function add for function" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", function=\"add\")" \
+	    "set explicit breakpoint by source file and function" 0
+	gdb_continue_to_breakpoint "break at function add for source and function" \
+	    ".*Break at function add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"$bp_location2\")" \
+	    "set explicit breakpoint by source file and line number" 0
+	gdb_continue_to_breakpoint "break at add for source and line" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"-source $srcfile -line $bp_location2\")" \
+	    "set explicit breakpoint by source file and line number in spec" 0
+	gdb_continue_to_breakpoint "break at add for source and line in spec" \
+	    ".*Break at add.*"
+
+	delete_breakpoints
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\")" \
+	    "RuntimeError: Specifying a source must also include a line, label or function.*" \
+	    "set invalid explicit breakpoint by source only"
+
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"foo.c\", line=\"5\")" \
+	    "No source file named foo.*" \
+	    "set invalid explicit breakpoint by missing source and line"
+	gdb_test "python bp1 = gdb.Breakpoint (source=\"$srcfile\", line=\"900\")" \
+	    "No line 900 in file \"$srcfile\".*" \
+	    "set invalid explicit breakpoint by source and invalid line"
+	gdb_test "python bp1 = gdb.Breakpoint (function=\"blah\")" \
+	    "Function \"blah\" not defined.*" \
+	    "set invalid explicit breakpoint by missing function"
+    }
+}
+
 test_bkpt_basic
 test_bkpt_deletion
 test_bkpt_cond_and_cmds
@@ -542,3 +621,4 @@ test_bkpt_temporary
 test_bkpt_address
 test_bkpt_pending
 test_bkpt_events
+test_bkpt_explicit_loc
-- 
2.14.3

Re: [python] Allow explicit locations in breakpoints.

[Simon Marchi]

On 2017-12-07 10:12 AM, Phil Muldoon wrote:
> Apologies, my github branch was more behind master than I at first
> thought. I've attached the update patch.
> 
> Cheers,
> 
> Phil
> 

Hi Phil,

That version is fine with me.  As mentioned on IRC, it would be nice to
have a Python parameter equivalent to the -q/--qualified CLI switch.  I'd
be fine with it being done as another patch on top.

Simon

Re: [python] Allow explicit locations in breakpoints.

[Eli Zaretskii]

> From: Phil Muldoon <pmuldoon@redhat.com>
> Date: Thu, 7 Dec 2017 10:02:44 +0000
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 9246659bfb..592fe70156 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -24,6 +24,9 @@
>       gdb.new_thread are emitted.  See the manual for further
>       description of these.
>  
> +  ** Python breakpoints can now accept explicit locations.  See the
> +     manual for a further description of this feature.
                   ^^^^^^^^^
"the detailed"?

Otherwise the documentation parts are okay.  Thanks.