* [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct @ 2023-04-03 14:54 Mohamed Bouhaouel 2023-04-04 11:37 ` Andrew Burgess 2023-04-04 18:27 ` Tom Tromey 0 siblings, 2 replies; 5+ messages in thread From: Mohamed Bouhaouel @ 2023-04-03 14:54 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct 2023-04-03 14:54 [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct Mohamed Bouhaouel @ 2023-04-04 11:37 ` Andrew Burgess 2023-04-04 14:38 ` Bouhaouel, Mohamed 2023-04-04 18:27 ` Tom Tromey 1 sibling, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2023-04-04 11:37 UTC (permalink / raw) To: Mohamed Bouhaouel, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct 2023-04-04 11:37 ` Andrew Burgess @ 2023-04-04 14:38 ` Bouhaouel, Mohamed 2023-04-04 16:29 ` Bouhaouel, Mohamed 0 siblings, 1 reply; 5+ messages in thread From: Bouhaouel, Mohamed @ 2023-04-04 14:38 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct 2023-04-04 14:38 ` Bouhaouel, Mohamed @ 2023-04-04 16:29 ` Bouhaouel, Mohamed 0 siblings, 0 replies; 5+ messages in thread From: Bouhaouel, Mohamed @ 2023-04-04 16:29 UTC (permalink / raw) To: Andrew Burgess, gdb-patches > > > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct 2023-04-03 14:54 [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct Mohamed Bouhaouel 2023-04-04 11:37 ` Andrew Burgess @ 2023-04-04 18:27 ` Tom Tromey 1 sibling, 0 replies; 5+ messages in thread From: Tom Tromey @ 2023-04-04 18:27 UTC (permalink / raw) To: Mohamed Bouhaouel via Gdb-patches; +Cc: Mohamed Bouhaouel >>>>> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-04-04 18:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-03 14:54 [PATCH 1/1] gdb, breakpoint: add a destructor to the watchpoint struct Mohamed Bouhaouel 2023-04-04 11:37 ` Andrew Burgess 2023-04-04 14:38 ` Bouhaouel, Mohamed 2023-04-04 16:29 ` Bouhaouel, Mohamed 2023-04-04 18:27 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).