[RFC] Let user set breakpoint with mouse.

Message ID 20210618122929.568323-1-daniellevin2607@gmail.com
State New
Headers show
Series
  • [RFC] Let user set breakpoint with mouse.
Related show

Commit Message

Philippe Waroquiers via Gdb-patches June 18, 2021, 12:29 p.m.
Greetings all

This is my first GDB patch.

I implemented the feature proposed by #23632. The user can now click on a line in either the source or disasm windows in tui to toggle a breakpoint there.

There is a good chance the large number of arguments passed to create_breakpoint is erroneous in an edge-case I have yet to find, which is why there is an RFC in the subject line.

I look forward to feedback, comments and so on.

Best,

Daniel Levin

---
 gdb/breakpoint.c        | 16 +++++++++++++++-
 gdb/breakpoint.h        |  5 +++++
 gdb/tui/tui-winsource.c | 33 +++++++++++++++++++++++++++++++++
 gdb/tui/tui-winsource.h |  8 ++++++++
 4 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.31.1

Comments

Philippe Waroquiers via Gdb-patches June 18, 2021, 12:50 p.m. | #1
Am Freitag, 18. Juni 2021, 14:30:09 MESZ hat Daniel Levin via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> Greetings all

>

> This is my first GDB patch.

>

> I implemented the feature proposed by #23632. The user can now click on a line in either the source or disasm windows in tui to toggle a breakpoint there.

>

> There is a good chance the large number of arguments passed to create_breakpoint is erroneous in an edge-case I have yet to find, which is why there is an RFC in the subject line.

>

> I look forward to feedback, comments and so on.


Just FYI, I also sent something similar to this some time ago:
https://sourceware.org/pipermail/gdb-patches/2021-March/176814.html

It worked slightly different, a left-click where a breakpoint already was,
would disable it, and only a middle-click would delete it.

But yours includes the disasm window, that's better.

I haven't tried yours yet, but with mine I had the problem that once I
created so many breakpoints that the command window shows the pagination
question, gdb would crash when trying to continue from there.
I'm wondering if this happens here as well.


Hannes


>

> Best,

>

> Daniel Levin

>

> ---

> gdb/breakpoint.c        | 16 +++++++++++++++-

> gdb/breakpoint.h        |  5 +++++

> gdb/tui/tui-winsource.c | 33 +++++++++++++++++++++++++++++++++

> gdb/tui/tui-winsource.h |  8 ++++++++

> 4 files changed, 61 insertions(+), 1 deletion(-)

>

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c

> index fb011fc1e0f..43900573fe5 100644

> --- a/gdb/breakpoint.c

> +++ b/gdb/breakpoint.c

> @@ -79,7 +79,6 @@

> #include <algorithm>

> #include "progspace-and-thread.h"

> #include "gdbsupport/array-view.h"

> -#include "gdbsupport/gdb_optional.h"

>

> /* Prototypes for local functions.  */

>

> @@ -4036,6 +4035,21 @@ breakpoint_init_inferior (enum inf_context context)

>   moribund_locations.clear ();

> }

>

> +gdb::optional<bp_location *>

> +get_breakpoint_here (const address_space *aspace, CORE_ADDR pc)

> +{

> +  for (bp_location *bl : all_bp_locations ())

> +    {

> +      if (bl->loc_type != bp_loc_software_breakpoint

> +      && bl->loc_type != bp_loc_hardware_breakpoint)

> +    continue;

> +

> +      if (breakpoint_location_address_match (bl, aspace, pc))

> +        return bl;

> +    }

> +  return {};

> +}

> +

> /* These functions concern about actual breakpoints inserted in the

>     target --- to e.g. check if we need to do decr_pc adjustment or if

>     we need to hop over the bkpt --- so we check for address space

> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h

> index e40504f14ed..533b0eae5e1 100644

> --- a/gdb/breakpoint.h

> +++ b/gdb/breakpoint.h

> @@ -30,6 +30,7 @@

> #include "gdbsupport/array-view.h"

> #include "gdbsupport/filtered-iterator.h"

> #include "gdbsupport/function-view.h"

> +#include "gdbsupport/gdb_optional.h"

> #include "gdbsupport/refcounted-object.h"

> #include "gdbsupport/safe-iterator.h"

> #include "cli/cli-script.h"

> @@ -1227,6 +1228,10 @@ enum breakpoint_here

>

> /* Prototypes for breakpoint-related functions.  */

>

> +/* Box possible breakpoint at the given location in an optional type */

> +extern gdb::optional<bp_location *>

> +get_breakpoint_here (const address_space *, CORE_ADDR);

> +

> extern enum breakpoint_here breakpoint_here_p (const address_space *,

>                           CORE_ADDR);

>

> diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c

> index afd51e95980..a4754bfa5f8 100644

> --- a/gdb/tui/tui-winsource.c

