[PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Mohamed Bouhaouel]

Make sure to unlink the related breakpoint when the watchpoint instance
is deleted.  This prevents having a wp-related breakpoint that is
linked to a NULL watchpoint (e.g.  the watchpoint instance is being
deleted when the 'watch' command fails).  With the below scenario,
having such a left out breakpoint will lead to a GDB hang, and this
is due to an infinite loop when deleting all inferior breakpoints.

Scenario:
	(gdb) awatch <VAR>
	Target does not support this type of hardware watchpoint.
	(gdb) rwatch <VAR>
	Target does not support this type of hardware watchpoint.
	(gdb) <continue the program until the end>
	>> HANG <<

Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
---
 gdb/breakpoint.c | 15 +++++++++++++++
 gdb/breakpoint.h |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7228acfd8fe..b9ddc85c30c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9581,6 +9581,21 @@ break_range_command (const char *arg, int from_tty)
   install_breakpoint (false, std::move (br), true);
 }
 
+/* See breakpoint.h.  */
+
+watchpoint::~watchpoint ()
+{
+  /* Make sure to unlink the destroyed watchpoint from the related
+     breakpoint ring.  */
+
+  breakpoint *bpt = this;
+  breakpoint *related = bpt;
+  while (related->related_breakpoint != bpt)
+    related = related->related_breakpoint;
+
+  related->related_breakpoint = bpt->related_breakpoint;
+}
+
 /*  Return non-zero if EXP is verified as constant.  Returned zero
     means EXP is variable.  Also the constant detection may fail for
     some constant expressions and in such case still falsely return
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7c5cf3f2bef..4f19e42dd83 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
   void print_recreate (struct ui_file *fp) const override;
   bool explains_signal (enum gdb_signal) override;
 
+  /* Destructor for WATCHPOINT.  */
+  virtual ~watchpoint ();
+
   /* String form of exp to use for displaying to the user (malloc'd),
      or NULL if none.  */
   gdb::unique_xmalloc_ptr<char> exp_string;
-- 
2.25.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Andrew Burgess]

Mohamed Bouhaouel via Gdb-patches <gdb-patches@sourceware.org> writes:

> Make sure to unlink the related breakpoint when the watchpoint instance
> is deleted.  This prevents having a wp-related breakpoint that is
> linked to a NULL watchpoint (e.g.  the watchpoint instance is being
> deleted when the 'watch' command fails).  With the below scenario,
> having such a left out breakpoint will lead to a GDB hang, and this
> is due to an infinite loop when deleting all inferior breakpoints.
>
> Scenario:
> 	(gdb) awatch <VAR>
> 	Target does not support this type of hardware watchpoint.
> 	(gdb) rwatch <VAR>
> 	Target does not support this type of hardware watchpoint.
> 	(gdb) <continue the program until the end>
> 	>> HANG <<
>

Is it not possible to create a test case based on this scenario?

Thanks,
Andrew

> Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
> ---
>  gdb/breakpoint.c | 15 +++++++++++++++
>  gdb/breakpoint.h |  3 +++
>  2 files changed, 18 insertions(+)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 7228acfd8fe..b9ddc85c30c 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -9581,6 +9581,21 @@ break_range_command (const char *arg, int from_tty)
>    install_breakpoint (false, std::move (br), true);
>  }
>  
> +/* See breakpoint.h.  */
> +
> +watchpoint::~watchpoint ()
> +{
> +  /* Make sure to unlink the destroyed watchpoint from the related
> +     breakpoint ring.  */
> +
> +  breakpoint *bpt = this;
> +  breakpoint *related = bpt;
> +  while (related->related_breakpoint != bpt)
> +    related = related->related_breakpoint;
> +
> +  related->related_breakpoint = bpt->related_breakpoint;
> +}
> +
>  /*  Return non-zero if EXP is verified as constant.  Returned zero
>      means EXP is variable.  Also the constant detection may fail for
>      some constant expressions and in such case still falsely return
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 7c5cf3f2bef..4f19e42dd83 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
>    void print_recreate (struct ui_file *fp) const override;
>    bool explains_signal (enum gdb_signal) override;
>  
> +  /* Destructor for WATCHPOINT.  */
> +  virtual ~watchpoint ();
> +
>    /* String form of exp to use for displaying to the user (malloc'd),
>       or NULL if none.  */
>    gdb::unique_xmalloc_ptr<char> exp_string;
> -- 
> 2.25.1
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Bouhaouel, Mohamed]

Thanks Andrew for your feedback.

> > Scenario:
> > 	(gdb) awatch <VAR>
> > 	Target does not support this type of hardware watchpoint.
> > 	(gdb) rwatch <VAR>
> > 	Target does not support this type of hardware watchpoint.
> > 	(gdb) <continue the program until the end>
> > 	>> HANG <<
> >
> 
> Is it not possible to create a test case based on this scenario?

