Re: [PATCH] Fix Python probe breakpoints

[Simon Marchi <simark@simark.ca>]

On 2019-12-07 10:10 p.m., George Barrett wrote:
> On Sat, Dec 07, 2019 at 09:35:30PM -0500, Simon Marchi wrote:
>> Put the name of the modified variable or function in parenthesis after the
>> file name, like:
>>
>> 	* breakpoint.c (bkpt_probe_breakpoint_ops): Make non-static.
> 
> Thanks for the correction, I was having a bit of trouble making sure I
> understood the instructions on the wiki correctly.

Well it was really not bad for a first time!

>> A suggestion to improve maintainability. To avoid duplicating the logic,
>> define a function "breakpoint_ops_for_event_location_type" that does this
>> conversion, and use it in the break_command_1 function too.
> 
> Sure. Should the addition of that function be a separate patch?

I think this is simple enough to be in the same patch.  If there had been dozens
of call sites to fix, it would have made sense to introduce the function in a
separate patch, but I don't think it's necessary here.

>> Can you check if Guile exhibits the same problem? I'm guessing that the
>> gdbscm_register_breakpoint_x should receive the same treatment.
> 
> A quick peek at the source indicates the Guile code has the same problem, but
> I can't really test it as it fails to build against recent libguile.

Indeed, it only builds with guile 2.0 (someone needs to port the code to guile 2.2).

Most distros have a "guile-2.0" package (the name may vary).  When installed, it's
then possible to configure gdb to use it with the --with-guile=guile-2.0 configure flag.

>> On 2019-12-07 11:46 a.m., George Barrett wrote:
>>> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.c b/gdb/testsuite/gdb.python/py-breakpoint.c
>>> index d102a5f306..bf5bec1200 100644
>>> --- a/gdb/testsuite/gdb.python/py-breakpoint.c
>>> +++ b/gdb/testsuite/gdb.python/py-breakpoint.c
>>> @@ -15,6 +15,10 @@
>>>     You should have received a copy of the GNU General Public License
>>>     along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
>>>  
>>> +#ifdef USE_PROBES
>>> +#include <sys/sdt.h>
>>> +#endif
>>> +
>>>  int result = 0;
>>>  
>>>  namespace foo_ns
>>> @@ -46,6 +50,9 @@ int main (int argc, char *argv[])
>>>      {
>>>        result += multiply (foo);  /* Break at multiply. */
>>>        result += add (bar); /* Break at add. */
>>> +#ifdef USE_PROBES
>>> +      STAP_PROBE1 (test, result_updated, result);
>>> +#endif
>>>      }
>>>  
>>>    return 0; /* Break at end. */
> 
> Just to check: is it okay to introduce a SystemTap dependency here? I just
> realised this could be made slightly more portable by using DTRACE_PROBE1
> instead of STAP_PROBE1; AFAICT that should work on BSD and Solaris, but sdt.h
> is probably not a standard header on those platforms either and it might still
> exclude things like Cygwin...

I'm not very familiar with this, but indeed using DTRACE_PROBE1 looks like a good
idea.  The sys/sdt.h headers seems to be the right one on FreeBSD and Solaris.  If
it's not, it won't be hard to fix for whoever wants to fix it.

Btw, I expect that your patch will be small enough that it doesn't require a copyright
assignment.  But if you plan on contributing patches larger than this, you will need
to file a copyright assignment with the FSF, contact me off-list if you'd like to do that.

Thanks,

Simon