> +++ b/gdb/tui/tui-winsource.c

> @@ -524,3 +524,36 @@ tui_source_window_base::update_exec_info ()

>     }

>   refresh_window ();

> }

> +

> +static CORE_ADDR

> +resolve_to_addr (tui_source_element *src_element)

> +{

> +  if (src_element->line_or_addr.loa == LOA_ADDRESS)

> +      return src_element->line_or_addr.u.addr;

> +

> +  CORE_ADDR loc_of_src_line;

> +  struct symtab_and_line cursal = get_current_source_symtab_and_line ();

> +  find_line_pc (cursal.symtab, src_element->line_or_addr.u.line_no, &loc_of_src_line);

> +  return loc_of_src_line;

> +}

> +

> +void

> +tui_source_window_base::toggle_bp_at_line (tui_source_element *src_element) {

> +  CORE_ADDR clicked_addr = resolve_to_addr (src_element);

> +  auto possible_prev_bp = get_breakpoint_here (current_program_space->aspace, clicked_addr);

> +  if (possible_prev_bp.has_value () && (*possible_prev_bp)->owner != nullptr)

> +    delete_breakpoint ((*possible_prev_bp)->owner);

> +  else

> +    {

> +      auto here = new_address_location (clicked_addr, nullptr, 0);

> +      create_breakpoint (m_gdbarch,

> +            here.get(), NULL, -1, NULL, false,

> +            0,

> +            0, bp_breakpoint,

> +            0,

> +            AUTO_BOOLEAN_TRUE,

> +            &bkpt_breakpoint_ops,

> +            0, 1,

> +            0, 0);

> +    }

> +}

> diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h

> index d4528e351f4..6eee0c72b43 100644

> --- a/gdb/tui/tui-winsource.h

> +++ b/gdb/tui/tui-winsource.h

> @@ -124,6 +124,14 @@ struct tui_source_window_base : public tui_win_info

>   /* Redraw the complete line of a source or disassembly window.  */

>   void show_source_line (int lineno);

>

> +  void click (int mouse_x, int mouse_y, int mouse_button) override

> +  {

> +    if (mouse_button == 1 && mouse_y < m_content.size ())

> +      toggle_bp_at_line (&m_content[mouse_y]);

> +  }

> +

> +  void toggle_bp_at_line (tui_source_element *src_element);

> +

>   /* Used for horizontal scroll.  */

>   int m_horizontal_offset = 0;

>   struct tui_line_or_address m_start_line_or_addr;

> --

> 2.31.1
Philippe Waroquiers via Gdb-patches June 18, 2021, 11:15 p.m. | #2
> Just FYI, I also sent something similar to this some time ago:


Hannes, thanks for the feedback. I did not see your earlier patch. In
reading some of the requests you received to it, such as pulling up
the breakpoint-setting member function to the base class, I realized I
inadvertently did that anyway.

> I haven't tried yours yet, but with mine I had the problem that once I

> created so many breakpoints that the command window shows the pagination

> question, gdb would crash when trying to continue from there.

> I'm wondering if this happens here as well.


I cannot reproduce a bug of that description in my implementation.
Perhaps we can take this discussion into a private thread to hash out
the details?

I found it surprising that I needed to write a function to find a
breakpoint placed at a given address. It seems natural in some sense
to have a high-level API in a debugger that allows simple read/writes
of a collection of breakpoints. Your implementation traverses the
linked list of breakpoints to find the one you're interested in, which
suggests that I didn't simply miss the "proper" way - which was a
concern of mine.

Best,

Daniel Levin


On Fri, Jun 18, 2021 at 2:50 PM Hannes Domani <ssbssa@yahoo.de> wrote:
>

>  Am Freitag, 18. Juni 2021, 14:30:09 MESZ hat Daniel Levin via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

>

> > Greetings all

> >

> > This is my first GDB patch.

> >

> > I implemented the feature proposed by #23632. The user can now click on a line in either the source or disasm windows in tui to toggle a breakpoint there.

> >

> > There is a good chance the large number of arguments passed to create_breakpoint is erroneous in an edge-case I have yet to find, which is why there is an RFC in the subject line.

> >

> > I look forward to feedback, comments and so on.

>

> Just FYI, I also sent something similar to this some time ago:

> https://sourceware.org/pipermail/gdb-patches/2021-March/176814.html

>

> It worked slightly different, a left-click where a breakpoint already was,

> would disable it, and only a middle-click would delete it.

>

> But yours includes the disasm window, that's better.

>

> I haven't tried yours yet, but with mine I had the problem that once I

> created so many breakpoints that the command window shows the pagination

> question, gdb would crash when trying to continue from there.

> I'm wondering if this happens here as well.

>

>

> Hannes

>

>

> >

> > Best,

> >

> > Daniel Levin

> >

> > ---