It is difficult to produce such scenario due to a weird behavior [1],
which needs to be addressed as well.  As a result, deleting watchpoints
before resuming would hide the issue.


[1] When the watch command fails, it is not possible to resume
unless we explicitly delete the watchpoint.  More details can be
found here:
https://sourceware.org/pipermail/gdb-patches/2023-March/198371.html)

Kind regards,
Mohamed



> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Tuesday, April 4, 2023 1:38 PM
> To: Bouhaouel, Mohamed <mohamed.bouhaouel@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint
> struct
> 
> Mohamed Bouhaouel via Gdb-patches <gdb-patches@sourceware.org>
> writes:
> 
> > Make sure to unlink the related breakpoint when the watchpoint instance
> > is deleted.  This prevents having a wp-related breakpoint that is
> > linked to a NULL watchpoint (e.g.  the watchpoint instance is being
> > deleted when the 'watch' command fails).  With the below scenario,
> > having such a left out breakpoint will lead to a GDB hang, and this
> > is due to an infinite loop when deleting all inferior breakpoints.
> >
> > Scenario:
> > 	(gdb) awatch <VAR>
> > 	Target does not support this type of hardware watchpoint.
> > 	(gdb) rwatch <VAR>
> > 	Target does not support this type of hardware watchpoint.
> > 	(gdb) <continue the program until the end>
> > 	>> HANG <<
> >
> 
> Is it not possible to create a test case based on this scenario?
> 
> Thanks,
> Andrew
> 
> > Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
> > ---
> >  gdb/breakpoint.c | 15 +++++++++++++++
> >  gdb/breakpoint.h |  3 +++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> > index 7228acfd8fe..b9ddc85c30c 100644
> > --- a/gdb/breakpoint.c
> > +++ b/gdb/breakpoint.c
> > @@ -9581,6 +9581,21 @@ break_range_command (const char *arg, int
> from_tty)
> >    install_breakpoint (false, std::move (br), true);
> >  }
> >
> > +/* See breakpoint.h.  */
> > +
> > +watchpoint::~watchpoint ()
> > +{
> > +  /* Make sure to unlink the destroyed watchpoint from the related
> > +     breakpoint ring.  */
> > +
> > +  breakpoint *bpt = this;
> > +  breakpoint *related = bpt;
> > +  while (related->related_breakpoint != bpt)
> > +    related = related->related_breakpoint;
> > +
> > +  related->related_breakpoint = bpt->related_breakpoint;
> > +}
> > +
> >  /*  Return non-zero if EXP is verified as constant.  Returned zero
> >      means EXP is variable.  Also the constant detection may fail for
> >      some constant expressions and in such case still falsely return
> > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> > index 7c5cf3f2bef..4f19e42dd83 100644
> > --- a/gdb/breakpoint.h
> > +++ b/gdb/breakpoint.h
> > @@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
> >    void print_recreate (struct ui_file *fp) const override;
> >    bool explains_signal (enum gdb_signal) override;
> >
> > +  /* Destructor for WATCHPOINT.  */
> > +  virtual ~watchpoint ();
> > +
> >    /* String form of exp to use for displaying to the user (malloc'd),
> >       or NULL if none.  */
> >    gdb::unique_xmalloc_ptr<char> exp_string;
> > --
> > 2.25.1
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Bouhaouel, Mohamed]

> > > Scenario:
> > > 	(gdb) awatch <VAR>
> > > 	Target does not support this type of hardware watchpoint.
> > > 	(gdb) rwatch <VAR>
> > > 	Target does not support this type of hardware watchpoint.
> > > 	(gdb) <continue the program until the end>
> > > 	>> HANG <<
> > >
> >
> > Is it not possible to create a test case based on this scenario?
> 
> It is difficult to produce such scenario due to a weird behavior [1],
> which needs to be addressed as well.  As a result, deleting watchpoints
> before resuming would hide the issue.


I have been able to come up with a similar scenario that exposes the hang.
- set can-use-hw-watchpoints 0
- stop inside a scope with local variables.
- (gdb) awatch <LOCAL VAR>
  Can't set read/access watchpoint when hardware watchpoints are disabled.
- (gdb) rwatch <LOCAL VAR>
  Can't set read/access watchpoint when hardware watchpoints are disabled.
- (gdb) <continue the program until the end>
  >> HANG <<

I will add a regression test and submit a revised patch.

Regards,
Mohamed




