* [PATCH] breakpoint: Make sure location types match before swapping @ 2020-04-01 1:38 Keno Fischer 2020-04-14 7:01 ` Keno Fischer 0 siblings, 1 reply; 17+ messages in thread From: Keno Fischer @ 2020-04-01 1:38 UTC (permalink / raw) To: gdb-patches This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1". In particular, what occurs in that case is that a hardware breakpoint is hit, after which GDB removes it and establishes a single step breakpoint at the same location. Afterwards, rather than simply removing this breakpoint and re-enabling the hardware breakpoint, GDB simply swaps the activation, without informing the server, leading to an inconsistency in GDB's view of the world and the server's view of the world. To remidy this situation, this patch adds a check that ensures two breakpoint locations have the same type before they are considered equal and thus eligible for silent swapping. gdb/ChangeLog: * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741 Signed-off-by: Keno Fischer <keno@juliacomputing.com> --- gdb/breakpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461b..582dae1946 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1, /* We compare bp_location.length in order to cover ranged breakpoints. */ return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, loc2->pspace->aspace, loc2->address) - && loc1->length == loc2->length); + && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type); } static void -- 2.24.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-01 1:38 [PATCH] breakpoint: Make sure location types match before swapping Keno Fischer @ 2020-04-14 7:01 ` Keno Fischer 2020-04-14 15:04 ` Simon Marchi 0 siblings, 1 reply; 17+ messages in thread From: Keno Fischer @ 2020-04-14 7:01 UTC (permalink / raw) To: gdb-patches Bump. It would be great to get this fixed. On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> wrote: > > This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1". > In particular, what occurs in that case is that a hardware breakpoint is hit, > after which GDB removes it and establishes a single step breakpoint at the > same location. Afterwards, rather than simply removing this breakpoint and > re-enabling the hardware breakpoint, GDB simply swaps the activation, without > informing the server, leading to an inconsistency in GDB's view of the world > and the server's view of the world. To remidy this situation, this > patch adds a check that ensures two breakpoint locations have the > same type before they are considered equal and thus eligible for silent > swapping. > > gdb/ChangeLog: > * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741 > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > --- > gdb/breakpoint.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index e49025461b..582dae1946 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1, > /* We compare bp_location.length in order to cover ranged breakpoints. */ > return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, > loc2->pspace->aspace, loc2->address) > - && loc1->length == loc2->length); > + && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type); > } > > static void > -- > 2.24.0 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-14 7:01 ` Keno Fischer @ 2020-04-14 15:04 ` Simon Marchi 2020-04-14 16:00 ` Andrew Burgess 0 siblings, 1 reply; 17+ messages in thread From: Simon Marchi @ 2020-04-14 15:04 UTC (permalink / raw) To: Keno Fischer, gdb-patches On 2020-04-14 3:01 a.m., Keno Fischer wrote: > Bump. It would be great to get this fixed. > > On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> wrote: >> >> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1". >> In particular, what occurs in that case is that a hardware breakpoint is hit, >> after which GDB removes it and establishes a single step breakpoint at the >> same location. Afterwards, rather than simply removing this breakpoint and >> re-enabling the hardware breakpoint, GDB simply swaps the activation, without >> informing the server, leading to an inconsistency in GDB's view of the world >> and the server's view of the world. To remidy this situation, this >> patch adds a check that ensures two breakpoint locations have the >> same type before they are considered equal and thus eligible for silent >> swapping. >> >> gdb/ChangeLog: >> * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741 >> >> Signed-off-by: Keno Fischer <keno@juliacomputing.com> >> --- >> gdb/breakpoint.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index e49025461b..582dae1946 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c >> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1, >> /* We compare bp_location.length in order to cover ranged breakpoints. */ >> return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, >> loc2->pspace->aspace, loc2->address) >> - && loc1->length == loc2->length); >> + && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type); >> } >> >> static void >> -- >> 2.24.0 >> I think the change makes sense, but this is not an area I know well, and it's one that is a bit sensitive. I'll do a full test run and take a bit more time to look at it. In the mean time, if anybody else wants to take a look, go for it. Simon ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-14 15:04 ` Simon Marchi @ 2020-04-14 16:00 ` Andrew Burgess 2020-04-14 16:26 ` Keno Fischer 2020-04-14 19:17 ` Pedro Alves 0 siblings, 2 replies; 17+ messages in thread From: Andrew Burgess @ 2020-04-14 16:00 UTC (permalink / raw) To: Simon Marchi; +Cc: Keno Fischer, gdb-patches * Simon Marchi <simark@simark.ca> [2020-04-14 11:04:47 -0400]: > On 2020-04-14 3:01 a.m., Keno Fischer wrote: > > Bump. It would be great to get this fixed. > > > > On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> wrote: > >> > >> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, but remove it using z1". > >> In particular, what occurs in that case is that a hardware breakpoint is hit, > >> after which GDB removes it and establishes a single step breakpoint at the > >> same location. Afterwards, rather than simply removing this breakpoint and > >> re-enabling the hardware breakpoint, GDB simply swaps the activation, without > >> informing the server, leading to an inconsistency in GDB's view of the world > >> and the server's view of the world. To remidy this situation, this > >> patch adds a check that ensures two breakpoint locations have the > >> same type before they are considered equal and thus eligible for silent > >> swapping. > >> > >> gdb/ChangeLog: > >> * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741 > >> > >> Signed-off-by: Keno Fischer <keno@juliacomputing.com> > >> --- > >> gdb/breakpoint.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > >> index e49025461b..582dae1946 100644 > >> --- a/gdb/breakpoint.c > >> +++ b/gdb/breakpoint.c > >> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location *loc1, > >> /* We compare bp_location.length in order to cover ranged breakpoints. */ > >> return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, > >> loc2->pspace->aspace, loc2->address) > >> - && loc1->length == loc2->length); > >> + && loc1->length == loc2->length && loc1->loc_type == loc2->loc_type); > >> } > >> > >> static void > >> -- > >> 2.24.0 > >> > > I think the change makes sense, but this is not an area I know well, and it's one that > is a bit sensitive. I'll do a full test run and take a bit more time to look at it. > > In the mean time, if anybody else wants to take a look, go for it. I was able to reproduce this issue with a hacked up RISC-V target (forced software single step on for bare-metal) running on a development board I have access too. I agree that this fix feels like the right thing to do, it's inline with location checking done in watchpoint_locations_match. The only question I had when comparing to watchpoint_locations_match, is whether we should be similarly comparing the types of the owner breakpoint rather than the actual location. However, the b/p type is overloaded to contain more than just whether the breakpoint is h/w or s/w, so in this case we have the user h/w breakpoint, its type is bp_hardware_breakpoint, while the software single step breakpoint has type bp_single_step. The conclusion I came to is that it really is the type of the location we need to compare here. The other slight issue I had with the patch was that based on the original description I still had to go and figure out the exact conditions that would trigger this bug. I believe that to trigger this you need a h/w breakpoint placed on an instruction that loops to itself, I don't see how else this could happen. I took a crack at writing a more detailed break down of the conditions that trigger this bug. I'm still running this through testing here, but I'd be interested to hear if you think the fuller description is helpful. Thanks, Andrew --- From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 From: Andrew Burgess <andrew.burgess@embecosm.com> Date: Tue, 14 Apr 2020 16:32:02 +0100 Subject: [PATCH] gdb: ensure location types match before swapping locations In the following conditions: - A target with hardware breakpoints available, and - A target that uses software single stepping, - An instruction at ADDRESS loops back to itself, Now consider the following steps: 1. The user places a hardware breakpoint at ADDRESS (an instruction that loops to itself), 2. The inferior runs and hits the breakpoint from #1, 3. The user tells GDB to 'continue'. In #3 when the user tells GDB to continue, GDB first disables the hardware breakpoint at ADDRESS, and then inserts a software single step breakpoint at ADDRESS. The original user created breakpoint was a hardware breakpoint, while the single step breakpoint will be a software breakpoint. GDB continues and immediately hits the software single step breakpoint. GDB then deletes the software single step breakpoint by calling delete_single_step_breakpoints, which eventually calls delete_breakpoint, which, once the breakpoint (and its locations) are deleted, calls update_global_location_list. During update_global_location_list GDB spots that we have an old location (the software single step breakpoint location) that is inserted, but being deleted, and a location at the same address which we are keeping, but which is not currently inserted (the original hardware breakpoint location), GDB then calls breakpoint_locations_match on these two locations. Currently the locations do match, and so GDB calls swap_insertion which swaps the "inserted" state of the two locations. GDB finally returns through the call stack and leaves delete_single_step_breakpoints. After this GDB continues with its normal "stopping" process, as part of this stopping process GDB removes all the breakpoints from the target. Due to the swap the original hardware breakpoint location is removed. The problem is that GDB inserted the software single step breakpoint as a software breakpoint, and then, thanks to the swap, tries to remove it as a hardware breakpoint. This will leave the target in an inconsistent state, and as in the original bug report (PR gdb/25741), could result in the target throwing an error. The solution for all this is to have two breakpoint locations of different types (hardware breakpoint vs software breakpoint) not match. The result of this is that GDB will no longer swap the inserted status of the two breakpoints. The original call to update_global_location_list (after deleting the software single step breakpoint) will at this point trigger a call to remove the breakpoint, something which will be done based on the type of that location. Later GDB will see that the original hardware breakpoint is already not inserted, and so will leave it alone. This patch was original proposed by Keno Fischer here: https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html gdb/ChangeLog: PR gdb/25741 * breakpoint.c (breakpoint_locations_match): Compare location types. --- gdb/ChangeLog | 6 ++++++ gdb/breakpoint.c | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..2ab40a8e40a 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -6838,7 +6838,8 @@ breakpoint_locations_match (struct bp_location *loc1, /* We compare bp_location.length in order to cover ranged breakpoints. */ return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, loc2->pspace->aspace, loc2->address) - && loc1->length == loc2->length); + && loc1->length == loc2->length + && loc1->loc_type == loc2->loc_type); } static void -- 2.25.2 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-14 16:00 ` Andrew Burgess @ 2020-04-14 16:26 ` Keno Fischer 2020-04-14 19:17 ` Pedro Alves 1 sibling, 0 replies; 17+ messages in thread From: Keno Fischer @ 2020-04-14 16:26 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, gdb-patches Thanks for the detailed write up - I wasn't quite sure of the exact circumstances that trigger it, since in my case it involved an external signal handler that was rewriting register state. That did in effect lead to the instruction looping back on itself. On Tue, Apr 14, 2020, 12:01 Andrew Burgess <andrew.burgess@embecosm.com> wrote: > * Simon Marchi <simark@simark.ca> [2020-04-14 11:04:47 -0400]: > > > On 2020-04-14 3:01 a.m., Keno Fischer wrote: > > > Bump. It would be great to get this fixed. > > > > > > On Tue, Mar 31, 2020 at 9:38 PM Keno Fischer <keno@juliacomputing.com> > wrote: > > >> > > >> This patch fixes PR gdb/25741 "GDB tries to set breakpoint using Z0, > but remove it using z1". > > >> In particular, what occurs in that case is that a hardware breakpoint > is hit, > > >> after which GDB removes it and establishes a single step breakpoint > at the > > >> same location. Afterwards, rather than simply removing this > breakpoint and > > >> re-enabling the hardware breakpoint, GDB simply swaps the activation, > without > > >> informing the server, leading to an inconsistency in GDB's view of > the world > > >> and the server's view of the world. To remidy this situation, this > > >> patch adds a check that ensures two breakpoint locations have the > > >> same type before they are considered equal and thus eligible for > silent > > >> swapping. > > >> > > >> gdb/ChangeLog: > > >> * breakpoint.c (breakpoint_locations_match): Fix PR gdb/25741 > > >> > > >> Signed-off-by: Keno Fischer <keno@juliacomputing.com> > > >> --- > > >> gdb/breakpoint.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > > >> index e49025461b..582dae1946 100644 > > >> --- a/gdb/breakpoint.c > > >> +++ b/gdb/breakpoint.c > > >> @@ -6838,7 +6838,7 @@ breakpoint_locations_match (struct bp_location > *loc1, > > >> /* We compare bp_location.length in order to cover ranged > breakpoints. */ > > >> return (breakpoint_address_match (loc1->pspace->aspace, > loc1->address, > > >> loc2->pspace->aspace, > loc2->address) > > >> - && loc1->length == loc2->length); > > >> + && loc1->length == loc2->length && loc1->loc_type == > loc2->loc_type); > > >> } > > >> > > >> static void > > >> -- > > >> 2.24.0 > > >> > > > > I think the change makes sense, but this is not an area I know well, and > it's one that > > is a bit sensitive. I'll do a full test run and take a bit more time to > look at it. > > > > In the mean time, if anybody else wants to take a look, go for it. > > I was able to reproduce this issue with a hacked up RISC-V target > (forced software single step on for bare-metal) running on a > development board I have access too. > > I agree that this fix feels like the right thing to do, it's inline > with location checking done in watchpoint_locations_match. The only > question I had when comparing to watchpoint_locations_match, is > whether we should be similarly comparing the types of the owner > breakpoint rather than the actual location. However, the b/p type is > overloaded to contain more than just whether the breakpoint is h/w or > s/w, so in this case we have the user h/w breakpoint, its type is > bp_hardware_breakpoint, while the software single step breakpoint has > type bp_single_step. The conclusion I came to is that it really is > the type of the location we need to compare here. > > The other slight issue I had with the patch was that based on the > original description I still had to go and figure out the exact > conditions that would trigger this bug. I believe that to trigger > this you need a h/w breakpoint placed on an instruction that loops to > itself, I don't see how else this could happen. > > I took a crack at writing a more detailed break down of the conditions > that trigger this bug. > > I'm still running this through testing here, but I'd be interested to > hear if you think the fuller description is helpful. > > Thanks, > Andrew > > --- > > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 > From: Andrew Burgess <andrew.burgess@embecosm.com> > Date: Tue, 14 Apr 2020 16:32:02 +0100 > Subject: [PATCH] gdb: ensure location types match before swapping locations > > In the following conditions: > > - A target with hardware breakpoints available, and > - A target that uses software single stepping, > - An instruction at ADDRESS loops back to itself, > > Now consider the following steps: > > 1. The user places a hardware breakpoint at ADDRESS (an instruction > that loops to itself), > > 2. The inferior runs and hits the breakpoint from #1, > > 3. The user tells GDB to 'continue'. > > In #3 when the user tells GDB to continue, GDB first disables the > hardware breakpoint at ADDRESS, and then inserts a software single > step breakpoint at ADDRESS. The original user created breakpoint was > a hardware breakpoint, while the single step breakpoint will be a > software breakpoint. > > GDB continues and immediately hits the software single step > breakpoint. > > GDB then deletes the software single step breakpoint by calling > delete_single_step_breakpoints, which eventually calls > delete_breakpoint, which, once the breakpoint (and its locations) are > deleted, calls update_global_location_list. > > During update_global_location_list GDB spots that we have an old > location (the software single step breakpoint location) that is > inserted, but being deleted, and a location at the same address which > we are keeping, but which is not currently inserted (the original > hardware breakpoint location), GDB then calls > breakpoint_locations_match on these two locations. Currently the > locations do match, and so GDB calls swap_insertion which swaps the > "inserted" state of the two locations. GDB finally returns through > the call stack and leaves delete_single_step_breakpoints. After this > GDB continues with its normal "stopping" process, as part of this > stopping process GDB removes all the breakpoints from the target. Due > to the swap the original hardware breakpoint location is removed. > > The problem is that GDB inserted the software single step breakpoint > as a software breakpoint, and then, thanks to the swap, tries to > remove it as a hardware breakpoint. This will leave the target in an > inconsistent state, and as in the original bug report (PR gdb/25741), > could result in the target throwing an error. > > The solution for all this is to have two breakpoint locations of > different types (hardware breakpoint vs software breakpoint) not > match. The result of this is that GDB will no longer swap the > inserted status of the two breakpoints. The original call to > update_global_location_list (after deleting the software single step > breakpoint) will at this point trigger a call to remove the > breakpoint, something which will be done based on the type of that > location. Later GDB will see that the original hardware breakpoint is > already not inserted, and so will leave it alone. > > This patch was original proposed by Keno Fischer here: > > https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html > > gdb/ChangeLog: > > PR gdb/25741 > * breakpoint.c (breakpoint_locations_match): Compare location > types. > --- > gdb/ChangeLog | 6 ++++++ > gdb/breakpoint.c | 3 ++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index e49025461ba..2ab40a8e40a 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -6838,7 +6838,8 @@ breakpoint_locations_match (struct bp_location *loc1, > /* We compare bp_location.length in order to cover ranged > breakpoints. */ > return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, > loc2->pspace->aspace, loc2->address) > - && loc1->length == loc2->length); > + && loc1->length == loc2->length > + && loc1->loc_type == loc2->loc_type); > } > > static void > -- > 2.25.2 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-14 16:00 ` Andrew Burgess 2020-04-14 16:26 ` Keno Fischer @ 2020-04-14 19:17 ` Pedro Alves 2020-04-15 20:46 ` Andrew Burgess 2020-04-17 12:28 ` Andrew Burgess 1 sibling, 2 replies; 17+ messages in thread From: Pedro Alves @ 2020-04-14 19:17 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi; +Cc: Keno Fischer, gdb-patches On 4/14/20 5:00 PM, Andrew Burgess wrote: > The other slight issue I had with the patch was that based on the > original description I still had to go and figure out the exact > conditions that would trigger this bug. I believe that to trigger > this you need a h/w breakpoint placed on an instruction that loops to > itself, I don't see how else this could happen. > > I took a crack at writing a more detailed break down of the conditions > that trigger this bug. > > I'm still running this through testing here, but I'd be interested to > hear if you think the fuller description is helpful. It is. > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 > From: Andrew Burgess <andrew.burgess@embecosm.com> > Date: Tue, 14 Apr 2020 16:32:02 +0100 > Subject: [PATCH] gdb: ensure location types match before swapping locations > > In the following conditions: > > - A target with hardware breakpoints available, and > - A target that uses software single stepping, > - An instruction at ADDRESS loops back to itself, > > Now consider the following steps: > > 1. The user places a hardware breakpoint at ADDRESS (an instruction > that loops to itself), > > 2. The inferior runs and hits the breakpoint from #1, > > 3. The user tells GDB to 'continue'. > > In #3 when the user tells GDB to continue, GDB first disables the > hardware breakpoint at ADDRESS, and then inserts a software single > step breakpoint at ADDRESS. The original user created breakpoint was > a hardware breakpoint, while the single step breakpoint will be a > software breakpoint. > > GDB continues and immediately hits the software single step > breakpoint. > > GDB then deletes the software single step breakpoint by calling > delete_single_step_breakpoints, which eventually calls > delete_breakpoint, which, once the breakpoint (and its locations) are > deleted, calls update_global_location_list. > > During update_global_location_list GDB spots that we have an old > location (the software single step breakpoint location) that is > inserted, but being deleted, and a location at the same address which > we are keeping, but which is not currently inserted (the original > hardware breakpoint location), GDB then calls > breakpoint_locations_match on these two locations. Currently the > locations do match, and so GDB calls swap_insertion which swaps the > "inserted" state of the two locations. GDB finally returns through > the call stack and leaves delete_single_step_breakpoints. After this > GDB continues with its normal "stopping" process, as part of this > stopping process GDB removes all the breakpoints from the target. Due > to the swap the original hardware breakpoint location is removed. > > The problem is that GDB inserted the software single step breakpoint > as a software breakpoint, and then, thanks to the swap, tries to > remove it as a hardware breakpoint. This will leave the target in an > inconsistent state, and as in the original bug report (PR gdb/25741), > could result in the target throwing an error. > > The solution for all this is to have two breakpoint locations of > different types (hardware breakpoint vs software breakpoint) not > match. The result of this is that GDB will no longer swap the > inserted status of the two breakpoints. The original call to > update_global_location_list (after deleting the software single step > breakpoint) will at this point trigger a call to remove the > breakpoint, something which will be done based on the type of that > location. Later GDB will see that the original hardware breakpoint is > already not inserted, and so will leave it alone. > > This patch was original proposed by Keno Fischer here: > > https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html > > gdb/ChangeLog: > > PR gdb/25741 > * breakpoint.c (breakpoint_locations_match): Compare location > types. This seems right at first blush, though there are some things that we need to look at. Here are 3 cases that found while looking for breakpoint_locations_match callers: #1 E.g., with this, GDB will now install both a hw breakpoint location and a software location at the same address. E.g., a contrived case to see it happen would be, with: (gdb) set breakpoint always-inserted on (gdb) set debug remote 1 before: (gdb) hbreak main Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 2 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK after: (gdb) hbreak main Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) break main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 2 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK Sending packet: $Z0,400736,1#47...Packet received: OK This is likely not to cause a problem, but it's worth it to consider. #2 Another thing to consider is the automatic hardware breakpoints support. See insert_bp_location. That means that breakpoint locations can start as software breakpoints but still end up inserted as hw breakpoints. Similarly to #1 above, without the patch, gdb is considering those locations as duplicates, and after the patch it no longer will. So we may end up with multiple insertions for the same location. Again, similar to #1. #3 I suspect this the (automatic hardware breakpoints) can interfere with e.g., update_breakpoint_locations 's logic of matching old locations with new locations, in the have_ambiguous_names case. I.e., after insertion the locations are hw breakpoint locations, but after a breakpoint re-set, the new locations will be software breakpoint locations until they try to be inserted. Before the patch, the disabled state will still be carried over. After the patch, they won't. I think. Maybe we need to explicitly consider the case in update_breakpoint_locations. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-14 19:17 ` Pedro Alves @ 2020-04-15 20:46 ` Andrew Burgess 2020-04-17 12:28 ` Andrew Burgess 1 sibling, 0 replies; 17+ messages in thread From: Andrew Burgess @ 2020-04-15 20:46 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, Keno Fischer, gdb-patches * Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]: > On 4/14/20 5:00 PM, Andrew Burgess wrote: > > > The other slight issue I had with the patch was that based on the > > original description I still had to go and figure out the exact > > conditions that would trigger this bug. I believe that to trigger > > this you need a h/w breakpoint placed on an instruction that loops to > > itself, I don't see how else this could happen. > > > > I took a crack at writing a more detailed break down of the conditions > > that trigger this bug. > > > > > I'm still running this through testing here, but I'd be interested to > > hear if you think the fuller description is helpful. > > It is. > > > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 > > From: Andrew Burgess <andrew.burgess@embecosm.com> > > Date: Tue, 14 Apr 2020 16:32:02 +0100 > > Subject: [PATCH] gdb: ensure location types match before swapping locations > > > > In the following conditions: > > > > - A target with hardware breakpoints available, and > > - A target that uses software single stepping, > > - An instruction at ADDRESS loops back to itself, > > > > Now consider the following steps: > > > > 1. The user places a hardware breakpoint at ADDRESS (an instruction > > that loops to itself), > > > > 2. The inferior runs and hits the breakpoint from #1, > > > > 3. The user tells GDB to 'continue'. > > > > In #3 when the user tells GDB to continue, GDB first disables the > > hardware breakpoint at ADDRESS, and then inserts a software single > > step breakpoint at ADDRESS. The original user created breakpoint was > > a hardware breakpoint, while the single step breakpoint will be a > > software breakpoint. > > > > GDB continues and immediately hits the software single step > > breakpoint. > > > > GDB then deletes the software single step breakpoint by calling > > delete_single_step_breakpoints, which eventually calls > > delete_breakpoint, which, once the breakpoint (and its locations) are > > deleted, calls update_global_location_list. > > > > During update_global_location_list GDB spots that we have an old > > location (the software single step breakpoint location) that is > > inserted, but being deleted, and a location at the same address which > > we are keeping, but which is not currently inserted (the original > > hardware breakpoint location), GDB then calls > > breakpoint_locations_match on these two locations. Currently the > > locations do match, and so GDB calls swap_insertion which swaps the > > "inserted" state of the two locations. GDB finally returns through > > the call stack and leaves delete_single_step_breakpoints. After this > > GDB continues with its normal "stopping" process, as part of this > > stopping process GDB removes all the breakpoints from the target. Due > > to the swap the original hardware breakpoint location is removed. > > > > The problem is that GDB inserted the software single step breakpoint > > as a software breakpoint, and then, thanks to the swap, tries to > > remove it as a hardware breakpoint. This will leave the target in an > > inconsistent state, and as in the original bug report (PR gdb/25741), > > could result in the target throwing an error. > > > > The solution for all this is to have two breakpoint locations of > > different types (hardware breakpoint vs software breakpoint) not > > match. The result of this is that GDB will no longer swap the > > inserted status of the two breakpoints. The original call to > > update_global_location_list (after deleting the software single step > > breakpoint) will at this point trigger a call to remove the > > breakpoint, something which will be done based on the type of that > > location. Later GDB will see that the original hardware breakpoint is > > already not inserted, and so will leave it alone. > > > > This patch was original proposed by Keno Fischer here: > > > > https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html > > > > gdb/ChangeLog: > > > > PR gdb/25741 > > * breakpoint.c (breakpoint_locations_match): Compare location > > types. > > This seems right at first blush, though there are some things > that we need to look at. Here are 3 cases that found while > looking for breakpoint_locations_match callers: > > #1 > > E.g., with this, GDB will now install both a hw breakpoint location > and a software location at the same address. E.g., a contrived case > to see it happen would be, with: > > (gdb) set breakpoint always-inserted on > (gdb) set debug remote 1 > > before: > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) b main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > > after: > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) break main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > Sending packet: $Z0,400736,1#47...Packet received: OK > > This is likely not to cause a problem, but it's worth it to consider. > > #2 > > Another thing to consider is the automatic hardware breakpoints > support. See insert_bp_location. That means that breakpoint > locations can start as software breakpoints but still end up > inserted as hw breakpoints. Similarly to #1 above, without the patch, > gdb is considering those locations as duplicates, and after the > patch it no longer will. So we may end up with multiple insertions > for the same location. Again, similar to #1. > > #3 > > I suspect this the (automatic hardware breakpoints) can interfere with > e.g., update_breakpoint_locations 's logic of matching old locations > with new locations, in the have_ambiguous_names case. I.e., after > insertion the locations are hw breakpoint locations, but after > a breakpoint re-set, the new locations will be software breakpoint > locations until they try to be inserted. Before the patch, the > disabled state will still be carried over. After the patch, they won't. > I think. > > Maybe we need to explicitly consider the case in > update_breakpoint_locations. I'll take another look at this code, but it sounds almost like we should split breakpoint_locations_match in two, we have: 1. match (v1) - two breakpoints at the same address match regardless of type, this would be used when deciding if we should insert bp_location 1, 2, or both. 2. match (v2) - when one breakpoint is already inserted, can another breakpoint "take over" its inserted status. It's almost as though we want: bool breakpoint_locations_swapable ( .... ) { return (breakpoint_locations_match ( .... ) && loc1->type == loc2->type); } Maybe? I'll take another look at this when I can. Thanks, Andrew ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-14 19:17 ` Pedro Alves 2020-04-15 20:46 ` Andrew Burgess @ 2020-04-17 12:28 ` Andrew Burgess 2020-04-17 17:22 ` Pedro Alves 2020-04-19 18:21 ` Pedro Alves 1 sibling, 2 replies; 17+ messages in thread From: Andrew Burgess @ 2020-04-17 12:28 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, Keno Fischer, gdb-patches * Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]: > On 4/14/20 5:00 PM, Andrew Burgess wrote: > > > The other slight issue I had with the patch was that based on the > > original description I still had to go and figure out the exact > > conditions that would trigger this bug. I believe that to trigger > > this you need a h/w breakpoint placed on an instruction that loops to > > itself, I don't see how else this could happen. > > > > I took a crack at writing a more detailed break down of the conditions > > that trigger this bug. > > > > > I'm still running this through testing here, but I'd be interested to > > hear if you think the fuller description is helpful. > > It is. > > > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 > > From: Andrew Burgess <andrew.burgess@embecosm.com> > > Date: Tue, 14 Apr 2020 16:32:02 +0100 > > Subject: [PATCH] gdb: ensure location types match before swapping locations > > > > In the following conditions: > > > > - A target with hardware breakpoints available, and > > - A target that uses software single stepping, > > - An instruction at ADDRESS loops back to itself, > > > > Now consider the following steps: > > > > 1. The user places a hardware breakpoint at ADDRESS (an instruction > > that loops to itself), > > > > 2. The inferior runs and hits the breakpoint from #1, > > > > 3. The user tells GDB to 'continue'. > > > > In #3 when the user tells GDB to continue, GDB first disables the > > hardware breakpoint at ADDRESS, and then inserts a software single > > step breakpoint at ADDRESS. The original user created breakpoint was > > a hardware breakpoint, while the single step breakpoint will be a > > software breakpoint. > > > > GDB continues and immediately hits the software single step > > breakpoint. > > > > GDB then deletes the software single step breakpoint by calling > > delete_single_step_breakpoints, which eventually calls > > delete_breakpoint, which, once the breakpoint (and its locations) are > > deleted, calls update_global_location_list. > > > > During update_global_location_list GDB spots that we have an old > > location (the software single step breakpoint location) that is > > inserted, but being deleted, and a location at the same address which > > we are keeping, but which is not currently inserted (the original > > hardware breakpoint location), GDB then calls > > breakpoint_locations_match on these two locations. Currently the > > locations do match, and so GDB calls swap_insertion which swaps the > > "inserted" state of the two locations. GDB finally returns through > > the call stack and leaves delete_single_step_breakpoints. After this > > GDB continues with its normal "stopping" process, as part of this > > stopping process GDB removes all the breakpoints from the target. Due > > to the swap the original hardware breakpoint location is removed. > > > > The problem is that GDB inserted the software single step breakpoint > > as a software breakpoint, and then, thanks to the swap, tries to > > remove it as a hardware breakpoint. This will leave the target in an > > inconsistent state, and as in the original bug report (PR gdb/25741), > > could result in the target throwing an error. > > > > The solution for all this is to have two breakpoint locations of > > different types (hardware breakpoint vs software breakpoint) not > > match. The result of this is that GDB will no longer swap the > > inserted status of the two breakpoints. The original call to > > update_global_location_list (after deleting the software single step > > breakpoint) will at this point trigger a call to remove the > > breakpoint, something which will be done based on the type of that > > location. Later GDB will see that the original hardware breakpoint is > > already not inserted, and so will leave it alone. > > > > This patch was original proposed by Keno Fischer here: > > > > https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html > > > > gdb/ChangeLog: > > > > PR gdb/25741 > > * breakpoint.c (breakpoint_locations_match): Compare location > > types. > > This seems right at first blush, though there are some things > that we need to look at. Here are 3 cases that found while > looking for breakpoint_locations_match callers: > > #1 > > E.g., with this, GDB will now install both a hw breakpoint location > and a software location at the same address. E.g., a contrived case > to see it happen would be, with: > > (gdb) set breakpoint always-inserted on > (gdb) set debug remote 1 > > before: > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) b main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > > after: > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) break main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > Sending packet: $Z0,400736,1#47...Packet received: OK While working on a revised patch I tried to reproduce this, and it's most odd. Notice that both before and after you have two Z1 packets, you're inserting the hardware breakpoint twice with no intermediate removal. I don't see this behaviour, even on an unmodified GDB, and I know some remove targets, for example OpenOCD, will sanity check against such double insertions and throw an error. Just though this was worth mentioning. Anyway, here's a revised patch. I've moved the location of the type check so it is only done now for the swapping case. This should resolve all the concerns you raised, while still addressing the original issue. I updated the commit message a bit too. What do you think? Thanks, Andrew --- commit 10d62976088600ee39b9b828bae93f82a44a2974 Author: Andrew Burgess <andrew.burgess@embecosm.com> Date: Tue Apr 14 16:32:02 2020 +0100 gdb: ensure location types match before swapping locations In the following conditions: - A target with hardware breakpoints available, and - A target that uses software single stepping, - An instruction at ADDRESS loops back to itself, Now consider the following steps: 1. The user places a hardware breakpoint at ADDRESS (an instruction that loops to itself), 2. The inferior runs and hits the breakpoint at ADDRESS, 3. The user tells GDB to 'continue'. In #3 when the user tells GDB to continue, GDB first disables the hardware breakpoint at ADDRESS, and then inserts a software single step breakpoint at ADDRESS. The original user created breakpoint was a hardware breakpoint, while the single step breakpoint will be a software breakpoint. GDB continues and immediately hits the software single step breakpoint. GDB then deletes the software single step breakpoint by calling delete_single_step_breakpoints, which eventually calls delete_breakpoint, which, once the breakpoint (and its locations) are deleted, calls update_global_location_list. During update_global_location_list GDB spots that we have an old location (the software single step breakpoint location) that is inserted, but being deleted, and a location (the original hardware breakpoint) at the same address which we are keeping, but which is not currently inserted, GDB then calls breakpoint_locations_match on these two locations. Currently the locations do match, and so GDB calls swap_insertion which swaps the "inserted" state of the two locations. The user created hardware breakpoint is marked as inserted, while the GDB internal software single step breakpoint is now marked as not inserted. After this GDB returns through the call stack and leaves delete_single_step_breakpoints. After this GDB continues with its normal "stopping" process, as part of this stopping process GDB removes all the breakpoints from the target. Due to the swap it is now the user created hardware breakpoint that is marked as inserted, so it is this breakpoint GDB tries to remove. The problem is that GDB inserted the software single step breakpoint as a software breakpoint, but is now trying to remove the hardware breakpoint. The problem is removing a software breakpoint is very different to removing a hardware breakpoint, this could result is some undetected undefined behaviour, or as in the original bug report (PR gdb/25741), could result in the target throwing an error. This original patch proposed by Keno Fischer is here: https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html This patch modified breakpoint_locations_match so that two breakpoints of different types would no longer match. This resolves the immediate problem described above, but introduces some other issues. The breakpoint_locations_match function is also used to control whether two breakpoints places at the same address should both be inserted or not. Consider this (slightly contrived) use case: (gdb) set debug remote 1 (gdb) set breakpoint always-inserted on (gdb) hbreak MyFunc0 Sending packet: $me0000202,2#84...Packet received: 0000 Hardware assisted breakpoint 3 at 0xe0000202: file test.c, line 288. Sending packet: $m80000188,1#63...Packet received: 00 Sending packet: $Z1,e0000202,2#ce...Packet received: OK (gdb) break MyFunc0 Note: breakpoint 3 also set at pc 0xe0000202. Sending packet: $me0000202,2#84...Packet received: 0000 Breakpoint 4 at 0xe0000202: file test.c, line 288. Sending packet: $m80000188,1#63...Packet received: 00 Notice after setting the hardware breakpoint we see the Z1 packet sent, but after setting the software breakpoint there's no Z0 packet, GDB sees that the locations match and doesn't make the second insertion. If we modify breakpoint_locations_match then we would insert both a hardware _and_ software breakpoint. This probably doesn't matter, but is not ideal, so a solution that doesn't do this would be better. The proposal here is to focus specifically on the case where we are considering swapping the inserted status of two breakpoint locations, and moves the location type check out of breakpoint_locations_match, and into the caller. This resolves the original issue, while avoiding the double insertion problem. gdb/ChangeLog: PR gdb/25741 * breakpoint.c (breakpoint_locations_match): Compare location types. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..afd6459a634 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode) gdb_assert (is_hardware_watchpoint (loc2->owner)); loc2->watchpoint_type = old_loc->watchpoint_type; } + else if (is_breakpoint (old_loc->owner)) + { + /* If we swap the inserted status of a + hardware and software breakpoint then GDB + will insert the breakpoint as one type, + and later try to remove the breakpoint as + the other type. This is not good. */ + gdb_assert (is_breakpoint (loc2->owner)); + if (old_loc->loc_type != loc2->loc_type) + continue; + } /* loc2 is a duplicated location. We need to check if it should be inserted in case it will be ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-17 12:28 ` Andrew Burgess @ 2020-04-17 17:22 ` Pedro Alves 2020-04-19 18:21 ` Pedro Alves 1 sibling, 0 replies; 17+ messages in thread From: Pedro Alves @ 2020-04-17 17:22 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, Keno Fischer, gdb-patches On 4/17/20 1:28 PM, Andrew Burgess wrote: > * Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]: > >> On 4/14/20 5:00 PM, Andrew Burgess wrote: >> >>> The other slight issue I had with the patch was that based on the >>> original description I still had to go and figure out the exact >>> conditions that would trigger this bug. I believe that to trigger >>> this you need a h/w breakpoint placed on an instruction that loops to >>> itself, I don't see how else this could happen. >>> >>> I took a crack at writing a more detailed break down of the conditions >>> that trigger this bug. >>> >> >>> I'm still running this through testing here, but I'd be interested to >>> hear if you think the fuller description is helpful. >> >> It is. >> >>> From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001 >>> From: Andrew Burgess <andrew.burgess@embecosm.com> >>> Date: Tue, 14 Apr 2020 16:32:02 +0100 >>> Subject: [PATCH] gdb: ensure location types match before swapping locations >>> >>> In the following conditions: >>> >>> - A target with hardware breakpoints available, and >>> - A target that uses software single stepping, >>> - An instruction at ADDRESS loops back to itself, >>> >>> Now consider the following steps: >>> >>> 1. The user places a hardware breakpoint at ADDRESS (an instruction >>> that loops to itself), >>> >>> 2. The inferior runs and hits the breakpoint from #1, >>> >>> 3. The user tells GDB to 'continue'. >>> >>> In #3 when the user tells GDB to continue, GDB first disables the >>> hardware breakpoint at ADDRESS, and then inserts a software single >>> step breakpoint at ADDRESS. The original user created breakpoint was >>> a hardware breakpoint, while the single step breakpoint will be a >>> software breakpoint. >>> >>> GDB continues and immediately hits the software single step >>> breakpoint. >>> >>> GDB then deletes the software single step breakpoint by calling >>> delete_single_step_breakpoints, which eventually calls >>> delete_breakpoint, which, once the breakpoint (and its locations) are >>> deleted, calls update_global_location_list. >>> >>> During update_global_location_list GDB spots that we have an old >>> location (the software single step breakpoint location) that is >>> inserted, but being deleted, and a location at the same address which >>> we are keeping, but which is not currently inserted (the original >>> hardware breakpoint location), GDB then calls >>> breakpoint_locations_match on these two locations. Currently the >>> locations do match, and so GDB calls swap_insertion which swaps the >>> "inserted" state of the two locations. GDB finally returns through >>> the call stack and leaves delete_single_step_breakpoints. After this >>> GDB continues with its normal "stopping" process, as part of this >>> stopping process GDB removes all the breakpoints from the target. Due >>> to the swap the original hardware breakpoint location is removed. >>> >>> The problem is that GDB inserted the software single step breakpoint >>> as a software breakpoint, and then, thanks to the swap, tries to >>> remove it as a hardware breakpoint. This will leave the target in an >>> inconsistent state, and as in the original bug report (PR gdb/25741), >>> could result in the target throwing an error. >>> >>> The solution for all this is to have two breakpoint locations of >>> different types (hardware breakpoint vs software breakpoint) not >>> match. The result of this is that GDB will no longer swap the >>> inserted status of the two breakpoints. The original call to >>> update_global_location_list (after deleting the software single step >>> breakpoint) will at this point trigger a call to remove the >>> breakpoint, something which will be done based on the type of that >>> location. Later GDB will see that the original hardware breakpoint is >>> already not inserted, and so will leave it alone. >>> >>> This patch was original proposed by Keno Fischer here: >>> >>> https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html >>> >>> gdb/ChangeLog: >>> >>> PR gdb/25741 >>> * breakpoint.c (breakpoint_locations_match): Compare location >>> types. >> >> This seems right at first blush, though there are some things >> that we need to look at. Here are 3 cases that found while >> looking for breakpoint_locations_match callers: >> >> #1 >> >> E.g., with this, GDB will now install both a hw breakpoint location >> and a software location at the same address. E.g., a contrived case >> to see it happen would be, with: >> >> (gdb) set breakpoint always-inserted on >> (gdb) set debug remote 1 >> >> before: >> >> (gdb) hbreak main >> Sending packet: $m400736,1#fe...Packet received: 48 >> Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. >> Sending packet: $Z1,400736,1#48...Packet received: OK >> (gdb) b main >> Note: breakpoint 1 also set at pc 0x400736. >> Sending packet: $m400736,1#fe...Packet received: 48 >> Breakpoint 2 at 0x400736: file threads.c, line 57. >> Sending packet: $Z1,400736,1#48...Packet received: OK >> >> after: >> >> (gdb) hbreak main >> Sending packet: $m400736,1#fe...Packet received: 48 >> Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. >> Sending packet: $Z1,400736,1#48...Packet received: OK >> (gdb) break main >> Note: breakpoint 1 also set at pc 0x400736. >> Sending packet: $m400736,1#fe...Packet received: 48 >> Breakpoint 2 at 0x400736: file threads.c, line 57. >> Sending packet: $Z1,400736,1#48...Packet received: OK >> Sending packet: $Z0,400736,1#47...Packet received: OK > > While working on a revised patch I tried to reproduce this, and it's > most odd. Notice that both before and after you have two Z1 packets, > you're inserting the hardware breakpoint twice with no intermediate > removal. That happens because when a software breakpoint location is created, it is created with condition_changed == condition_modified. Then, in update_global_location_list, we end up in force_breakpoint_reinsertion: /* Check if this is a new/duplicated location or a duplicated location that had its condition modified. If so, we want to send its condition to the target if evaluation of conditions is taking place there. */ if ((*loc2p)->condition_changed == condition_modified && (last_addr != old_loc->address || last_pspace_num != old_loc->pspace->num)) { force_breakpoint_reinsertion (*loc2p); last_pspace_num = old_loc->pspace->num; } force_breakpoint_reinsertion forces reinsertion of all breakpoint locations at the same address. > > I don't see this behaviour, even on an unmodified GDB, I was debugging against pristine master x86-64 gdbserver. And: (gdb) show breakpoint condition-evaluation Breakpoint condition evaluation mode is auto (currently target). Maybe you tested against a different remote server which doesn't support target-side condition evaluation? > and I know some remove targets, for example OpenOCD, will sanity check against such > double insertions and throw an error. If they do that, they should be fixed. z/Z packets are supposed to be idempotent. Double insertions are to be expected and should not cause an error. @emph{Implementation notes: A remote target shall return an empty string for an unrecognized breakpoint or watchpoint packet @var{type}. A remote target shall support either both or neither of a given @samp{Z@var{type}@dots{}} and @samp{z@var{type}@dots{}} packet pair. To avoid potential problems with duplicate packets, the operations should ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ be implemented in an idempotent way.} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We took advantage of that when we added support for target-side breakpoint conditions. > > Just though this was worth mentioning. > > Anyway, here's a revised patch. I've moved the location of the type > check so it is only done now for the swapping case. This should > resolve all the concerns you raised, while still addressing the > original issue. I updated the commit message a bit too. > > What do you think? Let me try to find a bit of time to give this a think. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] breakpoint: Make sure location types match before swapping 2020-04-17 12:28 ` Andrew Burgess 2020-04-17 17:22 ` Pedro Alves @ 2020-04-19 18:21 ` Pedro Alves 2020-04-19 18:49 ` [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) Pedro Alves 1 sibling, 1 reply; 17+ messages in thread From: Pedro Alves @ 2020-04-19 18:21 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, Keno Fischer, gdb-patches On 4/17/20 1:28 PM, Andrew Burgess wrote: > @@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > gdb_assert (is_hardware_watchpoint (loc2->owner)); > loc2->watchpoint_type = old_loc->watchpoint_type; > } > + else if (is_breakpoint (old_loc->owner)) > + { > + /* If we swap the inserted status of a > + hardware and software breakpoint then GDB > + will insert the breakpoint as one type, > + and later try to remove the breakpoint as > + the other type. This is not good. */ > + gdb_assert (is_breakpoint (loc2->owner)); > + if (old_loc->loc_type != loc2->loc_type) > + continue; > + } Unfortunately this doesn't actually work correctly. For example: $ gdb ~/gdb/tests/threads -ex "set breakpoint always-inserted on" -ex "tar rem :9999" -ex "set debug remote 1" ... (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b... Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 2 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK Note no Z0 was sent after "b main". That's because the sw and hw breakpoints are still considered duplicates. But now, let's delete the hw breakpoint: (gdb) del 1 Sending packet: $z1,400736,1#68...Packet received: OK Note z1 (remove hw break) was sent, but Z0 (insert sw break) was not. At this point, the target side has no breakpoint installed! That is very wrong, because we have "set breakpoint always-inserted on". If the target was running at this point (e.g., non-stop mode), even with "set breakpoint always-inserted off", GDB should keep breakpoints installed, otherwise threads would miss the breakpoint. "set breakpoint always-inserted on" is just a handy way to emulate that. If we do any other operation that forces installing breakpoints, then finally the Z0 is sent: (gdb) b main Note: breakpoint 2 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 3 at 0x400736: file threads.c, line 57. Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) I spent a good while yesterday trying to make this all work with GDB still considering hw breakpoints and sw breakpoints as duplicates. We also need to handle my concern about insert_bp_location changing the type of breakpoint locations from sw breakpoint to hw breakpoint, _after_ the update_global_location_list sorted out which breakpoints are duplicates of which. The result is that GDB behaves like this: (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b... Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 2 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) del 1 Sending packet: $Z0,400736,1#47...Packet received: OK Sending packet: $z1,400736,1#68...Packet received: OK Sending packet: $Z0,400736,1#47...Packet received: OK Note how after deleting breakpoint 1 (the hw break), GDB first installs the sw breakpoint, and then deletes the hw breakpoint. This order is important to ensures that at no moment could a thread miss breakpoint 2. The final Z0, is an artifact of the target-side condition handling. We could get rid of that but I didn't try it. It was only after all the effort that I realized that it's pointless to try to make hw and sw breakpoint locations match _if we always need to install the new sw break before deleting the hw break_. I mean, we go through all that effort, but then there's always going to be a small window where the remote target needs to be able to handle a sw breakpoint and a hw breakpoint at the same address anyway (e.g., handle target-side breakpoint conditions correctly, handle target-side commands correctly, etc.) So the patch below is the one I wrote yesterday. It wasn't finished, but was well advanced. I'm posting it for the archives and for discussion. At this point, I don't believe we should follow this patch's approach. I'll send another reply with today's new patch. From e2fe5095cc2e136dadbec373ebaed2938e282145 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Sat, 18 Apr 2020 23:28:46 +0100 Subject: [PATCH] Handle hw and sw breakpoints at the same address --- gdb/breakpoint.c | 314 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 236 insertions(+), 78 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..25ce3b9d217 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2405,6 +2405,8 @@ breakpoint_kind (struct bp_location *bl, CORE_ADDR *addr) return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr); } +static void handle_automatic_hardware_breakpoints (bp_location *bl); + /* Insert a low-level "breakpoint" of some type. BL is the breakpoint location. Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems. @@ -2454,69 +2456,6 @@ insert_bp_location (struct bp_location *bl, if (bl->loc_type == bp_loc_software_breakpoint || bl->loc_type == bp_loc_hardware_breakpoint) { - if (bl->owner->type != bp_hardware_breakpoint) - { - /* If the explicitly specified breakpoint type - is not hardware breakpoint, check the memory map to see - if the breakpoint address is in read only memory or not. - - Two important cases are: - - location type is not hardware breakpoint, memory - is readonly. We change the type of the location to - hardware breakpoint. - - location type is hardware breakpoint, memory is - read-write. This means we've previously made the - location hardware one, but then the memory map changed, - so we undo. - - When breakpoints are removed, remove_breakpoints will use - location types we've just set here, the only possible - problem is that memory map has changed during running - program, but it's not going to work anyway with current - gdb. */ - struct mem_region *mr - = lookup_mem_region (bl->target_info.reqstd_address); - - if (mr) - { - if (automatic_hardware_breakpoints) - { - enum bp_loc_type new_type; - - if (mr->attrib.mode != MEM_RW) - new_type = bp_loc_hardware_breakpoint; - else - new_type = bp_loc_software_breakpoint; - - if (new_type != bl->loc_type) - { - static int said = 0; - - bl->loc_type = new_type; - if (!said) - { - fprintf_filtered (gdb_stdout, - _("Note: automatically using " - "hardware breakpoints for " - "read-only addresses.\n")); - said = 1; - } - } - } - else if (bl->loc_type == bp_loc_software_breakpoint - && mr->attrib.mode != MEM_RW) - { - fprintf_unfiltered (tmp_error_stream, - _("Cannot insert breakpoint %d.\n" - "Cannot set software breakpoint " - "at read-only address %s\n"), - bl->owner->number, - paddress (bl->gdbarch, bl->address)); - return 1; - } - } - } - /* First check to see if we have to handle an overlay. */ if (overlay_debugging == ovly_off || bl->section == NULL @@ -2830,7 +2769,11 @@ insert_breakpoints (void) /* Updating watchpoints creates new locations, so update the global location list. Explicitly tell ugll to insert locations and - ignore breakpoints_always_inserted_mode. */ + ignore breakpoints_always_inserted_mode. Also, + update_global_location_list tries to "upgrade" software + breakpoints to hardware breakpoints to handle "set breakpoint + auto-hw", so we need to call it even if we don't have new + locations. */ update_global_location_list (UGLL_INSERT); } @@ -2905,6 +2848,22 @@ update_inserted_breakpoint_locations (void) } } +static void +breakpoint_error_stream (string_file *tmp_error_stream, + int *hw_breakpoint_error, + int *hw_bp_error_explained_already) +{ + /* If a hardware breakpoint or watchpoint was inserted, add a + message about possibly exhausted resources. */ + if (*hw_breakpoint_error && !*hw_bp_error_explained_already) + { + tmp_error_stream->printf ("Could not insert hardware breakpoints:\n\ +You may have requested too many hardware breakpoints/watchpoints.\n"); + } + target_terminal::ours_for_output (); + error_stream (*tmp_error_stream); +} + /* Used when starting or continuing the program. */ static void @@ -2991,17 +2950,9 @@ insert_breakpoint_locations (void) } if (error_flag) - { - /* If a hardware breakpoint or watchpoint was inserted, add a - message about possibly exhausted resources. */ - if (hw_breakpoint_error && !hw_bp_error_explained_already) - { - tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\ -You may have requested too many hardware breakpoints/watchpoints.\n"); - } - target_terminal::ours_for_output (); - error_stream (tmp_error_stream); - } + breakpoint_error_stream (&tmp_error_stream, + &hw_breakpoint_error, + &hw_bp_error_explained_already); } /* Used when the program stops. @@ -8515,8 +8466,93 @@ mention (struct breakpoint *b) } \f +static int +bp_location_number (bp_location *loc) +{ + int n = 1; + for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next) + { + if (l == loc) + return n; + ++n; + } + + gdb_assert_not_reached ("breakpoint location not found in owner breakpoint"); +} + static bool bp_loc_is_permanent (struct bp_location *loc); +static void +handle_automatic_hardware_breakpoints (bp_location *bl) +{ + if (bl->loc_type == bp_loc_software_breakpoint + || bl->loc_type == bp_loc_hardware_breakpoint) + { + if (bl->owner->type != bp_hardware_breakpoint) + { + /* If the explicitly specified breakpoint type is not + hardware breakpoint, check the memory map to see if the + breakpoint address is in read only memory or not. + + Two important cases are: + - location type is not hardware breakpoint, memory is + readonly. We change the type of the location to hardware + breakpoint. + - location type is hardware breakpoint, memory is + read-write. This means we've previously made the + location hardware one, but then the memory map changed, + so we undo. + + When breakpoints are removed, remove_breakpoints will use + location types we've just set here, the only possible + problem is that memory map has changed during running + program, but it's not going to work anyway with current + gdb. */ + struct mem_region *mr + = lookup_mem_region (bl->target_info.reqstd_address); + + if (mr) + { + if (automatic_hardware_breakpoints) + { + enum bp_loc_type new_type; + + if (mr->attrib.mode != MEM_RW) + new_type = bp_loc_hardware_breakpoint; + else + new_type = bp_loc_software_breakpoint; + + if (new_type != bl->loc_type) + { + static int said = 0; + + bl->loc_type = new_type; + if (!said) + { + fprintf_filtered (gdb_stdout, + _("Note: automatically using " + "hardware breakpoints for " + "read-only addresses.\n")); + said = 1; + } + } + } + else if (bl->loc_type == bp_loc_software_breakpoint + && mr->attrib.mode != MEM_RW) + { + bl->enabled = false; + + warning (_("Breakpoint %d.%d disabled.\n" + "Cannot set software breakpoint " + "at read-only address %s\n"), + bl->owner->number, bp_location_number (bl), + paddress (bl->gdbarch, bl->address)); + } + } + } + } +} + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -11451,6 +11487,29 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) if (a->permanent != b->permanent) return a->permanent > b->permanent; + /* Sort hardware breakpoint locations before software breakpoint + locations, in order to prefer inserting hardware breakpoint + locations. */ + auto type_order = [](bp_loc_type loc_type) + { + switch (loc_type) + { + case bp_loc_hardware_breakpoint: + return 0; + case bp_loc_software_breakpoint: + return 1; + case bp_loc_hardware_watchpoint: + return 2; + case bp_loc_other: + return 3; + } + + gdb_assert_not_reached ("bad switch"); + }; + + if (type_order (a->loc_type) < type_order (b->loc_type)) + return true; + /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of the same type at the same address can be sorted in arbitrary order. */ @@ -11625,6 +11684,7 @@ force_breakpoint_reinsertion (struct bp_location *bl) loc->cond_bytecode.reset (); } } + /* Called whether new breakpoints are created, or existing breakpoints deleted, to update the global location list and recompute which locations are duplicate of which. @@ -11654,6 +11714,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode) struct bp_location *awp_loc_first; /* access watchpoint */ struct bp_location *rwp_loc_first; /* read watchpoint */ + /* Used for the case we need to insert a sw breakpoint to replace a + hw breakpoint, and vice versa. */ + bool error_flag = false; + int disabled_breaks = 0; + int hw_breakpoint_error = 0; + int hw_bp_error_explained_already = 0; + string_file tmp_error_stream; + /* Explicitly mark the warning -- this will only + be printed if there was an error. */ + tmp_error_stream.puts ("Warning:\n"); + /* Saved former bp_locations array which we compare against the newly built bp_locations from the current state of ALL_BREAKPOINTS. */ struct bp_location **old_locp; @@ -11673,6 +11744,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode) ALL_BREAKPOINTS (b) for (loc = b->loc; loc; loc = loc->next) *locp++ = loc; + + /* See if we need to "upgrade" a software breakpoint to a hardware + breakpoint. Do this before deciding whether locations are + duplicates. Also do this before sorting because sorting order + depends on location type. */ + for (locp = bp_locations; + locp < bp_locations + bp_locations_count; + locp++) + { + loc = *locp; + if (!loc->inserted) + handle_automatic_hardware_breakpoints (loc); + } + std::sort (bp_locations, bp_locations + bp_locations_count, bp_location_is_less_than); @@ -11769,6 +11854,8 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* OLD_LOC comes from existing struct breakpoint. */ if (bl_address_is_meaningful (old_loc)) { + bp_location *replacement_loc = nullptr; + for (loc2p = locp; (loc2p < bp_locations + bp_locations_count && (*loc2p)->address == old_loc->address); @@ -11776,6 +11863,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode) { struct bp_location *loc2 = *loc2p; + if (loc2 == old_loc) + continue; + if (breakpoint_locations_match (loc2, old_loc)) { /* Read watchpoint locations are switched to @@ -11786,12 +11876,25 @@ update_global_location_list (enum ugll_insert_mode insert_mode) gdb_assert (is_hardware_watchpoint (loc2->owner)); loc2->watchpoint_type = old_loc->watchpoint_type; } + else if ((old_loc->loc_type == bp_loc_hardware_breakpoint + || old_loc->loc_type == bp_loc_software_breakpoint) + && loc2->loc_type != old_loc->loc_type) + { + /* A hardware breakpoint is being + removed. Don't consider software + breakpoint locations as duplicates, + to avoid consuming hardware resources + unnecessarily without user + request. */ + if (unduplicated_should_be_inserted (loc2)) + replacement_loc = loc2; + continue; + } /* loc2 is a duplicated location. We need to check if it should be inserted in case it will be unduplicated. */ - if (loc2 != old_loc - && unduplicated_should_be_inserted (loc2)) + if (unduplicated_should_be_inserted (loc2)) { swap_insertion (old_loc, loc2); keep_in_target = 1; @@ -11799,6 +11902,29 @@ update_global_location_list (enum ugll_insert_mode insert_mode) } } } + + /* If we're deleting a harware breakpoint, and we + have a replacement software breakpoint to install + (or vice versa), we must install the new one + before deleting the old one, so that the inferior + doesn't miss a breakpoint in case it is running + right now. */ + if (!keep_in_target && replacement_loc != nullptr) + { + scoped_restore_current_pspace_and_thread restore_pspace_thread; + + switch_to_program_space_and_thread (replacement_loc->pspace); + + replacement_loc->duplicate = false; + + int val + = insert_bp_location (replacement_loc, &tmp_error_stream, + &disabled_breaks, + &hw_breakpoint_error, + &hw_bp_error_explained_already); + if (val) + error_flag = val; + } } } @@ -11959,13 +12085,45 @@ update_global_location_list (enum ugll_insert_mode insert_mode) is not duplicated, and is the inserted one. All following are marked as duplicated, and are not inserted. */ if (loc->inserted) - swap_insertion (loc, *loc_first_p); + { + if (loc->loc_type == bp_loc_software_breakpoint + && (*loc_first_p)->loc_type == bp_loc_hardware_breakpoint) + { + /* Insert the replacement breakpoint first, so that + there's never a time the inferior could miss a + breakpoint, in case the inferior is running right + now. */ + scoped_restore_current_pspace_and_thread restore_pspace_thread; + + switch_to_program_space_and_thread ((*loc_first_p)->pspace); + + int val + = insert_bp_location ((*loc_first_p), &tmp_error_stream, + &disabled_breaks, + &hw_breakpoint_error, + &hw_bp_error_explained_already); + if (val) + error_flag = val; + + remove_breakpoint (loc); + } + else + swap_insertion (loc, *loc_first_p); + } loc->duplicate = 1; /* Clear the condition modification flag. */ loc->condition_changed = condition_unchanged; } + /* Should this be propagated down to insert_breakpoint_locations if + it's going to be called, so that we have a single place that + throws? */ + if (error_flag) + breakpoint_error_stream (&tmp_error_stream, + &hw_breakpoint_error, + &hw_bp_error_explained_already); + if (insert_mode == UGLL_INSERT || breakpoints_should_be_inserted_now ()) { if (insert_mode != UGLL_DONT_INSERT) base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079 -- 2.14.5 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) 2020-04-19 18:21 ` Pedro Alves @ 2020-04-19 18:49 ` Pedro Alves 2020-04-20 9:02 ` Andrew Burgess ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Pedro Alves @ 2020-04-19 18:49 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, Keno Fischer, gdb-patches On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote: > It was only after all the effort that I realized that it's > pointless to try to make hw and sw breakpoint locations > match _if we always need to install the new sw break before > deleting the hw break_. I mean, we go through all that > effort, but then there's always going to be a small window > where the remote target needs to be able to handle > a sw breakpoint and a hw breakpoint at the same address > anyway (e.g., handle target-side breakpoint conditions > correctly, handle target-side commands correctly, etc.) > > So the patch below is the one I wrote yesterday. It > wasn't finished, but was well advanced. I'm posting > it for the archives and for discussion. At this point, > I don't believe we should follow this patch's approach. > I'll send another reply with today's new patch. So here's my proposed patch. This makes breakpoint_locations_match consider the location's type too, like Keno's original patch. But then does a bunch more stuff to actually make that work correctly. - We need to handle the duplicates detection better. Take a look at the loop at the tail end of update_global_location_list. Currently, because breakpoint locations aren't sorted by type, we can end up with, at the same address, a sw break, then a hw break, then a sw break, etc. The result is that that second sw break won't be considered a duplicate of the first sw break. Seems like we already handle that incorrectly for range breakpoints. - The "set breakpoint auto-hw" handling is moved out of insert_bp_location to update_global_location_list, before the duplicates determination. This change comes with one visible behavior change with "set breakpoint auto-hw off", however. Here: Before: (gdb) break *0x400487 Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. (gdb) c Continuing. Warning: Cannot insert breakpoint 2. Cannot set software breakpoint at read-only address 0x400487 Command aborted. (gdb) After: (gdb) break *0x400487 Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. warning: Breakpoint 2.1 disabled. Cannot set software breakpoint at read-only address 0x400487 I.e., we now warn earlier, instead of waiting until resume time to error out. - In update_breakpoint_locations, the logic of matching old locations with new locations, in the have_ambiguous_names case, is updated to still consider sw vs hw locations the same. - I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any needed updating. Note that that macro walks all locations at a given address, and doesn't call breakpoint_locations_match. The result against GDBserver is this: (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b0.... Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 2 at 0x400736: file threads.c, line 57. Sending packet: $Z0,400736,1#47...Packet received: OK Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) del 1 Sending packet: $z1,400736,1#68...Packet received: OK Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) Or, with "set breakpoint condition-evaluation host", (gdb) hbreak main Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 3 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 4 at 0x400736: file threads.c, line 57. Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) del 3 Sending packet: $z1,400736,1#68...Packet received: OK From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Sun, 19 Apr 2020 18:25:55 +0100 Subject: [PATCH] Stop considering hw and sw breakpoints duplicates --- gdb/breakpoint.c | 269 ++++++++++++++------- gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +- 2 files changed, 183 insertions(+), 88 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index e49025461ba..27799e89807 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *, static int watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2); +static int breakpoint_locations_match (struct bp_location *loc1, + struct bp_location *loc2, + bool sw_hw_bps_match = false); + static int breakpoint_location_address_match (struct bp_location *bl, const struct address_space *aspace, CORE_ADDR addr); @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl) return; /* Do a first pass to check for locations with no assigned - conditions or conditions that fail to parse to a valid agent expression - bytecode. If any of these happen, then it's no use to send conditions - to the target since this location will always trigger and generate a - response back to GDB. */ + conditions or conditions that fail to parse to a valid agent + expression bytecode. If any of these happen, then it's no use to + send conditions to the target since this location will always + trigger and generate a response back to GDB. Note we consider + all locations at the same address irrespective of type, i.e., + even if the locations aren't considered duplicates (e.g., + software breakpoint and hardware breakpoint at the same + address). */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl) } } - /* No NULL conditions or failed bytecode generation. Build a condition list - for this location's address. */ + /* No NULL conditions or failed bytecode generation. Build a + condition list for this location's address. If we have software + and hardware locations at the same address, they aren't + considered duplicates, but we still marge all the conditions + anyway, as it's simpler, and doesn't really make a practical + difference. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl) if (dprintf_style != dprintf_style_agent) return; - /* For now, if we have any duplicate location that isn't a dprintf, - don't install the target-side commands, as that would make the - breakpoint not be reported to the core, and we'd lose + /* For now, if we have any location at the same address that isn't a + dprintf, don't install the target-side commands, as that would + make the breakpoint not be reported to the core, and we'd lose control. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl) } } - /* No NULL commands or failed bytecode generation. Build a command list - for this location's address. */ + /* No NULL commands or failed bytecode generation. Build a command + list for all duplicate locations at this location's address. + Note that here we must care for whether the breakpoint location + types are considered duplicates, otherwise, say, if we have a + software and hardware location at the same address, the target + could end up running the commands twice. For the moment, we only + support targets-side commands with dprintf, but it doesn't hurt + to be pedantically correct in case that changes. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); - if (loc->owner->extra_string + if (breakpoint_locations_match (bl, loc) + && loc->owner->extra_string && is_breakpoint (loc->owner) && loc->pspace->num == bl->pspace->num && loc->owner->enable_state == bp_enabled @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl, if (bl->loc_type == bp_loc_software_breakpoint || bl->loc_type == bp_loc_hardware_breakpoint) { - if (bl->owner->type != bp_hardware_breakpoint) - { - /* If the explicitly specified breakpoint type - is not hardware breakpoint, check the memory map to see - if the breakpoint address is in read only memory or not. - - Two important cases are: - - location type is not hardware breakpoint, memory - is readonly. We change the type of the location to - hardware breakpoint. - - location type is hardware breakpoint, memory is - read-write. This means we've previously made the - location hardware one, but then the memory map changed, - so we undo. - - When breakpoints are removed, remove_breakpoints will use - location types we've just set here, the only possible - problem is that memory map has changed during running - program, but it's not going to work anyway with current - gdb. */ - struct mem_region *mr - = lookup_mem_region (bl->target_info.reqstd_address); - - if (mr) - { - if (automatic_hardware_breakpoints) - { - enum bp_loc_type new_type; - - if (mr->attrib.mode != MEM_RW) - new_type = bp_loc_hardware_breakpoint; - else - new_type = bp_loc_software_breakpoint; - - if (new_type != bl->loc_type) - { - static int said = 0; - - bl->loc_type = new_type; - if (!said) - { - fprintf_filtered (gdb_stdout, - _("Note: automatically using " - "hardware breakpoints for " - "read-only addresses.\n")); - said = 1; - } - } - } - else if (bl->loc_type == bp_loc_software_breakpoint - && mr->attrib.mode != MEM_RW) - { - fprintf_unfiltered (tmp_error_stream, - _("Cannot insert breakpoint %d.\n" - "Cannot set software breakpoint " - "at read-only address %s\n"), - bl->owner->number, - paddress (bl->gdbarch, bl->address)); - return 1; - } - } - } - /* First check to see if we have to handle an overlay. */ if (overlay_debugging == ovly_off || bl->section == NULL @@ -2830,7 +2786,11 @@ insert_breakpoints (void) /* Updating watchpoints creates new locations, so update the global location list. Explicitly tell ugll to insert locations and - ignore breakpoints_always_inserted_mode. */ + ignore breakpoints_always_inserted_mode. Also, + update_global_location_list tries to "upgrade" software + breakpoints to hardware breakpoints to handle "set breakpoint + auto-hw", so we need to call it even if we don't have new + locations. */ update_global_location_list (UGLL_INSERT); } @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1, /* Assuming LOC1 and LOC2's types' have meaningful target addresses (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent - the same location. */ + the same location. If SW_HW_BPS_MATCH is true, then software + breakpoint locations and hardware breakpoint locations match, + otherwise they don't. */ static int -breakpoint_locations_match (struct bp_location *loc1, - struct bp_location *loc2) +breakpoint_locations_match (struct bp_location *loc1, + struct bp_location *loc2, + bool sw_hw_bps_match) { int hw_point1, hw_point2; @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1, else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner)) return tracepoint_locations_match (loc1, loc2); else - /* We compare bp_location.length in order to cover ranged breakpoints. */ + /* We compare bp_location.length in order to cover ranged + breakpoints. Keep this in sync with + bp_location_is_less_than. */ return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, loc2->pspace->aspace, loc2->address) + && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match) && loc1->length == loc2->length); } @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b) } \f +/* Return the location number of LOC. */ + +static int +bp_location_number (bp_location *loc) +{ + int n = 1; + for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next) + { + if (l == loc) + return n; + ++n; + } + + gdb_assert_not_reached ("breakpoint location not found in owner breakpoint"); +} + static bool bp_loc_is_permanent (struct bp_location *loc); +/* Handle "set breakpoint auto-hw". + + If the explicitly specified breakpoint type is not hardware + breakpoint, check the memory map to see whether the breakpoint + address is in read-only memory. + + And then, if "set breakpoint auto-hw" is enabled: + + - location type is not hardware breakpoint, memory is read-only. + We change the type of the location to hardware breakpoint. + + - location type is hardware breakpoint, memory is read-write. This + means we've previously made the location hardware one, but then the + memory map changed, so we undo. + + However, if "set breakpoint auto-hw" is disabled, and memory is + read-only, we warn and disable the breakpoint location. */ + +static void +handle_automatic_hardware_breakpoints (bp_location *bl) +{ + if (bl->loc_type == bp_loc_software_breakpoint + || bl->loc_type == bp_loc_hardware_breakpoint) + { + if (bl->owner->type != bp_hardware_breakpoint) + { + /* When breakpoints are removed, remove_breakpoints will use + location types we've just set here, the only possible + problem is that memory map has changed during running + program, but it's not going to work anyway with current + gdb. */ + mem_region *mr = lookup_mem_region (bl->address); + + if (mr != nullptr) + { + if (automatic_hardware_breakpoints) + { + enum bp_loc_type new_type; + + if (mr->attrib.mode != MEM_RW) + new_type = bp_loc_hardware_breakpoint; + else + new_type = bp_loc_software_breakpoint; + + if (new_type != bl->loc_type) + { + static bool said = false; + + bl->loc_type = new_type; + if (!said) + { + fprintf_filtered (gdb_stdout, + _("Note: automatically using " + "hardware breakpoints for " + "read-only addresses.\n")); + said = true; + } + } + } + else if (bl->loc_type == bp_loc_software_breakpoint + && mr->attrib.mode != MEM_RW) + { + bl->enabled = false; + + warning (_("Breakpoint %d.%d disabled.\n" + "Cannot set software breakpoint " + "at read-only address %s\n"), + bl->owner->number, bp_location_number (bl), + paddress (bl->gdbarch, bl->address)); + } + } + } + } +} + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) if (a->permanent != b->permanent) return a->permanent > b->permanent; + /* Sort by type in order to make duplicate determination easier. + See update_global_location_list. This is kept in sync with + breakpoint_locations_match. */ + if (a->loc_type < b->loc_type) + return true; + + /* Likewise, for range-breakpoints, sort by length. */ + if (a->loc_type == bp_loc_hardware_breakpoint + && b->loc_type == bp_loc_hardware_breakpoint + && a->length < b->length) + return true; + /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of the same type at the same address can be sorted in arbitrary order. */ @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl) loc->cond_bytecode.reset (); } } + /* Called whether new breakpoints are created, or existing breakpoints deleted, to update the global location list and recompute which locations are duplicate of which. @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode) ALL_BREAKPOINTS (b) for (loc = b->loc; loc; loc = loc->next) *locp++ = loc; + + /* See if we need to "upgrade" a software breakpoint to a hardware + breakpoint. Do this before deciding whether locations are + duplicates. Also do this before sorting because sorting order + depends on location type. */ + for (locp = bp_locations; + locp < bp_locations + bp_locations_count; + locp++) + { + loc = *locp; + if (!loc->inserted) + handle_automatic_hardware_breakpoints (loc); + } + std::sort (bp_locations, bp_locations + bp_locations_count, bp_location_is_less_than); @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode) { struct bp_location *loc2 = *loc2p; + if (loc2 == old_loc) + continue; + if (breakpoint_locations_match (loc2, old_loc)) { /* Read watchpoint locations are switched to @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* loc2 is a duplicated location. We need to check if it should be inserted in case it will be unduplicated. */ - if (loc2 != old_loc - && unduplicated_should_be_inserted (loc2)) + if (unduplicated_should_be_inserted (loc2)) { swap_insertion (old_loc, loc2); keep_in_target = 1; @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b, if (have_ambiguous_names) { for (; l; l = l->next) - if (breakpoint_locations_match (e, l)) - { - l->enabled = 0; - break; - } + { + /* Ignore software vs hardware location type at + this point, because with "set breakpoint + auto-hw", after a re-set, locations that were + hardware can end up as software, or vice versa. + As mentioned above, this is an heuristic and in + practice should give the correct answer often + enough. */ + if (breakpoint_locations_match (e, l, true)) + { + l->enabled = 0; + break; + } + } } else { diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp index 65f7baae9f5..b950e433ed8 100644 --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \ # Ensure that inserting a software breakpoint in a known-read-only # region fails. gdb_test "break *$main_lo" \ - "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \ + "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \ "inserting software breakpoint in read-only memory fails" delete_breakpoints base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079 -- 2.14.5 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) 2020-04-19 18:49 ` [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) Pedro Alves @ 2020-04-20 9:02 ` Andrew Burgess 2020-04-21 16:24 ` Christian Biesinger 2020-05-02 20:13 ` [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) Pedro Alves 2 siblings, 0 replies; 17+ messages in thread From: Andrew Burgess @ 2020-04-20 9:02 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, Keno Fischer, gdb-patches * Pedro Alves <palves@redhat.com> [2020-04-19 19:49:12 +0100]: > On 4/19/20 7:21 PM, Pedro Alves via Gdb-patches wrote: > > > It was only after all the effort that I realized that it's > > pointless to try to make hw and sw breakpoint locations > > match _if we always need to install the new sw break before > > deleting the hw break_. I mean, we go through all that > > effort, but then there's always going to be a small window > > where the remote target needs to be able to handle > > a sw breakpoint and a hw breakpoint at the same address > > anyway (e.g., handle target-side breakpoint conditions > > correctly, handle target-side commands correctly, etc.) > > > > So the patch below is the one I wrote yesterday. It > > wasn't finished, but was well advanced. I'm posting > > it for the archives and for discussion. At this point, > > I don't believe we should follow this patch's approach. > > I'll send another reply with today's new patch. > > So here's my proposed patch. > > This makes breakpoint_locations_match consider the location's > type too, like Keno's original patch. But then does a bunch > more stuff to actually make that work correctly. LGTM. Thanks, Andrew > > - We need to handle the duplicates detection better. Take a look > at the loop at the tail end of update_global_location_list. > Currently, because breakpoint locations aren't sorted by type, > we can end up with, at the same address, a sw break, then a hw break, > then a sw break, etc. The result is that that second sw break won't > be considered a duplicate of the first sw break. Seems like > we already handle that incorrectly for range breakpoints. > > - The "set breakpoint auto-hw" handling is moved out of insert_bp_location > to update_global_location_list, before the duplicates determination. > > This change comes with one visible behavior change with > "set breakpoint auto-hw off", however. Here: > > Before: > > (gdb) break *0x400487 > Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. > (gdb) c > Continuing. > Warning: > Cannot insert breakpoint 2. > Cannot set software breakpoint at read-only address 0x400487 > > Command aborted. > (gdb) > > After: > > (gdb) break *0x400487 > Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. > warning: Breakpoint 2.1 disabled. > Cannot set software breakpoint at read-only address 0x400487 > > I.e., we now warn earlier, instead of waiting until resume time to error out. > > - In update_breakpoint_locations, the logic of matching old locations > with new locations, in the have_ambiguous_names case, is updated > to still consider sw vs hw locations the same. > > - I went through all ALL_BP_LOCATIONS_AT_ADDR uses, to see if any > needed updating. Note that that macro walks all locations at > a given address, and doesn't call breakpoint_locations_match. > > The result against GDBserver is this: > > (gdb) hbreak main > Sending packet: $m400700,40#28...Packet received: 89e58b0.... > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > > (gdb) b main > Note: breakpoint 1 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 2 at 0x400736: file threads.c, line 57. > Sending packet: $Z0,400736,1#47...Packet received: OK > Sending packet: $Z1,400736,1#48...Packet received: OK > > (gdb) del 1 > Sending packet: $z1,400736,1#68...Packet received: OK > Sending packet: $Z0,400736,1#47...Packet received: OK > (gdb) > > Or, with "set breakpoint condition-evaluation host", > > (gdb) hbreak main > Sending packet: $m400736,1#fe...Packet received: 48 > Hardware assisted breakpoint 3 at 0x400736: file threads.c, line 57. > Sending packet: $Z1,400736,1#48...Packet received: OK > (gdb) b main > Note: breakpoint 3 also set at pc 0x400736. > Sending packet: $m400736,1#fe...Packet received: 48 > Breakpoint 4 at 0x400736: file threads.c, line 57. > Sending packet: $Z0,400736,1#47...Packet received: OK > (gdb) del 3 > Sending packet: $z1,400736,1#68...Packet received: OK > > From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Sun, 19 Apr 2020 18:25:55 +0100 > Subject: [PATCH] Stop considering hw and sw breakpoints duplicates > > --- > gdb/breakpoint.c | 269 ++++++++++++++------- > gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +- > 2 files changed, 183 insertions(+), 88 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index e49025461ba..27799e89807 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *, > static int watchpoint_locations_match (struct bp_location *loc1, > struct bp_location *loc2); > > +static int breakpoint_locations_match (struct bp_location *loc1, > + struct bp_location *loc2, > + bool sw_hw_bps_match = false); > + > static int breakpoint_location_address_match (struct bp_location *bl, > const struct address_space *aspace, > CORE_ADDR addr); > @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl) > return; > > /* Do a first pass to check for locations with no assigned > - conditions or conditions that fail to parse to a valid agent expression > - bytecode. If any of these happen, then it's no use to send conditions > - to the target since this location will always trigger and generate a > - response back to GDB. */ > + conditions or conditions that fail to parse to a valid agent > + expression bytecode. If any of these happen, then it's no use to > + send conditions to the target since this location will always > + trigger and generate a response back to GDB. Note we consider > + all locations at the same address irrespective of type, i.e., > + even if the locations aren't considered duplicates (e.g., > + software breakpoint and hardware breakpoint at the same > + address). */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > loc = (*loc2p); > @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl) > } > } > > - /* No NULL conditions or failed bytecode generation. Build a condition list > - for this location's address. */ > + /* No NULL conditions or failed bytecode generation. Build a > + condition list for this location's address. If we have software > + and hardware locations at the same address, they aren't > + considered duplicates, but we still marge all the conditions > + anyway, as it's simpler, and doesn't really make a practical > + difference. */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > loc = (*loc2p); > @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl) > if (dprintf_style != dprintf_style_agent) > return; > > - /* For now, if we have any duplicate location that isn't a dprintf, > - don't install the target-side commands, as that would make the > - breakpoint not be reported to the core, and we'd lose > + /* For now, if we have any location at the same address that isn't a > + dprintf, don't install the target-side commands, as that would > + make the breakpoint not be reported to the core, and we'd lose > control. */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl) > } > } > > - /* No NULL commands or failed bytecode generation. Build a command list > - for this location's address. */ > + /* No NULL commands or failed bytecode generation. Build a command > + list for all duplicate locations at this location's address. > + Note that here we must care for whether the breakpoint location > + types are considered duplicates, otherwise, say, if we have a > + software and hardware location at the same address, the target > + could end up running the commands twice. For the moment, we only > + support targets-side commands with dprintf, but it doesn't hurt > + to be pedantically correct in case that changes. */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > loc = (*loc2p); > - if (loc->owner->extra_string > + if (breakpoint_locations_match (bl, loc) > + && loc->owner->extra_string > && is_breakpoint (loc->owner) > && loc->pspace->num == bl->pspace->num > && loc->owner->enable_state == bp_enabled > @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl, > if (bl->loc_type == bp_loc_software_breakpoint > || bl->loc_type == bp_loc_hardware_breakpoint) > { > - if (bl->owner->type != bp_hardware_breakpoint) > - { > - /* If the explicitly specified breakpoint type > - is not hardware breakpoint, check the memory map to see > - if the breakpoint address is in read only memory or not. > - > - Two important cases are: > - - location type is not hardware breakpoint, memory > - is readonly. We change the type of the location to > - hardware breakpoint. > - - location type is hardware breakpoint, memory is > - read-write. This means we've previously made the > - location hardware one, but then the memory map changed, > - so we undo. > - > - When breakpoints are removed, remove_breakpoints will use > - location types we've just set here, the only possible > - problem is that memory map has changed during running > - program, but it's not going to work anyway with current > - gdb. */ > - struct mem_region *mr > - = lookup_mem_region (bl->target_info.reqstd_address); > - > - if (mr) > - { > - if (automatic_hardware_breakpoints) > - { > - enum bp_loc_type new_type; > - > - if (mr->attrib.mode != MEM_RW) > - new_type = bp_loc_hardware_breakpoint; > - else > - new_type = bp_loc_software_breakpoint; > - > - if (new_type != bl->loc_type) > - { > - static int said = 0; > - > - bl->loc_type = new_type; > - if (!said) > - { > - fprintf_filtered (gdb_stdout, > - _("Note: automatically using " > - "hardware breakpoints for " > - "read-only addresses.\n")); > - said = 1; > - } > - } > - } > - else if (bl->loc_type == bp_loc_software_breakpoint > - && mr->attrib.mode != MEM_RW) > - { > - fprintf_unfiltered (tmp_error_stream, > - _("Cannot insert breakpoint %d.\n" > - "Cannot set software breakpoint " > - "at read-only address %s\n"), > - bl->owner->number, > - paddress (bl->gdbarch, bl->address)); > - return 1; > - } > - } > - } > - > /* First check to see if we have to handle an overlay. */ > if (overlay_debugging == ovly_off > || bl->section == NULL > @@ -2830,7 +2786,11 @@ insert_breakpoints (void) > > /* Updating watchpoints creates new locations, so update the global > location list. Explicitly tell ugll to insert locations and > - ignore breakpoints_always_inserted_mode. */ > + ignore breakpoints_always_inserted_mode. Also, > + update_global_location_list tries to "upgrade" software > + breakpoints to hardware breakpoints to handle "set breakpoint > + auto-hw", so we need to call it even if we don't have new > + locations. */ > update_global_location_list (UGLL_INSERT); > } > > @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1, > > /* Assuming LOC1 and LOC2's types' have meaningful target addresses > (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent > - the same location. */ > + the same location. If SW_HW_BPS_MATCH is true, then software > + breakpoint locations and hardware breakpoint locations match, > + otherwise they don't. */ > > static int > -breakpoint_locations_match (struct bp_location *loc1, > - struct bp_location *loc2) > +breakpoint_locations_match (struct bp_location *loc1, > + struct bp_location *loc2, > + bool sw_hw_bps_match) > { > int hw_point1, hw_point2; > > @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1, > else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner)) > return tracepoint_locations_match (loc1, loc2); > else > - /* We compare bp_location.length in order to cover ranged breakpoints. */ > + /* We compare bp_location.length in order to cover ranged > + breakpoints. Keep this in sync with > + bp_location_is_less_than. */ > return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, > loc2->pspace->aspace, loc2->address) > + && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match) > && loc1->length == loc2->length); > } > > @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b) > } > \f > > +/* Return the location number of LOC. */ > + > +static int > +bp_location_number (bp_location *loc) > +{ > + int n = 1; > + for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next) > + { > + if (l == loc) > + return n; > + ++n; > + } > + > + gdb_assert_not_reached ("breakpoint location not found in owner breakpoint"); > +} > + > static bool bp_loc_is_permanent (struct bp_location *loc); > > +/* Handle "set breakpoint auto-hw". > + > + If the explicitly specified breakpoint type is not hardware > + breakpoint, check the memory map to see whether the breakpoint > + address is in read-only memory. > + > + And then, if "set breakpoint auto-hw" is enabled: > + > + - location type is not hardware breakpoint, memory is read-only. > + We change the type of the location to hardware breakpoint. > + > + - location type is hardware breakpoint, memory is read-write. This > + means we've previously made the location hardware one, but then the > + memory map changed, so we undo. > + > + However, if "set breakpoint auto-hw" is disabled, and memory is > + read-only, we warn and disable the breakpoint location. */ > + > +static void > +handle_automatic_hardware_breakpoints (bp_location *bl) > +{ > + if (bl->loc_type == bp_loc_software_breakpoint > + || bl->loc_type == bp_loc_hardware_breakpoint) > + { > + if (bl->owner->type != bp_hardware_breakpoint) > + { > + /* When breakpoints are removed, remove_breakpoints will use > + location types we've just set here, the only possible > + problem is that memory map has changed during running > + program, but it's not going to work anyway with current > + gdb. */ > + mem_region *mr = lookup_mem_region (bl->address); > + > + if (mr != nullptr) > + { > + if (automatic_hardware_breakpoints) > + { > + enum bp_loc_type new_type; > + > + if (mr->attrib.mode != MEM_RW) > + new_type = bp_loc_hardware_breakpoint; > + else > + new_type = bp_loc_software_breakpoint; > + > + if (new_type != bl->loc_type) > + { > + static bool said = false; > + > + bl->loc_type = new_type; > + if (!said) > + { > + fprintf_filtered (gdb_stdout, > + _("Note: automatically using " > + "hardware breakpoints for " > + "read-only addresses.\n")); > + said = true; > + } > + } > + } > + else if (bl->loc_type == bp_loc_software_breakpoint > + && mr->attrib.mode != MEM_RW) > + { > + bl->enabled = false; > + > + warning (_("Breakpoint %d.%d disabled.\n" > + "Cannot set software breakpoint " > + "at read-only address %s\n"), > + bl->owner->number, bp_location_number (bl), > + paddress (bl->gdbarch, bl->address)); > + } > + } > + } > + } > +} > + > static struct bp_location * > add_location_to_breakpoint (struct breakpoint *b, > const struct symtab_and_line *sal) > @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) > if (a->permanent != b->permanent) > return a->permanent > b->permanent; > > + /* Sort by type in order to make duplicate determination easier. > + See update_global_location_list. This is kept in sync with > + breakpoint_locations_match. */ > + if (a->loc_type < b->loc_type) > + return true; > + > + /* Likewise, for range-breakpoints, sort by length. */ > + if (a->loc_type == bp_loc_hardware_breakpoint > + && b->loc_type == bp_loc_hardware_breakpoint > + && a->length < b->length) > + return true; > + > /* Make the internal GDB representation stable across GDB runs > where A and B memory inside GDB can differ. Breakpoint locations of > the same type at the same address can be sorted in arbitrary order. */ > @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl) > loc->cond_bytecode.reset (); > } > } > + > /* Called whether new breakpoints are created, or existing breakpoints > deleted, to update the global location list and recompute which > locations are duplicate of which. > @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > ALL_BREAKPOINTS (b) > for (loc = b->loc; loc; loc = loc->next) > *locp++ = loc; > + > + /* See if we need to "upgrade" a software breakpoint to a hardware > + breakpoint. Do this before deciding whether locations are > + duplicates. Also do this before sorting because sorting order > + depends on location type. */ > + for (locp = bp_locations; > + locp < bp_locations + bp_locations_count; > + locp++) > + { > + loc = *locp; > + if (!loc->inserted) > + handle_automatic_hardware_breakpoints (loc); > + } > + > std::sort (bp_locations, bp_locations + bp_locations_count, > bp_location_is_less_than); > > @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > { > struct bp_location *loc2 = *loc2p; > > + if (loc2 == old_loc) > + continue; > + > if (breakpoint_locations_match (loc2, old_loc)) > { > /* Read watchpoint locations are switched to > @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > /* loc2 is a duplicated location. We need to check > if it should be inserted in case it will be > unduplicated. */ > - if (loc2 != old_loc > - && unduplicated_should_be_inserted (loc2)) > + if (unduplicated_should_be_inserted (loc2)) > { > swap_insertion (old_loc, loc2); > keep_in_target = 1; > @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b, > if (have_ambiguous_names) > { > for (; l; l = l->next) > - if (breakpoint_locations_match (e, l)) > - { > - l->enabled = 0; > - break; > - } > + { > + /* Ignore software vs hardware location type at > + this point, because with "set breakpoint > + auto-hw", after a re-set, locations that were > + hardware can end up as software, or vice versa. > + As mentioned above, this is an heuristic and in > + practice should give the correct answer often > + enough. */ > + if (breakpoint_locations_match (e, l, true)) > + { > + l->enabled = 0; > + break; > + } > + } > } > else > { > diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp > index 65f7baae9f5..b950e433ed8 100644 > --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp > +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp > @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \ > # Ensure that inserting a software breakpoint in a known-read-only > # region fails. > gdb_test "break *$main_lo" \ > - "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \ > + "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \ > "inserting software breakpoint in read-only memory fails" > > delete_breakpoints > > base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079 > -- > 2.14.5 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) 2020-04-19 18:49 ` [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) Pedro Alves 2020-04-20 9:02 ` Andrew Burgess @ 2020-04-21 16:24 ` Christian Biesinger 2020-04-21 18:31 ` Pedro Alves 2020-05-02 20:13 ` [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) Pedro Alves 2 siblings, 1 reply; 17+ messages in thread From: Christian Biesinger @ 2020-04-21 16:24 UTC (permalink / raw) To: Pedro Alves; +Cc: Andrew Burgess, Simon Marchi, Keno Fischer, gdb-patches On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> wrote: > From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Sun, 19 Apr 2020 18:25:55 +0100 > Subject: [PATCH] Stop considering hw and sw breakpoints duplicates > > --- > gdb/breakpoint.c | 269 ++++++++++++++------- > gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +- > 2 files changed, 183 insertions(+), 88 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index e49025461ba..27799e89807 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *, > static int watchpoint_locations_match (struct bp_location *loc1, > struct bp_location *loc2); > > +static int breakpoint_locations_match (struct bp_location *loc1, > + struct bp_location *loc2, > + bool sw_hw_bps_match = false); > + Should this return a bool? > static int breakpoint_location_address_match (struct bp_location *bl, > const struct address_space *aspace, > CORE_ADDR addr); And this, I guess. > @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl) > return; > > /* Do a first pass to check for locations with no assigned > - conditions or conditions that fail to parse to a valid agent expression > - bytecode. If any of these happen, then it's no use to send conditions > - to the target since this location will always trigger and generate a > - response back to GDB. */ > + conditions or conditions that fail to parse to a valid agent > + expression bytecode. If any of these happen, then it's no use to > + send conditions to the target since this location will always > + trigger and generate a response back to GDB. Note we consider > + all locations at the same address irrespective of type, i.e., > + even if the locations aren't considered duplicates (e.g., > + software breakpoint and hardware breakpoint at the same > + address). */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > loc = (*loc2p); > @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl) > } > } > > - /* No NULL conditions or failed bytecode generation. Build a condition list > - for this location's address. */ > + /* No NULL conditions or failed bytecode generation. Build a > + condition list for this location's address. If we have software > + and hardware locations at the same address, they aren't > + considered duplicates, but we still marge all the conditions > + anyway, as it's simpler, and doesn't really make a practical > + difference. */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > loc = (*loc2p); > @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl) > if (dprintf_style != dprintf_style_agent) > return; > > - /* For now, if we have any duplicate location that isn't a dprintf, > - don't install the target-side commands, as that would make the > - breakpoint not be reported to the core, and we'd lose > + /* For now, if we have any location at the same address that isn't a > + dprintf, don't install the target-side commands, as that would > + make the breakpoint not be reported to the core, and we'd lose > control. */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl) > } > } > > - /* No NULL commands or failed bytecode generation. Build a command list > - for this location's address. */ > + /* No NULL commands or failed bytecode generation. Build a command > + list for all duplicate locations at this location's address. > + Note that here we must care for whether the breakpoint location > + types are considered duplicates, otherwise, say, if we have a > + software and hardware location at the same address, the target > + could end up running the commands twice. For the moment, we only > + support targets-side commands with dprintf, but it doesn't hurt > + to be pedantically correct in case that changes. */ > ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) > { > loc = (*loc2p); > - if (loc->owner->extra_string > + if (breakpoint_locations_match (bl, loc) > + && loc->owner->extra_string > && is_breakpoint (loc->owner) > && loc->pspace->num == bl->pspace->num > && loc->owner->enable_state == bp_enabled > @@ -2454,69 +2473,6 @@ insert_bp_location (struct bp_location *bl, > if (bl->loc_type == bp_loc_software_breakpoint > || bl->loc_type == bp_loc_hardware_breakpoint) > { > - if (bl->owner->type != bp_hardware_breakpoint) > - { > - /* If the explicitly specified breakpoint type > - is not hardware breakpoint, check the memory map to see > - if the breakpoint address is in read only memory or not. > - > - Two important cases are: > - - location type is not hardware breakpoint, memory > - is readonly. We change the type of the location to > - hardware breakpoint. > - - location type is hardware breakpoint, memory is > - read-write. This means we've previously made the > - location hardware one, but then the memory map changed, > - so we undo. > - > - When breakpoints are removed, remove_breakpoints will use > - location types we've just set here, the only possible > - problem is that memory map has changed during running > - program, but it's not going to work anyway with current > - gdb. */ > - struct mem_region *mr > - = lookup_mem_region (bl->target_info.reqstd_address); > - > - if (mr) > - { > - if (automatic_hardware_breakpoints) > - { > - enum bp_loc_type new_type; > - > - if (mr->attrib.mode != MEM_RW) > - new_type = bp_loc_hardware_breakpoint; > - else > - new_type = bp_loc_software_breakpoint; > - > - if (new_type != bl->loc_type) > - { > - static int said = 0; > - > - bl->loc_type = new_type; > - if (!said) > - { > - fprintf_filtered (gdb_stdout, > - _("Note: automatically using " > - "hardware breakpoints for " > - "read-only addresses.\n")); > - said = 1; > - } > - } > - } > - else if (bl->loc_type == bp_loc_software_breakpoint > - && mr->attrib.mode != MEM_RW) > - { > - fprintf_unfiltered (tmp_error_stream, > - _("Cannot insert breakpoint %d.\n" > - "Cannot set software breakpoint " > - "at read-only address %s\n"), > - bl->owner->number, > - paddress (bl->gdbarch, bl->address)); > - return 1; > - } > - } > - } > - > /* First check to see if we have to handle an overlay. */ > if (overlay_debugging == ovly_off > || bl->section == NULL > @@ -2830,7 +2786,11 @@ insert_breakpoints (void) > > /* Updating watchpoints creates new locations, so update the global > location list. Explicitly tell ugll to insert locations and > - ignore breakpoints_always_inserted_mode. */ > + ignore breakpoints_always_inserted_mode. Also, > + update_global_location_list tries to "upgrade" software > + breakpoints to hardware breakpoints to handle "set breakpoint > + auto-hw", so we need to call it even if we don't have new > + locations. */ > update_global_location_list (UGLL_INSERT); > } > > @@ -6813,11 +6773,14 @@ tracepoint_locations_match (struct bp_location *loc1, > > /* Assuming LOC1 and LOC2's types' have meaningful target addresses > (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent > - the same location. */ > + the same location. If SW_HW_BPS_MATCH is true, then software > + breakpoint locations and hardware breakpoint locations match, > + otherwise they don't. */ > > static int > -breakpoint_locations_match (struct bp_location *loc1, > - struct bp_location *loc2) > +breakpoint_locations_match (struct bp_location *loc1, > + struct bp_location *loc2, > + bool sw_hw_bps_match) > { > int hw_point1, hw_point2; > > @@ -6835,9 +6798,12 @@ breakpoint_locations_match (struct bp_location *loc1, > else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner)) > return tracepoint_locations_match (loc1, loc2); > else > - /* We compare bp_location.length in order to cover ranged breakpoints. */ > + /* We compare bp_location.length in order to cover ranged > + breakpoints. Keep this in sync with > + bp_location_is_less_than. */ > return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, > loc2->pspace->aspace, loc2->address) > + && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match) > && loc1->length == loc2->length); > } > > @@ -8515,8 +8481,99 @@ mention (struct breakpoint *b) > } > > > +/* Return the location number of LOC. */ > + > +static int > +bp_location_number (bp_location *loc) > +{ > + int n = 1; > + for (bp_location *l = loc->owner->loc; l != nullptr; l = l->next) > + { > + if (l == loc) > + return n; > + ++n; > + } > + > + gdb_assert_not_reached ("breakpoint location not found in owner breakpoint"); > +} > + > static bool bp_loc_is_permanent (struct bp_location *loc); > > +/* Handle "set breakpoint auto-hw". > + > + If the explicitly specified breakpoint type is not hardware > + breakpoint, check the memory map to see whether the breakpoint > + address is in read-only memory. > + > + And then, if "set breakpoint auto-hw" is enabled: > + > + - location type is not hardware breakpoint, memory is read-only. > + We change the type of the location to hardware breakpoint. > + > + - location type is hardware breakpoint, memory is read-write. This > + means we've previously made the location hardware one, but then the > + memory map changed, so we undo. > + > + However, if "set breakpoint auto-hw" is disabled, and memory is > + read-only, we warn and disable the breakpoint location. */ > + > +static void > +handle_automatic_hardware_breakpoints (bp_location *bl) > +{ > + if (bl->loc_type == bp_loc_software_breakpoint > + || bl->loc_type == bp_loc_hardware_breakpoint) > + { > + if (bl->owner->type != bp_hardware_breakpoint) > + { > + /* When breakpoints are removed, remove_breakpoints will use > + location types we've just set here, the only possible > + problem is that memory map has changed during running > + program, but it's not going to work anyway with current > + gdb. */ > + mem_region *mr = lookup_mem_region (bl->address); > + > + if (mr != nullptr) > + { > + if (automatic_hardware_breakpoints) > + { > + enum bp_loc_type new_type; > + > + if (mr->attrib.mode != MEM_RW) > + new_type = bp_loc_hardware_breakpoint; > + else > + new_type = bp_loc_software_breakpoint; > + > + if (new_type != bl->loc_type) > + { > + static bool said = false; > + > + bl->loc_type = new_type; > + if (!said) > + { > + fprintf_filtered (gdb_stdout, > + _("Note: automatically using " > + "hardware breakpoints for " > + "read-only addresses.\n")); > + said = true; > + } > + } > + } > + else if (bl->loc_type == bp_loc_software_breakpoint > + && mr->attrib.mode != MEM_RW) > + { > + bl->enabled = false; > + > + warning (_("Breakpoint %d.%d disabled.\n" > + "Cannot set software breakpoint " > + "at read-only address %s\n"), > + bl->owner->number, bp_location_number (bl), > + paddress (bl->gdbarch, bl->address)); > + } > + } > + } > + } > +} > + > static struct bp_location * > add_location_to_breakpoint (struct breakpoint *b, > const struct symtab_and_line *sal) > @@ -11451,6 +11508,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) > if (a->permanent != b->permanent) > return a->permanent > b->permanent; > > + /* Sort by type in order to make duplicate determination easier. > + See update_global_location_list. This is kept in sync with > + breakpoint_locations_match. */ > + if (a->loc_type < b->loc_type) > + return true; > + > + /* Likewise, for range-breakpoints, sort by length. */ > + if (a->loc_type == bp_loc_hardware_breakpoint > + && b->loc_type == bp_loc_hardware_breakpoint > + && a->length < b->length) > + return true; > + > /* Make the internal GDB representation stable across GDB runs > where A and B memory inside GDB can differ. Breakpoint locations of > the same type at the same address can be sorted in arbitrary order. */ > @@ -11625,6 +11694,7 @@ force_breakpoint_reinsertion (struct bp_location *bl) > loc->cond_bytecode.reset (); > } > } > + > /* Called whether new breakpoints are created, or existing breakpoints > deleted, to update the global location list and recompute which > locations are duplicate of which. > @@ -11673,6 +11743,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > ALL_BREAKPOINTS (b) > for (loc = b->loc; loc; loc = loc->next) > *locp++ = loc; > + > + /* See if we need to "upgrade" a software breakpoint to a hardware > + breakpoint. Do this before deciding whether locations are > + duplicates. Also do this before sorting because sorting order > + depends on location type. */ > + for (locp = bp_locations; > + locp < bp_locations + bp_locations_count; > + locp++) > + { > + loc = *locp; > + if (!loc->inserted) > + handle_automatic_hardware_breakpoints (loc); > + } > + > std::sort (bp_locations, bp_locations + bp_locations_count, > bp_location_is_less_than); > > @@ -11776,6 +11860,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > { > struct bp_location *loc2 = *loc2p; > > + if (loc2 == old_loc) > + continue; > + > if (breakpoint_locations_match (loc2, old_loc)) > { > /* Read watchpoint locations are switched to > @@ -11790,8 +11877,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) > /* loc2 is a duplicated location. We need to check > if it should be inserted in case it will be > unduplicated. */ > - if (loc2 != old_loc > - && unduplicated_should_be_inserted (loc2)) > + if (unduplicated_should_be_inserted (loc2)) > { > swap_insertion (old_loc, loc2); > keep_in_target = 1; > @@ -13508,11 +13594,20 @@ update_breakpoint_locations (struct breakpoint *b, > if (have_ambiguous_names) > { > for (; l; l = l->next) > - if (breakpoint_locations_match (e, l)) > - { > - l->enabled = 0; > - break; > - } > + { > + /* Ignore software vs hardware location type at > + this point, because with "set breakpoint > + auto-hw", after a re-set, locations that were > + hardware can end up as software, or vice versa. > + As mentioned above, this is an heuristic and in > + practice should give the correct answer often > + enough. */ > + if (breakpoint_locations_match (e, l, true)) > + { > + l->enabled = 0; > + break; > + } > + } > } > else > { > diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp > index 65f7baae9f5..b950e433ed8 100644 > --- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp > +++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp > @@ -171,7 +171,7 @@ gdb_test "p /x *(char *) $main_lo = 1" \ > # Ensure that inserting a software breakpoint in a known-read-only > # region fails. > gdb_test "break *$main_lo" \ > - "Cannot insert breakpoint .*Cannot set software breakpoint at read-only address $main_lo.*" \ > + "warning: Breakpoint .* disabled.*Cannot set software breakpoint at read-only address $main_lo.*" \ > "inserting software breakpoint in read-only memory fails" > > delete_breakpoints > > base-commit: 8e4979ac1ea78147ecbcbf81af5e946873dda079 > -- > 2.14.5 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) 2020-04-21 16:24 ` Christian Biesinger @ 2020-04-21 18:31 ` Pedro Alves 0 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2020-04-21 18:31 UTC (permalink / raw) To: Christian Biesinger; +Cc: Simon Marchi, Keno Fischer, gdb-patches On 4/21/20 5:24 PM, Christian Biesinger via Gdb-patches wrote: > On Sun, Apr 19, 2020 at 1:49 PM Pedro Alves via Gdb-patches > <gdb-patches@sourceware.org> wrote: >> From 10d0944768e6ce59861c1522ed48449422a76736 Mon Sep 17 00:00:00 2001 >> From: Pedro Alves <palves@redhat.com> >> Date: Sun, 19 Apr 2020 18:25:55 +0100 >> Subject: [PATCH] Stop considering hw and sw breakpoints duplicates >> >> --- >> gdb/breakpoint.c | 269 ++++++++++++++------- >> gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +- >> 2 files changed, 183 insertions(+), 88 deletions(-) >> >> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> index e49025461ba..27799e89807 100644 >> --- a/gdb/breakpoint.c >> +++ b/gdb/breakpoint.c >> @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *, >> static int watchpoint_locations_match (struct bp_location *loc1, >> struct bp_location *loc2); >> >> +static int breakpoint_locations_match (struct bp_location *loc1, >> + struct bp_location *loc2, >> + bool sw_hw_bps_match = false); >> + > > Should this return a bool? > >> static int breakpoint_location_address_match (struct bp_location *bl, >> const struct address_space *aspace, >> CORE_ADDR addr); > > And this, I guess. Yeah, they should. Old, pre-C++ code. It feels like an orthogonal change though, particularly since I think we could change all of those two, plus watchpoint_locations_match at the same time. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) 2020-04-19 18:49 ` [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) Pedro Alves 2020-04-20 9:02 ` Andrew Burgess 2020-04-21 16:24 ` Christian Biesinger @ 2020-05-02 20:13 ` Pedro Alves 2020-05-17 18:25 ` Pedro Alves 2 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2020-05-02 20:13 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, Keno Fischer, gdb-patches On 4/19/20 7:49 PM, Pedro Alves via Gdb-patches wrote: > This makes breakpoint_locations_match consider the location's > type too, like Keno's original patch. But then does a bunch > more stuff to actually make that work correctly. > > - We need to handle the duplicates detection better. Take a look > at the loop at the tail end of update_global_location_list. > Currently, because breakpoint locations aren't sorted by type, > we can end up with, at the same address, a sw break, then a hw break, > then a sw break, etc. The result is that that second sw break won't > be considered a duplicate of the first sw break. Seems like > we already handle that incorrectly for range breakpoints. > > - The "set breakpoint auto-hw" handling is moved out of insert_bp_location > to update_global_location_list, before the duplicates determination. > > This change comes with one visible behavior change with > "set breakpoint auto-hw off", however. Here: > > Before: > > (gdb) break *0x400487 > Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. > (gdb) c > Continuing. > Warning: > Cannot insert breakpoint 2. > Cannot set software breakpoint at read-only address 0x400487 > > Command aborted. > (gdb) > > After: > > (gdb) break *0x400487 > Breakpoint 2 at 0x400487: file .../src/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c, line 22. > warning: Breakpoint 2.1 disabled. > Cannot set software breakpoint at read-only address 0x400487 > > I.e., we now warn earlier, instead of waiting until resume time to error out. While looking at the test_single_step part of gdb.base/breapoint-in-ro-region.exp, I realized that the change mentioned above isn't correct as is. The problem is that we would be warning and disabling the location, but if that location belongs to e.g., a software single-step location, GDB would end up not installing the location (because it was disabled), and then the inferior would be resumed, and GDB would lose control of the inferior, as it run free with no software single-step breakpoint installed. So instead I've moved the "set breakpoint auto-hw ON" handling to update_global_location_list, and left the "set breakpoint auto-hw OFF" where it was, in insert_bp_location. This means there's no longer a user-visible change. I have not tested this on a software single-step target (I don't think I have such a target readily handy), but I believe that this new version will pass there, and the previous version would fail. > + /* See if we need to "upgrade" a software breakpoint to a hardware > + breakpoint. Do this before deciding whether locations are > + duplicates. Also do this before sorting because sorting order > + depends on location type. */ > + for (locp = bp_locations; > + locp < bp_locations + bp_locations_count; > + locp++) > + { > + loc = *locp; > + if (!loc->inserted) > + handle_automatic_hardware_breakpoints (loc); This here missed checking should_be_inserted -- there's no point in calling handle_automatic_hardware_breakpoints for disabled locations. It no longer makes that much of a difference with the "set breakpoint auto-hw off" warning removed from handle_automatic_hardware_breakpoints, but it doesn't hurt to be correct. I've also realized that before: commit 7c16b83e0521a007e4d86fc30e334b41b01668b4 Author: Pedro Alves <palves@redhat.com> AuthorDate: Wed Oct 15 20:18:31 2014 +0100 Put single-step breakpoints on the bp_location chain we were already inserting hw breakpoints and single-step breakpoints at the same address. But not hw breakpoints and regular software breakpoints, though. So it was still broken, but I thought that was interesting in the sense that it just seems like the hw-and-sw-breakpoints at the same address case just wasn't thought about much in the early days. I've added a new testcase too now. It fails against gdbserver on current master, passes with the fix. I've also now updated the git commit log and write ChangeLog entries. WDYT of this version? From 0a08d49d0aa0cdfb1e6f4da044df2084d3403dc3 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Sat, 2 May 2020 21:04:00 +0100 Subject: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) In the following conditions: - A target with hardware breakpoints available, and - A target that uses software single stepping, - An instruction at ADDRESS loops back to itself, Now consider the following steps: 1. The user places a hardware breakpoint at ADDRESS (an instruction that loops to itself), 2. The inferior runs and hits the breakpoint at ADDRESS, 3. The user tells GDB to 'continue'. In #3 when the user tells GDB to continue, GDB first disables the hardware breakpoint at ADDRESS, and then inserts a software single-step breakpoint at ADDRESS. The original user-created breakpoint was a hardware breakpoint, while the single-step breakpoint will be a software breakpoint. GDB continues and immediately hits the software single-step breakpoint. GDB then deletes the software single-step breakpoint by calling delete_single_step_breakpoints, which eventually calls delete_breakpoint, which, once the breakpoint (and its locations) are deleted, calls update_global_location_list. During update_global_location_list GDB spots that we have an old location (the software single step breakpoint location) that is inserted, but being deleted, and a location (the original hardware breakpoint) at the same address which we are keeping, but which is not currently inserted, GDB then calls breakpoint_locations_match on these two locations. Currently the locations do match, and so GDB calls swap_insertion which swaps the "inserted" state of the two locations. The user created hardware breakpoint is marked as inserted, while the GDB internal software single step breakpoint is now marked as not inserted. After this GDB returns through the call stack and leaves delete_single_step_breakpoints. After this GDB continues with its normal "stopping" process, as part of this stopping process GDB removes all the breakpoints from the target. Due to the swap it is now the user-created hardware breakpoint that is marked as inserted, so it is this breakpoint GDB tries to remove. The problem is that GDB inserted the software single-step breakpoint as a software breakpoint, but is now trying to remove the hardware breakpoint. The problem is removing a software breakpoint is very different to removing a hardware breakpoint, this could result is some undetected undefined behaviour, or as in the original bug report (PR gdb/25741), could result in the target throwing an error. With "set breakpoint always-inserted on", we can easily reproduce this against GDBserver. E.g.: (gdb) hbreak main Sending packet: $m400700,40#28...Packet received: 89e58b.... Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK Packet Z1 (hardware-breakpoint) is supported (gdb) b main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 2 at 0x400736: file threads.c, line 57. (gdb) del Delete all breakpoints? (y or n) y Sending packet: $z0,400736,1#67...Packet received: E01 warning: Error removing breakpoint 2 This patch adds a testcase that does exactly that. Trying to enhance GDB to handle this scenario while continuing to avoid inserting redundant software and hardware breakpoints at the same address turns out futile, because, given non-stop and breakpoints always-inserted, if the user: #1 - inserts a hw breakpoint, then #2 - inserts a sw breakpoint at the same address, and then #3 - removes the original hw breakpoint, GDB would have to make sure to insert the sw breakpoint before removing the hw breakpoint, to avoid running threads missing the breakpoint. I.e., there's always going to be a window where a target needs to be able to handle both sw and a hw breakpoints installed at the same address. You can see more detailed description of that issue here: https://sourceware.org/pipermail/gdb-patches/2020-April/167738.html So the fix here is to just stop considering software breakpoints and hw breakpoints duplicates, and let GDB insert sw and hw breakpoints at the same address. The central change is to make breakpoint_locations_match consider the location's type too. There are several other changes necessary to actually make that that work correctly, however: - We need to handle the duplicates detection better. Take a look at the loop at the tail end of update_global_location_list. Currently, because breakpoint locations aren't sorted by type, we can end up with, at the same address, a sw break, then a hw break, then a sw break, etc. The result is that that second sw break won't be considered a duplicate of the first sw break. Seems like we already handle that incorrectly for range breakpoints. - The "set breakpoint auto-hw on" handling is moved out of insert_bp_location to update_global_location_list, before the duplicates determination. Moving "set breakpoint auto-hw off" handling as well and downgrading it to a warning+'disabling the location' was considered too, but in the end discarded, because we want to error out with internal and momentary breakpoints, like software single-step breakpoints. Disabling such locations at update_global_location_list time would make GDB lose control of the inferior. - In update_breakpoint_locations, the logic of matching old locations with new locations, in the have_ambiguous_names case, is updated to still consider sw vs hw locations the same. - Review all ALL_BP_LOCATIONS_AT_ADDR uses, and update those that might need to be updated, and update comments for those that don't. Note that that macro walks all locations at a given address, and doesn't call breakpoint_locations_match. The result against GDBserver (with "set breakpoint condition-evaluation host" to avoid seeing confusing reinsertions) is: (gdb) hbreak main Sending packet: $m400736,1#fe...Packet received: 48 Hardware assisted breakpoint 1 at 0x400736: file main.c, line 57. Sending packet: $Z1,400736,1#48...Packet received: OK (gdb) b main Note: breakpoint 1 also set at pc 0x400736. Sending packet: $m400736,1#fe...Packet received: 48 Breakpoint 4 at 0x400736: file main.c, line 57. Sending packet: $Z0,400736,1#47...Packet received: OK (gdb) del 3 Sending packet: $z1,400736,1#68...Packet received: OK gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> Andrew Burgess <andrew.burgess@embecosm.com> Keno Fischer <keno@juliacomputing.com> PR gdb/25741 * breakpoint.c (build_target_condition_list): Update comments. (build_target_command_list): Update comments and skip matching locations. (insert_bp_location): Move "set breakpoint auto-hw on" handling to a separate function. Simplify "set breakpoint auto-hw off" handling. (insert_breakpoints): Update comment. (tracepoint_locations_match): New parameter. For breakpoints, compare location types too, if the caller wants to. (handle_automatic_hardware_breakpoints): New functions. (bp_location_is_less_than): Also sort by location type and hardware breakpoint length. (update_global_location_list): Handle "set breakpoint auto-hw on" here. (update_breakpoint_locations): Ask breakpoint_locations_match to ignore location types. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> PR gdb/25741 * gdb.base/hw-sw-break-same-address.exp: New file. --- gdb/breakpoint.c | 253 ++++++++++++++------- .../gdb.base/hw-sw-break-same-address.exp | 73 ++++++ 2 files changed, 240 insertions(+), 86 deletions(-) create mode 100644 gdb/testsuite/gdb.base/hw-sw-break-same-address.exp diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 858f4c75691..3feed929e78 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -144,6 +144,10 @@ static void describe_other_breakpoints (struct gdbarch *, static int watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2); +static int breakpoint_locations_match (struct bp_location *loc1, + struct bp_location *loc2, + bool sw_hw_bps_match = false); + static int breakpoint_location_address_match (struct bp_location *bl, const struct address_space *aspace, CORE_ADDR addr); @@ -2125,10 +2129,14 @@ build_target_condition_list (struct bp_location *bl) return; /* Do a first pass to check for locations with no assigned - conditions or conditions that fail to parse to a valid agent expression - bytecode. If any of these happen, then it's no use to send conditions - to the target since this location will always trigger and generate a - response back to GDB. */ + conditions or conditions that fail to parse to a valid agent + expression bytecode. If any of these happen, then it's no use to + send conditions to the target since this location will always + trigger and generate a response back to GDB. Note we consider + all locations at the same address irrespective of type, i.e., + even if the locations aren't considered duplicates (e.g., + software breakpoint and hardware breakpoint at the same + address). */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); @@ -2177,8 +2185,12 @@ build_target_condition_list (struct bp_location *bl) } } - /* No NULL conditions or failed bytecode generation. Build a condition list - for this location's address. */ + /* No NULL conditions or failed bytecode generation. Build a + condition list for this location's address. If we have software + and hardware locations at the same address, they aren't + considered duplicates, but we still marge all the conditions + anyway, as it's simpler, and doesn't really make a practical + difference. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); @@ -2296,9 +2308,9 @@ build_target_command_list (struct bp_location *bl) if (dprintf_style != dprintf_style_agent) return; - /* For now, if we have any duplicate location that isn't a dprintf, - don't install the target-side commands, as that would make the - breakpoint not be reported to the core, and we'd lose + /* For now, if we have any location at the same address that isn't a + dprintf, don't install the target-side commands, as that would + make the breakpoint not be reported to the core, and we'd lose control. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { @@ -2360,12 +2372,19 @@ build_target_command_list (struct bp_location *bl) } } - /* No NULL commands or failed bytecode generation. Build a command list - for this location's address. */ + /* No NULL commands or failed bytecode generation. Build a command + list for all duplicate locations at this location's address. + Note that here we must care for whether the breakpoint location + types are considered duplicates, otherwise, say, if we have a + software and hardware location at the same address, the target + could end up running the commands twice. For the moment, we only + support targets-side commands with dprintf, but it doesn't hurt + to be pedantically correct in case that changes. */ ALL_BP_LOCATIONS_AT_ADDR (loc2p, locp, bl->address) { loc = (*loc2p); - if (loc->owner->extra_string + if (breakpoint_locations_match (bl, loc) + && loc->owner->extra_string && is_breakpoint (loc->owner) && loc->pspace->num == bl->pspace->num && loc->owner->enable_state == bp_enabled @@ -2451,72 +2470,31 @@ insert_bp_location (struct bp_location *bl, bl->needs_update = 0; } + /* If "set breakpoint auto-hw" is "on" and a software breakpoint was + set at a read-only address, then a breakpoint location will have + been changed to hardware breakpoint before we get here. If it is + "off" however, error out before actually trying to insert the + breakpoint, with a nicer error message. */ if (bl->loc_type == bp_loc_software_breakpoint - || bl->loc_type == bp_loc_hardware_breakpoint) + && !automatic_hardware_breakpoints) { - if (bl->owner->type != bp_hardware_breakpoint) - { - /* If the explicitly specified breakpoint type - is not hardware breakpoint, check the memory map to see - if the breakpoint address is in read only memory or not. - - Two important cases are: - - location type is not hardware breakpoint, memory - is readonly. We change the type of the location to - hardware breakpoint. - - location type is hardware breakpoint, memory is - read-write. This means we've previously made the - location hardware one, but then the memory map changed, - so we undo. - - When breakpoints are removed, remove_breakpoints will use - location types we've just set here, the only possible - problem is that memory map has changed during running - program, but it's not going to work anyway with current - gdb. */ - struct mem_region *mr - = lookup_mem_region (bl->target_info.reqstd_address); - - if (mr) - { - if (automatic_hardware_breakpoints) - { - enum bp_loc_type new_type; - - if (mr->attrib.mode != MEM_RW) - new_type = bp_loc_hardware_breakpoint; - else - new_type = bp_loc_software_breakpoint; - - if (new_type != bl->loc_type) - { - static int said = 0; + mem_region *mr = lookup_mem_region (bl->address); - bl->loc_type = new_type; - if (!said) - { - fprintf_filtered (gdb_stdout, - _("Note: automatically using " - "hardware breakpoints for " - "read-only addresses.\n")); - said = 1; - } - } - } - else if (bl->loc_type == bp_loc_software_breakpoint - && mr->attrib.mode != MEM_RW) - { - fprintf_unfiltered (tmp_error_stream, - _("Cannot insert breakpoint %d.\n" - "Cannot set software breakpoint " - "at read-only address %s\n"), - bl->owner->number, - paddress (bl->gdbarch, bl->address)); - return 1; - } - } + if (mr != nullptr && mr->attrib.mode != MEM_RW) + { + fprintf_unfiltered (tmp_error_stream, + _("Cannot insert breakpoint %d.\n" + "Cannot set software breakpoint " + "at read-only address %s\n"), + bl->owner->number, + paddress (bl->gdbarch, bl->address)); + return 1; } - + } + + if (bl->loc_type == bp_loc_software_breakpoint + || bl->loc_type == bp_loc_hardware_breakpoint) + { /* First check to see if we have to handle an overlay. */ if (overlay_debugging == ovly_off || bl->section == NULL @@ -2830,7 +2808,11 @@ insert_breakpoints (void) /* Updating watchpoints creates new locations, so update the global location list. Explicitly tell ugll to insert locations and - ignore breakpoints_always_inserted_mode. */ + ignore breakpoints_always_inserted_mode. Also, + update_global_location_list tries to "upgrade" software + breakpoints to hardware breakpoints to handle "set breakpoint + auto-hw", so we need to call it even if we don't have new + locations. */ update_global_location_list (UGLL_INSERT); } @@ -6813,11 +6795,14 @@ tracepoint_locations_match (struct bp_location *loc1, /* Assuming LOC1 and LOC2's types' have meaningful target addresses (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent - the same location. */ + the same location. If SW_HW_BPS_MATCH is true, then software + breakpoint locations and hardware breakpoint locations match, + otherwise they don't. */ static int -breakpoint_locations_match (struct bp_location *loc1, - struct bp_location *loc2) +breakpoint_locations_match (struct bp_location *loc1, + struct bp_location *loc2, + bool sw_hw_bps_match) { int hw_point1, hw_point2; @@ -6835,9 +6820,12 @@ breakpoint_locations_match (struct bp_location *loc1, else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner)) return tracepoint_locations_match (loc1, loc2); else - /* We compare bp_location.length in order to cover ranged breakpoints. */ + /* We compare bp_location.length in order to cover ranged + breakpoints. Keep this in sync with + bp_location_is_less_than. */ return (breakpoint_address_match (loc1->pspace->aspace, loc1->address, loc2->pspace->aspace, loc2->address) + && (loc1->loc_type == loc2->loc_type || sw_hw_bps_match) && loc1->length == loc2->length); } @@ -8517,6 +8505,61 @@ mention (struct breakpoint *b) static bool bp_loc_is_permanent (struct bp_location *loc); +/* Handle "set breakpoint auto-hw on". + + If the explicitly specified breakpoint type is not hardware + breakpoint, check the memory map to see whether the breakpoint + address is in read-only memory. + + - location type is not hardware breakpoint, memory is read-only. + We change the type of the location to hardware breakpoint. + + - location type is hardware breakpoint, memory is read-write. This + means we've previously made the location hardware one, but then the + memory map changed, so we undo. +*/ + +static void +handle_automatic_hardware_breakpoints (bp_location *bl) +{ + if (automatic_hardware_breakpoints + && bl->owner->type != bp_hardware_breakpoint + && (bl->loc_type == bp_loc_software_breakpoint + || bl->loc_type == bp_loc_hardware_breakpoint)) + { + /* When breakpoints are removed, remove_breakpoints will use + location types we've just set here, the only possible problem + is that memory map has changed during running program, but + it's not going to work anyway with current gdb. */ + mem_region *mr = lookup_mem_region (bl->address); + + if (mr != nullptr) + { + enum bp_loc_type new_type; + + if (mr->attrib.mode != MEM_RW) + new_type = bp_loc_hardware_breakpoint; + else + new_type = bp_loc_software_breakpoint; + + if (new_type != bl->loc_type) + { + static bool said = false; + + bl->loc_type = new_type; + if (!said) + { + fprintf_filtered (gdb_stdout, + _("Note: automatically using " + "hardware breakpoints for " + "read-only addresses.\n")); + said = true; + } + } + } + } +} + static struct bp_location * add_location_to_breakpoint (struct breakpoint *b, const struct symtab_and_line *sal) @@ -11451,6 +11494,18 @@ bp_location_is_less_than (const bp_location *a, const bp_location *b) if (a->permanent != b->permanent) return a->permanent > b->permanent; + /* Sort by type in order to make duplicate determination easier. + See update_global_location_list. This is kept in sync with + breakpoint_locations_match. */ + if (a->loc_type < b->loc_type) + return true; + + /* Likewise, for range-breakpoints, sort by length. */ + if (a->loc_type == bp_loc_hardware_breakpoint + && b->loc_type == bp_loc_hardware_breakpoint + && a->length < b->length) + return true; + /* Make the internal GDB representation stable across GDB runs where A and B memory inside GDB can differ. Breakpoint locations of the same type at the same address can be sorted in arbitrary order. */ @@ -11625,6 +11680,7 @@ force_breakpoint_reinsertion (struct bp_location *bl) loc->cond_bytecode.reset (); } } + /* Called whether new breakpoints are created, or existing breakpoints deleted, to update the global location list and recompute which locations are duplicate of which. @@ -11673,6 +11729,20 @@ update_global_location_list (enum ugll_insert_mode insert_mode) ALL_BREAKPOINTS (b) for (loc = b->loc; loc; loc = loc->next) *locp++ = loc; + + /* See if we need to "upgrade" a software breakpoint to a hardware + breakpoint. Do this before deciding whether locations are + duplicates. Also do this before sorting because sorting order + depends on location type. */ + for (locp = bp_locations; + locp < bp_locations + bp_locations_count; + locp++) + { + loc = *locp; + if (!loc->inserted && should_be_inserted (loc)) + handle_automatic_hardware_breakpoints (loc); + } + std::sort (bp_locations, bp_locations + bp_locations_count, bp_location_is_less_than); @@ -11776,6 +11846,9 @@ update_global_location_list (enum ugll_insert_mode insert_mode) { struct bp_location *loc2 = *loc2p; + if (loc2 == old_loc) + continue; + if (breakpoint_locations_match (loc2, old_loc)) { /* Read watchpoint locations are switched to @@ -11790,8 +11863,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode) /* loc2 is a duplicated location. We need to check if it should be inserted in case it will be unduplicated. */ - if (loc2 != old_loc - && unduplicated_should_be_inserted (loc2)) + if (unduplicated_should_be_inserted (loc2)) { swap_insertion (old_loc, loc2); keep_in_target = 1; @@ -13508,11 +13580,20 @@ update_breakpoint_locations (struct breakpoint *b, if (have_ambiguous_names) { for (; l; l = l->next) - if (breakpoint_locations_match (e, l)) - { - l->enabled = 0; - break; - } + { + /* Ignore software vs hardware location type at + this point, because with "set breakpoint + auto-hw", after a re-set, locations that were + hardware can end up as software, or vice versa. + As mentioned above, this is an heuristic and in + practice should give the correct answer often + enough. */ + if (breakpoint_locations_match (e, l, true)) + { + l->enabled = 0; + break; + } + } } else { diff --git a/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp b/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp new file mode 100644 index 00000000000..92896ff4db5 --- /dev/null +++ b/gdb/testsuite/gdb.base/hw-sw-break-same-address.exp @@ -0,0 +1,73 @@ +# Copyright 2020 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test that inserting a hardware and a software breakpoint at the same +# address behaves as expected. GDB used to consider hw and sw +# breakpoint locations as duplicate locations, which would lead to bad +# behavior. See PR gdb/25741. + +if {[skip_hw_breakpoint_tests]} { + return 0 +} + +set test hbreak +set srcfile ${test}.c +if { [prepare_for_testing "failed to prepare" ${test} ${srcfile}] } { + return -1 +} + +if ![runto_main] { + fail "can't run to main" + return -1 +} + +delete_breakpoints + +gdb_test_no_output "set breakpoint always-inserted on" +gdb_test_no_output "set breakpoint condition-evaluation host" +gdb_test_no_output "set confirm off" + +# Test inserting a hw breakpoint first, then a sw breakpoint at the +# same address. +with_test_prefix "hw-sw" { + gdb_test "hbreak main" \ + "Hardware assisted breakpoint .* at .*" \ + "hbreak" + + gdb_test "break main" \ + "Note: breakpoint .* also set at .*\r\nBreakpoint .* at .*" \ + "break" + + # A bad GDB debugging against GDBserver would output a warning + # here: + # delete breakpoints + # warning: Error removing breakpoint 3 + # (gdb) FAIL: gdb.base/hw-sw-break-same-address.exp: hw-sw: delete breakpoints + gdb_test_no_output "delete breakpoints" +} + +# Now the opposite: test inserting a sw breakpoint first, then a hw +# breakpoint at the same address. +with_test_prefix "sw-hw" { + gdb_test "break main" \ + "Breakpoint .* at .*" \ + "break" + + gdb_test "hbreak main" \ + "Note: breakpoint .* also set at .*\r\nHardware assisted breakpoint .* at .*" \ + "hbreak" + + gdb_test_no_output "delete breakpoints" +} base-commit: 8c164434186b471fc43a47055af9632c56affdcd -- 2.14.5 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) 2020-05-02 20:13 ` [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) Pedro Alves @ 2020-05-17 18:25 ` Pedro Alves 2020-05-17 18:50 ` Keno Fischer 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2020-05-17 18:25 UTC (permalink / raw) To: Andrew Burgess; +Cc: Simon Marchi, Keno Fischer, gdb-patches On 5/2/20 9:13 PM, Pedro Alves via Gdb-patches wrote: > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > Andrew Burgess <andrew.burgess@embecosm.com> > Keno Fischer <keno@juliacomputing.com> > > PR gdb/25741 > * breakpoint.c (build_target_condition_list): Update comments. > (build_target_command_list): Update comments and skip matching > locations. > (insert_bp_location): Move "set breakpoint auto-hw on" handling to > a separate function. Simplify "set breakpoint auto-hw off" > handling. > (insert_breakpoints): Update comment. > (tracepoint_locations_match): New parameter. For breakpoints, > compare location types too, if the caller wants to. > (handle_automatic_hardware_breakpoints): New functions. > (bp_location_is_less_than): Also sort by location type and > hardware breakpoint length. > (update_global_location_list): Handle "set breakpoint auto-hw on" > here. > (update_breakpoint_locations): Ask breakpoint_locations_match to > ignore location types. > > gdb/testsuite/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > PR gdb/25741 > * gdb.base/hw-sw-break-same-address.exp: New file. I've merged this now. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) 2020-05-17 18:25 ` Pedro Alves @ 2020-05-17 18:50 ` Keno Fischer 0 siblings, 0 replies; 17+ messages in thread From: Keno Fischer @ 2020-05-17 18:50 UTC (permalink / raw) To: Pedro Alves; +Cc: Andrew Burgess, Simon Marchi, gdb-patches On Sun, May 17, 2020, 14:25 Pedro Alves <palves@redhat.com> wrote: > > I've merged this now. > > Thanks, > Pedro Alves Thanks folks. Appreciate all the work that went into this. Keno ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-17 18:51 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-01 1:38 [PATCH] breakpoint: Make sure location types match before swapping Keno Fischer 2020-04-14 7:01 ` Keno Fischer 2020-04-14 15:04 ` Simon Marchi 2020-04-14 16:00 ` Andrew Burgess 2020-04-14 16:26 ` Keno Fischer 2020-04-14 19:17 ` Pedro Alves 2020-04-15 20:46 ` Andrew Burgess 2020-04-17 12:28 ` Andrew Burgess 2020-04-17 17:22 ` Pedro Alves 2020-04-19 18:21 ` Pedro Alves 2020-04-19 18:49 ` [PATCH] Stop considering hw and sw breakpoints duplicates (Re: [PATCH] breakpoint: Make sure location types match before swapping) Pedro Alves 2020-04-20 9:02 ` Andrew Burgess 2020-04-21 16:24 ` Christian Biesinger 2020-04-21 18:31 ` Pedro Alves 2020-05-02 20:13 ` [PATCH v2] Stop considering hw and sw breakpoint locations duplicates (PR gdb/25741) Pedro Alves 2020-05-17 18:25 ` Pedro Alves 2020-05-17 18:50 ` Keno Fischer
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).