> > gdb/breakpoint.c        | 16 +++++++++++++++-

> > gdb/breakpoint.h        |  5 +++++

> > gdb/tui/tui-winsource.c | 33 +++++++++++++++++++++++++++++++++

> > gdb/tui/tui-winsource.h |  8 ++++++++

> > 4 files changed, 61 insertions(+), 1 deletion(-)

> >

> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c

> > index fb011fc1e0f..43900573fe5 100644

> > --- a/gdb/breakpoint.c

> > +++ b/gdb/breakpoint.c

> > @@ -79,7 +79,6 @@

> > #include <algorithm>

> > #include "progspace-and-thread.h"

> > #include "gdbsupport/array-view.h"

> > -#include "gdbsupport/gdb_optional.h"

> >

> > /* Prototypes for local functions.  */

> >

> > @@ -4036,6 +4035,21 @@ breakpoint_init_inferior (enum inf_context context)

> >   moribund_locations.clear ();

> > }

> >

> > +gdb::optional<bp_location *>

> > +get_breakpoint_here (const address_space *aspace, CORE_ADDR pc)

> > +{

> > +  for (bp_location *bl : all_bp_locations ())

> > +    {

> > +      if (bl->loc_type != bp_loc_software_breakpoint

> > +      && bl->loc_type != bp_loc_hardware_breakpoint)

> > +    continue;

> > +

> > +      if (breakpoint_location_address_match (bl, aspace, pc))

> > +        return bl;

> > +    }

> > +  return {};

> > +}

> > +

> > /* These functions concern about actual breakpoints inserted in the

> >     target --- to e.g. check if we need to do decr_pc adjustment or if

> >     we need to hop over the bkpt --- so we check for address space

> > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h

> > index e40504f14ed..533b0eae5e1 100644

> > --- a/gdb/breakpoint.h

> > +++ b/gdb/breakpoint.h

> > @@ -30,6 +30,7 @@

> > #include "gdbsupport/array-view.h"

> > #include "gdbsupport/filtered-iterator.h"

> > #include "gdbsupport/function-view.h"

> > +#include "gdbsupport/gdb_optional.h"

> > #include "gdbsupport/refcounted-object.h"

> > #include "gdbsupport/safe-iterator.h"

> > #include "cli/cli-script.h"

> > @@ -1227,6 +1228,10 @@ enum breakpoint_here

> >

> > /* Prototypes for breakpoint-related functions.  */

> >

> > +/* Box possible breakpoint at the given location in an optional type */

> > +extern gdb::optional<bp_location *>

> > +get_breakpoint_here (const address_space *, CORE_ADDR);

> > +

> > extern enum breakpoint_here breakpoint_here_p (const address_space *,

> >                           CORE_ADDR);

> >

> > diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c

> > index afd51e95980..a4754bfa5f8 100644

> > --- a/gdb/tui/tui-winsource.c

> > +++ b/gdb/tui/tui-winsource.c

> > @@ -524,3 +524,36 @@ tui_source_window_base::update_exec_info ()

> >     }

> >   refresh_window ();

> > }

> > +

> > +static CORE_ADDR

> > +resolve_to_addr (tui_source_element *src_element)

> > +{

> > +  if (src_element->line_or_addr.loa == LOA_ADDRESS)

> > +      return src_element->line_or_addr.u.addr;

> > +

> > +  CORE_ADDR loc_of_src_line;

> > +  struct symtab_and_line cursal = get_current_source_symtab_and_line ();

> > +  find_line_pc (cursal.symtab, src_element->line_or_addr.u.line_no, &loc_of_src_line);

> > +  return loc_of_src_line;

> > +}

> > +

> > +void

> > +tui_source_window_base::toggle_bp_at_line (tui_source_element *src_element) {

> > +  CORE_ADDR clicked_addr = resolve_to_addr (src_element);

> > +  auto possible_prev_bp = get_breakpoint_here (current_program_space->aspace, clicked_addr);

> > +  if (possible_prev_bp.has_value () && (*possible_prev_bp)->owner != nullptr)

> > +    delete_breakpoint ((*possible_prev_bp)->owner);

> > +  else

> > +    {

> > +      auto here = new_address_location (clicked_addr, nullptr, 0);

> > +      create_breakpoint (m_gdbarch,

> > +            here.get(), NULL, -1, NULL, false,

> > +            0,

> > +            0, bp_breakpoint,

> > +            0,

> > +            AUTO_BOOLEAN_TRUE,

> > +            &bkpt_breakpoint_ops,

> > +            0, 1,

> > +            0, 0);

> > +    }

> > +}

> > diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h

> > index d4528e351f4..6eee0c72b43 100644

> > --- a/gdb/tui/tui-winsource.h

> > +++ b/gdb/tui/tui-winsource.h

> > @@ -124,6 +124,14 @@ struct tui_source_window_base : public tui_win_info