> -----Original Message-----
> From: Bouhaouel, Mohamed
> Sent: Tuesday, April 4, 2023 4:39 PM
> To: Andrew Burgess <aburgess@redhat.com>; gdb-patches@sourceware.org
> Subject: RE: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint
> struct
> 
> 
> Thanks Andrew for your feedback.
> 
> > > Scenario:
> > > 	(gdb) awatch <VAR>
> > > 	Target does not support this type of hardware watchpoint.
> > > 	(gdb) rwatch <VAR>
> > > 	Target does not support this type of hardware watchpoint.
> > > 	(gdb) <continue the program until the end>
> > > 	>> HANG <<
> > >
> >
> > Is it not possible to create a test case based on this scenario?
> 
> It is difficult to produce such scenario due to a weird behavior [1],
> which needs to be addressed as well.  As a result, deleting watchpoints
> before resuming would hide the issue.
> 
> 
> [1] When the watch command fails, it is not possible to resume
> unless we explicitly delete the watchpoint.  More details can be
> found here:
> https://sourceware.org/pipermail/gdb-patches/2023-March/198371.html)
> 
> Kind regards,
> Mohamed
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Burgess <aburgess@redhat.com>
> > Sent: Tuesday, April 4, 2023 1:38 PM
> > To: Bouhaouel, Mohamed <mohamed.bouhaouel@intel.com>; gdb-
> > patches@sourceware.org
> > Subject: Re: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint
> > struct
> >
> > Mohamed Bouhaouel via Gdb-patches <gdb-patches@sourceware.org>
> > writes:
> >
> > > Make sure to unlink the related breakpoint when the watchpoint instance
> > > is deleted.  This prevents having a wp-related breakpoint that is
> > > linked to a NULL watchpoint (e.g.  the watchpoint instance is being
> > > deleted when the 'watch' command fails).  With the below scenario,
> > > having such a left out breakpoint will lead to a GDB hang, and this
> > > is due to an infinite loop when deleting all inferior breakpoints.
> > >
> > > Scenario:
> > > 	(gdb) awatch <VAR>
> > > 	Target does not support this type of hardware watchpoint.
> > > 	(gdb) rwatch <VAR>
> > > 	Target does not support this type of hardware watchpoint.
> > > 	(gdb) <continue the program until the end>
> > > 	>> HANG <<
> > >
> >
> > Is it not possible to create a test case based on this scenario?
> >
> > Thanks,
> > Andrew
> >
> > > Signed-off-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
> > > ---
> > >  gdb/breakpoint.c | 15 +++++++++++++++
> > >  gdb/breakpoint.h |  3 +++
> > >  2 files changed, 18 insertions(+)
> > >
> > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> > > index 7228acfd8fe..b9ddc85c30c 100644
> > > --- a/gdb/breakpoint.c
> > > +++ b/gdb/breakpoint.c
> > > @@ -9581,6 +9581,21 @@ break_range_command (const char *arg, int
> > from_tty)
> > >    install_breakpoint (false, std::move (br), true);
> > >  }
> > >
> > > +/* See breakpoint.h.  */
> > > +
> > > +watchpoint::~watchpoint ()
> > > +{
> > > +  /* Make sure to unlink the destroyed watchpoint from the related
> > > +     breakpoint ring.  */
> > > +
> > > +  breakpoint *bpt = this;
> > > +  breakpoint *related = bpt;
> > > +  while (related->related_breakpoint != bpt)
> > > +    related = related->related_breakpoint;
> > > +
> > > +  related->related_breakpoint = bpt->related_breakpoint;
> > > +}
> > > +
> > >  /*  Return non-zero if EXP is verified as constant.  Returned zero
> > >      means EXP is variable.  Also the constant detection may fail for
> > >      some constant expressions and in such case still falsely return
> > > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> > > index 7c5cf3f2bef..4f19e42dd83 100644
> > > --- a/gdb/breakpoint.h
> > > +++ b/gdb/breakpoint.h
> > > @@ -933,6 +933,9 @@ struct watchpoint : public breakpoint
> > >    void print_recreate (struct ui_file *fp) const override;
> > >    bool explains_signal (enum gdb_signal) override;
> > >
> > > +  /* Destructor for WATCHPOINT.  */
> > > +  virtual ~watchpoint ();
> > > +
> > >    /* String form of exp to use for displaying to the user (malloc'd),
> > >       or NULL if none.  */
> > >    gdb::unique_xmalloc_ptr<char> exp_string;
> > > --
> > > 2.25.1
> > >
> > > Intel Deutschland GmbH
> > > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> > > Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
> > > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> > > Chairperson of the Supervisory Board: Nicole Lau
> > > Registered Office: Munich
> > > Commercial Register: Amtsgericht Muenchen HRB 186928

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct

[Tom Tromey]

>>>>> Mohamed Bouhaouel via Gdb-patches <gdb-patches@sourceware.org> writes:

> +  /* Destructor for WATCHPOINT.  */
> +  virtual ~watchpoint ();

I don't think the 'virtual' is needed here.

Tom