> >   /* Redraw the complete line of a source or disassembly window.  */

> >   void show_source_line (int lineno);

> >

> > +  void click (int mouse_x, int mouse_y, int mouse_button) override

> > +  {

> > +    if (mouse_button == 1 && mouse_y < m_content.size ())

> > +      toggle_bp_at_line (&m_content[mouse_y]);

> > +  }

> > +

> > +  void toggle_bp_at_line (tui_source_element *src_element);

> > +

> >   /* Used for horizontal scroll.  */

> >   int m_horizontal_offset = 0;

> >   struct tui_line_or_address m_start_line_or_addr;

> > --

> > 2.31.1

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fb011fc1e0f..43900573fe5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -79,7 +79,6 @@ 
 #include <algorithm>
 #include "progspace-and-thread.h"
 #include "gdbsupport/array-view.h"
-#include "gdbsupport/gdb_optional.h"
 
 /* Prototypes for local functions.  */
 
@@ -4036,6 +4035,21 @@  breakpoint_init_inferior (enum inf_context context)
   moribund_locations.clear ();
 }
 
+gdb::optional<bp_location *>
+get_breakpoint_here (const address_space *aspace, CORE_ADDR pc)
+{
+  for (bp_location *bl : all_bp_locations ())
+    {
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
+	continue;
+
+      if (breakpoint_location_address_match (bl, aspace, pc))
+        return bl;
+    }
+  return {};
+}
+
 /* These functions concern about actual breakpoints inserted in the
    target --- to e.g. check if we need to do decr_pc adjustment or if
    we need to hop over the bkpt --- so we check for address space
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e40504f14ed..533b0eae5e1 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -30,6 +30,7 @@ 
 #include "gdbsupport/array-view.h"
 #include "gdbsupport/filtered-iterator.h"
 #include "gdbsupport/function-view.h"
+#include "gdbsupport/gdb_optional.h"
 #include "gdbsupport/refcounted-object.h"
 #include "gdbsupport/safe-iterator.h"
 #include "cli/cli-script.h"
@@ -1227,6 +1228,10 @@  enum breakpoint_here
 
 /* Prototypes for breakpoint-related functions.  */
 
+/* Box possible breakpoint at the given location in an optional type */
+extern gdb::optional<bp_location *>
+get_breakpoint_here (const address_space *, CORE_ADDR);
+
 extern enum breakpoint_here breakpoint_here_p (const address_space *,
 					       CORE_ADDR);
 
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index afd51e95980..a4754bfa5f8 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -524,3 +524,36 @@  tui_source_window_base::update_exec_info ()
     }
   refresh_window ();
 }
+
+static CORE_ADDR
+resolve_to_addr (tui_source_element *src_element)
+{
+  if (src_element->line_or_addr.loa == LOA_ADDRESS)
+	  return src_element->line_or_addr.u.addr;
+
+  CORE_ADDR loc_of_src_line;
+  struct symtab_and_line cursal = get_current_source_symtab_and_line ();
+  find_line_pc (cursal.symtab, src_element->line_or_addr.u.line_no, &loc_of_src_line);
+  return loc_of_src_line;
+}
+
+void
+tui_source_window_base::toggle_bp_at_line (tui_source_element *src_element) {
+  CORE_ADDR clicked_addr = resolve_to_addr (src_element);
+  auto possible_prev_bp = get_breakpoint_here (current_program_space->aspace, clicked_addr);
+  if (possible_prev_bp.has_value () && (*possible_prev_bp)->owner != nullptr)
+    delete_breakpoint ((*possible_prev_bp)->owner);
+  else
+    {
+      auto here = new_address_location (clicked_addr, nullptr, 0);
+      create_breakpoint (m_gdbarch,
+			 here.get(), NULL, -1, NULL, false,
+			 0,
+			 0, bp_breakpoint,
+			 0,
+			 AUTO_BOOLEAN_TRUE,
+			 &bkpt_breakpoint_ops,
+			 0, 1, 
+			 0, 0);
+    }
+}
diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
index d4528e351f4..6eee0c72b43 100644
--- a/gdb/tui/tui-winsource.h
+++ b/gdb/tui/tui-winsource.h
@@ -124,6 +124,14 @@  struct tui_source_window_base : public tui_win_info
   /* Redraw the complete line of a source or disassembly window.  */
   void show_source_line (int lineno);
 
+  void click (int mouse_x, int mouse_y, int mouse_button) override
+  {
+    if (mouse_button == 1 && mouse_y < m_content.size ())
+      toggle_bp_at_line (&m_content[mouse_y]);
+  }
+
+  void toggle_bp_at_line (tui_source_element *src_element);
+
   /* Used for horizontal scroll.  */
   int m_horizontal_offset = 0;
   struct tui_line_or_address m_start_line_or_addr;