[2/2] Fix advance/until and multiple locations (PR gdb/26524)

Message ID 20200822203859.26622-3-pedro@palves.net
State New
Headers show
Series
  • Fix some advance/until issues
Related show

Commit Message

Pedro Alves Aug. 22, 2020, 8:38 p.m.
If you do "advance LINESPEC", and LINESPEC expands to more than one
location, GDB just errors out:

   if (sals.size () != 1)
     error (_("Couldn't get information on specified line."));

For example, advancing to a line in an inlined function, inlined three
times:

 (gdb) b 21
 Breakpoint 1 at 0x55555555516f: advance.cc:21. (3 locations)
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   <MULTIPLE>
 1.1                         y   0x000055555555516f in inline_func at advance.cc:21
 1.2                         y   0x000055555555517e in inline_func at advance.cc:21
 1.3                         y   0x000055555555518d in inline_func at advance.cc:21
 (gdb) advance 21
 Couldn't get information on specified line.
 (gdb)

Similar issue with the "until" command, as it shares the
implementation with "advance".

Since, as the comment in gdb.base/advance.exp says, "advance <location>"
is really just syntactic sugar for "tbreak <location>;continue",
fix this by making GDB insert a breakpoint at all the resolved
locations.

A new testcase is included, which exercises both "advance" and
"until", in two different cases expanding to multiple locations:

  - inlined functions
  - C++ overloads

This also exercises the inline frames issue fixed by the previous
patch.

gdb/ChangeLog:

	PR gdb/26524
	* breakpoint.c (until_break_fsm) <location_breakpoint,
	caller_breakpoint>: Delete fields.
	<breakpoints>: New field.
	<until_break_fsm>: Adjust to save a breakpoint vector instead of
	two individual breakpoints.
	(until_break_fsm::should_stop): Loop over breakpoints in the
	breakpoint vector.
	(until_break_fsm::clean_up): Adjust to clear the breakpoints
	vector.
	(until_break_command): Handle location expanding into multiple
	sals.

gdb/testsuite/ChangeLog:

	PR gdb/26523
	PR gdb/26524
	* gdb.base/advance-until-multiple-locations.cc: New.
	* gdb.base/advance-until-multiple-locations.exp: New.
---
 gdb/breakpoint.c                                   |  77 ++++++-----
 .../gdb.base/advance-until-multiple-locations.cc   |  61 +++++++++
 .../gdb.base/advance-until-multiple-locations.exp  | 142 +++++++++++++++++++++
 3 files changed, 239 insertions(+), 41 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
 create mode 100644 gdb/testsuite/gdb.base/advance-until-multiple-locations.exp

-- 
2.14.5

Comments

Simon Marchi Aug. 26, 2020, 9:01 p.m. | #1
On 2020-08-22 4:38 p.m., Pedro Alves wrote:
> If you do "advance LINESPEC", and LINESPEC expands to more than one

> location, GDB just errors out:

> 

>    if (sals.size () != 1)

>      error (_("Couldn't get information on specified line."));

> 

> For example, advancing to a line in an inlined function, inlined three

> times:

> 

>  (gdb) b 21

>  Breakpoint 1 at 0x55555555516f: advance.cc:21. (3 locations)

>  (gdb) info breakpoints

>  Num     Type           Disp Enb Address            What

>  1       breakpoint     keep y   <MULTIPLE>

>  1.1                         y   0x000055555555516f in inline_func at advance.cc:21

>  1.2                         y   0x000055555555517e in inline_func at advance.cc:21

>  1.3                         y   0x000055555555518d in inline_func at advance.cc:21

>  (gdb) advance 21

>  Couldn't get information on specified line.

>  (gdb)

> 

> Similar issue with the "until" command, as it shares the

> implementation with "advance".

> 

> Since, as the comment in gdb.base/advance.exp says, "advance <location>"

> is really just syntactic sugar for "tbreak <location>;continue",

> fix this by making GDB insert a breakpoint at all the resolved

> locations.

> 

> A new testcase is included, which exercises both "advance" and

> "until", in two different cases expanding to multiple locations:

> 

>   - inlined functions

>   - C++ overloads

> 

> This also exercises the inline frames issue fixed by the previous

> patch.

> 

> gdb/ChangeLog:

> 

> 	PR gdb/26524

> 	* breakpoint.c (until_break_fsm) <location_breakpoint,

> 	caller_breakpoint>: Delete fields.

> 	<breakpoints>: New field.

> 	<until_break_fsm>: Adjust to save a breakpoint vector instead of

> 	two individual breakpoints.

> 	(until_break_fsm::should_stop): Loop over breakpoints in the

> 	breakpoint vector.

> 	(until_break_fsm::clean_up): Adjust to clear the breakpoints

> 	vector.

> 	(until_break_command): Handle location expanding into multiple

> 	sals.

> 

> gdb/testsuite/ChangeLog:

> 

> 	PR gdb/26523

> 	PR gdb/26524

> 	* gdb.base/advance-until-multiple-locations.cc: New.

> 	* gdb.base/advance-until-multiple-locations.exp: New.

> ---

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

>  .../gdb.base/advance-until-multiple-locations.cc   |  61 +++++++++

>  .../gdb.base/advance-until-multiple-locations.exp  | 142 +++++++++++++++++++++

>  3 files changed, 239 insertions(+), 41 deletions(-)

>  create mode 100644 gdb/testsuite/gdb.base/advance-until-multiple-locations.cc

>  create mode 100644 gdb/testsuite/gdb.base/advance-until-multiple-locations.exp

> 

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

> index 977599db1db..4f94a0acbac 100644

> --- a/gdb/breakpoint.c

> +++ b/gdb/breakpoint.c

> @@ -10950,20 +10950,15 @@ struct until_break_fsm : public thread_fsm

>    /* The thread that was current when the command was executed.  */

>    int thread;

>  

> -  /* The breakpoint set at the destination location.  */

> -  breakpoint_up location_breakpoint;

> -

> -  /* Breakpoint set at the return address in the caller frame.  May be

> -     NULL.  */

> -  breakpoint_up caller_breakpoint;

> +  /* The breakpoint set at the return address in the caller frame,

> +     plus breakpoints at the destination location.  */


I think it would make sense to use the plural "locations" in the comment, if we are
talking about multiple destinations locations in terms of machine code.  Though it
could be ok at the singular if you meant the source location, ofwhich there is only
one.

> +  std::vector<breakpoint_up> breakpoints;

>  

>    until_break_fsm (struct interp *cmd_interp, int thread,

> -		   breakpoint_up &&location_breakpoint,

> -		   breakpoint_up &&caller_breakpoint)

> +		   std::vector<breakpoint_up> &&breakpoints)

>      : thread_fsm (cmd_interp),

>        thread (thread),

> -      location_breakpoint (std::move (location_breakpoint)),

> -      caller_breakpoint (std::move (caller_breakpoint))

> +      breakpoints (std::move (breakpoints))

>    {

>    }

>  

> @@ -10978,12 +10973,13 @@ struct until_break_fsm : public thread_fsm

>  bool

>  until_break_fsm::should_stop (struct thread_info *tp)

>  {

> -  if (bpstat_find_breakpoint (tp->control.stop_bpstat,

> -			      location_breakpoint.get ()) != NULL

> -      || (caller_breakpoint != NULL

> -	  && bpstat_find_breakpoint (tp->control.stop_bpstat,

> -				     caller_breakpoint.get ()) != NULL))

> -    set_finished ();

> +  for (const breakpoint_up &bp : breakpoints)

> +    if (bpstat_find_breakpoint (tp->control.stop_bpstat,

> +				bp.get ()) != NULL)

> +      {

> +	set_finished ();

> +	break;

> +      }


When using nested control structures, I thought we required braces for the
outer ones, like this (as mentioned in the GNU coding standards):

  for (...)
    {
      if (...)
        ...
    }

If that's not the case, I have been misleading people for a while!

>  

>    return true;

>  }

> @@ -10995,8 +10991,7 @@ void

>  until_break_fsm::clean_up (struct thread_info *)

>  {

>    /* Clean up our temporary breakpoints.  */

> -  location_breakpoint.reset ();

> -  caller_breakpoint.reset ();

> +  breakpoints.clear ();

>    delete_longjmp_breakpoint (thread);

>  }

>  

> @@ -11034,16 +11029,12 @@ until_break_command (const char *arg, int from_tty, int anywhere)

>         : decode_line_1 (location.get (), DECODE_LINE_FUNFIRSTLINE,

>  			NULL, NULL, 0));

>  

> -  if (sals.size () != 1)

> +  if (sals.empty ())

>      error (_("Couldn't get information on specified line."));

>  

> -  symtab_and_line &sal = sals[0];

> -

>    if (*arg)

>      error (_("Junk at end of arguments."));

>  

> -  resolve_sal_pc (&sal);

> -

>    tp = inferior_thread ();

>    thread = tp->global_num;

>  

> @@ -11060,7 +11051,7 @@ until_break_command (const char *arg, int from_tty, int anywhere)

>    /* Keep within the current frame, or in frames called by the current

>       one.  */

>  

> -  breakpoint_up caller_breakpoint;

> +  std::vector<breakpoint_up> breakpoints;

>  

>    gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;

>  

> @@ -11072,10 +11063,11 @@ until_break_command (const char *arg, int from_tty, int anywhere)

>        sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0);

>        sal2.pc = frame_unwind_caller_pc (frame);

>        caller_gdbarch = frame_unwind_caller_arch (frame);

> -      caller_breakpoint = set_momentary_breakpoint (caller_gdbarch,

> -						    sal2,

> -						    caller_frame_id,

> -						    bp_until);

> +

> +      breakpoint_up caller_breakpoint

> +	= set_momentary_breakpoint (caller_gdbarch, sal2,

> +				    caller_frame_id, bp_until);

> +      breakpoints.emplace_back (std::move (caller_breakpoint));

>  

>        set_longjmp_breakpoint (tp, caller_frame_id);

>        lj_deleter.emplace (thread);

> @@ -11084,21 +11076,24 @@ until_break_command (const char *arg, int from_tty, int anywhere)

>    /* set_momentary_breakpoint could invalidate FRAME.  */

>    frame = NULL;

>  

> -  breakpoint_up location_breakpoint;

> -  if (anywhere)

> -    /* If the user told us to continue until a specified location,

> -       we don't specify a frame at which we need to stop.  */

> -    location_breakpoint = set_momentary_breakpoint (frame_gdbarch, sal,

> -						    null_frame_id, bp_until);

> -  else

> -    /* Otherwise, specify the selected frame, because we want to stop

> -       only at the very same frame.  */

> -    location_breakpoint = set_momentary_breakpoint (frame_gdbarch, sal,

> -						    stack_frame_id, bp_until);

> +  /* If the user told us to continue until a specified location, we

> +     don't specify a frame at which we need to stop.  Otherwise,

> +     specify the selected frame, because we want to stop only at the

> +     very same frame.  */

> +  frame_id stop_frame_id = anywhere ? null_frame_id : stack_frame_id;

> +

> +  for (symtab_and_line &sal : sals)

> +    {

> +      resolve_sal_pc (&sal);

> +

> +      breakpoint_up location_breakpoint

> +	= set_momentary_breakpoint (frame_gdbarch, sal,

> +				    stop_frame_id, bp_until);

> +      breakpoints.emplace_back (std::move (location_breakpoint));

> +    }


I was a bit surprised to see multiple breakpoints being created here.  Since the
parallel is that advance/until is like "tbreak <loc>; continue", I would have
expected a single breakpoint with multiple locations.

I attributed that to the fact this chain of functions take a single sal:

- set_momentary_breakpoint, which calls
- set_raw_breakpoint, which calls
- init_raw_breakpoint

I presume that they could all be made to accept a list of sals instead of a single
sal, and init_raw_breakpoint would add all of them to the breakpoint.

But since that is all internal things never seen by the user, I don't think it would
be worth it to do this change.

So, the two patches LGTM.  I think they are safe enough to merge right now, before the
gdb 10 branch is created.  They address new scenarios without risking too much breaking
old ones.

Simon
Pedro Alves Aug. 27, 2020, 7:54 p.m. | #2
On 8/26/20 10:01 PM, Simon Marchi wrote:
> On 2020-08-22 4:38 p.m., Pedro Alves wrote:


>> -  /* Breakpoint set at the return address in the caller frame.  May be

>> -     NULL.  */

>> -  breakpoint_up caller_breakpoint;

>> +  /* The breakpoint set at the return address in the caller frame,

>> +     plus breakpoints at the destination location.  */

> 

> I think it would make sense to use the plural "locations" in the comment, if we are

> talking about multiple destinations locations in terms of machine code.  Though it

> could be ok at the singular if you meant the source location, ofwhich there is only

> one.


Thanks for spotting that.  It was a typo.  I've added "all" as well:

  /* The breakpoint set at the return address in the caller frame,
     plus breakpoints at all the destination locations.  */
  std::vector<breakpoint_up> breakpoints;

>> +  for (const breakpoint_up &bp : breakpoints)

>> +    if (bpstat_find_breakpoint (tp->control.stop_bpstat,

>> +				bp.get ()) != NULL)

>> +      {

>> +	set_finished ();

>> +	break;

>> +      }

> 

> When using nested control structures, I thought we required braces for the

> outer ones, like this (as mentioned in the GNU coding standards):

> 

>   for (...)

>     {

>       if (...)

>         ...

>     }

> 


Hmm, where in the GNU coding standards?  I don't recall seeing that.

There's a rule for nested if-statements, at 
<https://www.gnu.org/prep/standards/standards.html#Writing-C>:

  "When you have an if-else statement nested in another if statement, always put braces around the if-else. Thus, never write like this: "

That helps with avoiding confusion and the dangling else problem,
though compilers warn about bad indentation nowadays catching
it too.

But here I don't see the benefits.

I don't see it in https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
nor in the GCC standards either.

It's also frequently used in GDB:

 $ grep -rn "for (" gdb/ -A 1 | grep "if (" | wc -l
 564

Here in the whole tree:

 $ grep -rn "for (" -A 1 | grep "if (" | wc -l
 2472

And here in gcc, just under the gcc/ dir:

 $ grep -rn "for (" gcc/ -A 1 | grep "if (" | wc -l
 6469                                                                                                                                                                        

You can see how they typically look with e.g.:

 $ grep -rn "for (" -A 1 | grep "if (" -B 1 | less

> If that's not the case, I have been misleading people for a while!


I'm afraid you may have!

>> +

>> +  for (symtab_and_line &sal : sals)

>> +    {

>> +      resolve_sal_pc (&sal);

>> +

>> +      breakpoint_up location_breakpoint

>> +	= set_momentary_breakpoint (frame_gdbarch, sal,

>> +				    stop_frame_id, bp_until);

>> +      breakpoints.emplace_back (std::move (location_breakpoint));

>> +    }

> 

> I was a bit surprised to see multiple breakpoints being created here.  Since the

> parallel is that advance/until is like "tbreak <loc>; continue", I would have

> expected a single breakpoint with multiple locations.

> 

> I attributed that to the fact this chain of functions take a single sal:

> 

> - set_momentary_breakpoint, which calls

> - set_raw_breakpoint, which calls

> - init_raw_breakpoint

> 

> I presume that they could all be made to accept a list of sals instead of a single

> sal, and init_raw_breakpoint would add all of them to the breakpoint.

> 

> But since that is all internal things never seen by the user, I don't think it would

> be worth it to do this change.


Right, it's all internal.  Momentary breakpoints don't even have a breakpoint
number (it's always 0).  It wouldn't make a real difference.

> 

> So, the two patches LGTM.  I think they are safe enough to merge right now, before the

> gdb 10 branch is created.  They address new scenarios without risking too much breaking

> old ones.


Alright, thanks much!  I'm going to merge them.

Thanks,
Pedro Alves
Simon Marchi Aug. 27, 2020, 8:01 p.m. | #3
On 2020-08-27 3:54 p.m., Pedro Alves wrote:
> On 8/26/20 10:01 PM, Simon Marchi wrote:

>> On 2020-08-22 4:38 p.m., Pedro Alves wrote:

> 

>>> -  /* Breakpoint set at the return address in the caller frame.  May be

>>> -     NULL.  */

>>> -  breakpoint_up caller_breakpoint;

>>> +  /* The breakpoint set at the return address in the caller frame,

>>> +     plus breakpoints at the destination location.  */

>>

>> I think it would make sense to use the plural "locations" in the comment, if we are

>> talking about multiple destinations locations in terms of machine code.  Though it

>> could be ok at the singular if you meant the source location, ofwhich there is only

>> one.

> 

> Thanks for spotting that.  It was a typo.  I've added "all" as well:

> 

>   /* The breakpoint set at the return address in the caller frame,

>      plus breakpoints at all the destination locations.  */

>   std::vector<breakpoint_up> breakpoints;

> 

>>> +  for (const breakpoint_up &bp : breakpoints)

>>> +    if (bpstat_find_breakpoint (tp->control.stop_bpstat,

>>> +				bp.get ()) != NULL)

>>> +      {

>>> +	set_finished ();

>>> +	break;

>>> +      }

>>

>> When using nested control structures, I thought we required braces for the

>> outer ones, like this (as mentioned in the GNU coding standards):

>>

>>   for (...)

>>     {

>>       if (...)

>>         ...

>>     }

>>

> 

> Hmm, where in the GNU coding standards?  I don't recall seeing that.

> 

> There's a rule for nested if-statements, at 

> <https://www.gnu.org/prep/standards/standards.html#Writing-C>:

> 

>   "When you have an if-else statement nested in another if statement, always put braces around the if-else. Thus, never write like this: "

> 

> That helps with avoiding confusion and the dangling else problem,

> though compilers warn about bad indentation nowadays catching

> it too.

> 

> But here I don't see the benefits.

> 

> I don't see it in https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

> nor in the GCC standards either.

> 

> It's also frequently used in GDB:

> 

>  $ grep -rn "for (" gdb/ -A 1 | grep "if (" | wc -l

>  564

> 

> Here in the whole tree:

> 

>  $ grep -rn "for (" -A 1 | grep "if (" | wc -l

>  2472

> 

> And here in gcc, just under the gcc/ dir:

> 

>  $ grep -rn "for (" gcc/ -A 1 | grep "if (" | wc -l

>  6469                                                                                                                                                                        

> 

> You can see how they typically look with e.g.:

> 

>  $ grep -rn "for (" -A 1 | grep "if (" -B 1 | less

> 

>> If that's not the case, I have been misleading people for a while!

> 

> I'm afraid you may have!


I was indeed referring to the if-else thing in the standard.  By extension, I had assumed
it applied to all control structures.  If the standard explained the rationale (like the
Google coding standards do, for example), I wouldn't have made this mistake :(.

So it doesn't even apply if you just have two if.

  if (hello)
    if (bonjour)
      printf ("hola\n");

would be correct (though you could merge the two conditions with an && and have a single if...).

Simon
Tom de Vries Aug. 28, 2020, 8:31 a.m. | #4
On 8/22/20 10:38 PM, Pedro Alves wrote:
> 	* gdb.base/advance-until-multiple-locations.cc: New.

> 	* gdb.base/advance-until-multiple-locations.exp: New.


Hi,

just to note that this fails for me with gcc 7, gcc 4.8 and clang 10:
...
Breakpoint 10, test () at advance-until-multiple-locations.cc:45^M
45        int i = 0;^M
(gdb) until ovld_func^M
main () at advance-until-multiple-locations.cc:61^M
61      }^M
(gdb) FAIL: gdb.base/advance-until-multiple-locations.exp:
until_overload: until ovld_func
...

It passes with gcc-8, gcc-9, gcc-10 and gcc-11.

Thanks,
- Tom
Tom de Vries Aug. 28, 2020, 8:49 a.m. | #5
On 8/28/20 10:31 AM, Tom de Vries wrote:
> On 8/22/20 10:38 PM, Pedro Alves wrote:

>> 	* gdb.base/advance-until-multiple-locations.cc: New.

>> 	* gdb.base/advance-until-multiple-locations.exp: New.

> 

> Hi,

> 

> just to note that this fails for me with gcc 7, gcc 4.8 and clang 10:

> ...

> Breakpoint 10, test () at advance-until-multiple-locations.cc:45^M

> 45        int i = 0;^M

> (gdb) until ovld_func^M

> main () at advance-until-multiple-locations.cc:61^M

> 61      }^M

> (gdb) FAIL: gdb.base/advance-until-multiple-locations.exp:

> until_overload: until ovld_func

> ...

> 

> It passes with gcc-8, gcc-9, gcc-10 and gcc-11.


OK, this is due to the difference in line number info between gcc-7 and
gcc-8, a known issue: with gcc-8, we have additional "recommended
breakpoint locations".

This fixes it for me:
...
diff --git a/gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
b/gdb/t
estsuite/gdb.base/advance-until-multiple-locations.cc
index a90493805f..15e651abae 100644
--- a/gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
+++ b/gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
@@ -58,4 +58,4 @@ int
 main ()
 {
   return test ();
-}
+} // main
diff --git a/gdb/testsuite/gdb.base/advance-until-multiple-locations.exp
b/gdb/
testsuite/gdb.base/advance-until-multiple-locations.exp
index a6a1de6653..037b81947e 100644
--- a/gdb/testsuite/gdb.base/advance-until-multiple-locations.exp
+++ b/gdb/testsuite/gdb.base/advance-until-multiple-locations.exp
@@ -129,7 +129,7 @@ proc_with_prefix until_overload {} {
     # ovld_func is a different frame, so it shouldn't cause a stop.
     # Instead, the program should stop at the caller frame.
     gdb_test "until ovld_func" \
-       "main .* at .*return test \\(\\);.*"
+       "main .* at .*(return test \\(\\);|\} // main)"
 }

 foreach_with_prefix cmd {"until" "advance"} {
...

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 977599db1db..4f94a0acbac 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10950,20 +10950,15 @@  struct until_break_fsm : public thread_fsm
   /* The thread that was current when the command was executed.  */
   int thread;
 
-  /* The breakpoint set at the destination location.  */
-  breakpoint_up location_breakpoint;
-
-  /* Breakpoint set at the return address in the caller frame.  May be
-     NULL.  */
-  breakpoint_up caller_breakpoint;
+  /* The breakpoint set at the return address in the caller frame,
+     plus breakpoints at the destination location.  */
+  std::vector<breakpoint_up> breakpoints;
 
   until_break_fsm (struct interp *cmd_interp, int thread,
-		   breakpoint_up &&location_breakpoint,
-		   breakpoint_up &&caller_breakpoint)
+		   std::vector<breakpoint_up> &&breakpoints)
     : thread_fsm (cmd_interp),
       thread (thread),
-      location_breakpoint (std::move (location_breakpoint)),
-      caller_breakpoint (std::move (caller_breakpoint))
+      breakpoints (std::move (breakpoints))
   {
   }
 
@@ -10978,12 +10973,13 @@  struct until_break_fsm : public thread_fsm
 bool
 until_break_fsm::should_stop (struct thread_info *tp)
 {
-  if (bpstat_find_breakpoint (tp->control.stop_bpstat,
-			      location_breakpoint.get ()) != NULL
-      || (caller_breakpoint != NULL
-	  && bpstat_find_breakpoint (tp->control.stop_bpstat,
-				     caller_breakpoint.get ()) != NULL))
-    set_finished ();
+  for (const breakpoint_up &bp : breakpoints)
+    if (bpstat_find_breakpoint (tp->control.stop_bpstat,
+				bp.get ()) != NULL)
+      {
+	set_finished ();
+	break;
+      }
 
   return true;
 }
@@ -10995,8 +10991,7 @@  void
 until_break_fsm::clean_up (struct thread_info *)
 {
   /* Clean up our temporary breakpoints.  */
-  location_breakpoint.reset ();
-  caller_breakpoint.reset ();
+  breakpoints.clear ();
   delete_longjmp_breakpoint (thread);
 }
 
@@ -11034,16 +11029,12 @@  until_break_command (const char *arg, int from_tty, int anywhere)
        : decode_line_1 (location.get (), DECODE_LINE_FUNFIRSTLINE,
 			NULL, NULL, 0));
 
-  if (sals.size () != 1)
+  if (sals.empty ())
     error (_("Couldn't get information on specified line."));
 
-  symtab_and_line &sal = sals[0];
-
   if (*arg)
     error (_("Junk at end of arguments."));
 
-  resolve_sal_pc (&sal);
-
   tp = inferior_thread ();
   thread = tp->global_num;
 
@@ -11060,7 +11051,7 @@  until_break_command (const char *arg, int from_tty, int anywhere)
   /* Keep within the current frame, or in frames called by the current
      one.  */
 
-  breakpoint_up caller_breakpoint;
+  std::vector<breakpoint_up> breakpoints;
 
   gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;
 
@@ -11072,10 +11063,11 @@  until_break_command (const char *arg, int from_tty, int anywhere)
       sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0);
       sal2.pc = frame_unwind_caller_pc (frame);
       caller_gdbarch = frame_unwind_caller_arch (frame);
-      caller_breakpoint = set_momentary_breakpoint (caller_gdbarch,
-						    sal2,
-						    caller_frame_id,
-						    bp_until);
+
+      breakpoint_up caller_breakpoint
+	= set_momentary_breakpoint (caller_gdbarch, sal2,
+				    caller_frame_id, bp_until);
+      breakpoints.emplace_back (std::move (caller_breakpoint));
 
       set_longjmp_breakpoint (tp, caller_frame_id);
       lj_deleter.emplace (thread);
@@ -11084,21 +11076,24 @@  until_break_command (const char *arg, int from_tty, int anywhere)
   /* set_momentary_breakpoint could invalidate FRAME.  */
   frame = NULL;
 
-  breakpoint_up location_breakpoint;
-  if (anywhere)
-    /* If the user told us to continue until a specified location,
-       we don't specify a frame at which we need to stop.  */
-    location_breakpoint = set_momentary_breakpoint (frame_gdbarch, sal,
-						    null_frame_id, bp_until);
-  else
-    /* Otherwise, specify the selected frame, because we want to stop
-       only at the very same frame.  */
-    location_breakpoint = set_momentary_breakpoint (frame_gdbarch, sal,
-						    stack_frame_id, bp_until);
+  /* If the user told us to continue until a specified location, we
+     don't specify a frame at which we need to stop.  Otherwise,
+     specify the selected frame, because we want to stop only at the
+     very same frame.  */
+  frame_id stop_frame_id = anywhere ? null_frame_id : stack_frame_id;
+
+  for (symtab_and_line &sal : sals)
+    {
+      resolve_sal_pc (&sal);
+
+      breakpoint_up location_breakpoint
+	= set_momentary_breakpoint (frame_gdbarch, sal,
+				    stop_frame_id, bp_until);
+      breakpoints.emplace_back (std::move (location_breakpoint));
+    }
 
   tp->thread_fsm = new until_break_fsm (command_interp (), tp->global_num,
-					std::move (location_breakpoint),
-					std::move (caller_breakpoint));
+					std::move (breakpoints));
 
   if (lj_deleter)
     lj_deleter->release ();
diff --git a/gdb/testsuite/gdb.base/advance-until-multiple-locations.cc b/gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
new file mode 100644
index 00000000000..a90493805fa
--- /dev/null
+++ b/gdb/testsuite/gdb.base/advance-until-multiple-locations.cc
@@ -0,0 +1,61 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   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/>.  */
+
+static inline int __attribute__ ((always_inline))
+inline_func (int i)
+{
+  i++;          /* multiple locations here */
+  return i;
+}
+
+int global = 0;
+
+void
+ovld_func ()
+{
+  global = 1;
+}
+
+void
+ovld_func (int)
+{
+  global = 2;
+}
+
+/* This is a separate function so that we can test that "until" stops
+   at the caller.  */
+
+int
+test ()
+{
+  int i = 0;
+
+  i = inline_func (i);
+  i = inline_func (i);
+  i = inline_func (i);
+
+  ovld_func ();
+  ovld_func (0);
+
+  return 0;
+}
+
+int
+main ()
+{
+  return test ();
+}
diff --git a/gdb/testsuite/gdb.base/advance-until-multiple-locations.exp b/gdb/testsuite/gdb.base/advance-until-multiple-locations.exp
new file mode 100644
index 00000000000..a6a1de6653d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/advance-until-multiple-locations.exp
@@ -0,0 +1,142 @@ 
+# 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 'advance/until LINESPEC' where LINESPEC expands to multiple
+# locations.
+
+standard_testfile .cc
+
+if { [skip_cplus_tests] } { continue }
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  {debug c++}] } {
+    return -1
+}
+
+set lineno [gdb_get_line_number "multiple locations here"]
+
+# advance/until to an inlined line number, which has been inlined
+# multiple times, when the program is stopped at the same inlined
+# function.
+proc_with_prefix until_advance_lineno_from_inlined {cmd} {
+    global lineno
+
+    if ![runto test] {
+	fail "can't run to test"
+	return
+    }
+
+    gdb_breakpoint $lineno
+    gdb_continue_to_breakpoint "break here"
+
+    set lineno2 [expr $lineno + 1]
+
+    gdb_test "$cmd $lineno2" \
+	"inline_func .* at .*:$lineno2.*return i.*" \
+	"$cmd line number"
+}
+
+# advance/until to a line number, which has been inlined multiple
+# times, when the program is stopped at a non-inlined function.
+
+proc_with_prefix until_advance_lineno_from_non_inlined {cmd} {
+    global lineno
+
+    if ![runto test] {
+	fail "can't run to test"
+	return
+    }
+
+    gdb_test "$cmd $lineno" \
+	"inline_func .* at .*:$lineno.*multiple locations here.*" \
+	"$cmd line number"
+}
+
+# Test advancing to an inlined function, which has been inlined
+# multiple times.
+
+proc_with_prefix until_advance_inline_func {cmd} {
+    global lineno
+
+    if ![runto test] {
+	fail "can't run to test"
+	return
+    }
+
+    gdb_test "$cmd inline_func" \
+	"inline_func .* at .*:$lineno.*multiple locations here.*"
+}
+
+# Test advancing to an overloaded function, which is another form of a
+# linespec expanding to multiple locations.  GDB will stop at the
+# first overload called.
+
+proc_with_prefix advance_overload {} {
+    global lineno
+
+    if ![runto test] {
+	fail "can't run to test"
+	return
+    }
+
+    # Test that advance stops at the first overload called by the
+    # program.
+
+    gdb_test "advance ovld_func" \
+	"ovld_func .* at .*global = 1.*" \
+	"first advance stops at ovld_func()"
+
+    # Now test that advance also stops at the other overload called by
+    # the program.
+
+    # Need to issue the advance twice, because advance also stops upon
+    # exit from the current stack frame.
+    gdb_test "advance ovld_func" \
+	"ovld_func \\(0\\);.*" \
+	"second advance stops at caller"
+
+    gdb_test "advance ovld_func" \
+	"ovld_func .* at .*global = 2.*" \
+	"third advance stops at ovld_func(int)"
+}
+
+# Test "until" to an overloaded function, which is another form of a
+# linespec expanding to multiple locations.  Unlike "advance", "until"
+# only stops if still in the same frame.  Since the overloaded
+# function is a different frame, the program should stop at the caller
+# frame instead.
+
+proc_with_prefix until_overload {} {
+    global lineno
+
+    if ![runto test] {
+	fail "can't run to test"
+	return
+    }
+
+    # ovld_func is a different frame, so it shouldn't cause a stop.
+    # Instead, the program should stop at the caller frame.
+    gdb_test "until ovld_func" \
+	"main .* at .*return test \\(\\);.*"
+}
+
+foreach_with_prefix cmd {"until" "advance"} {
+    until_advance_lineno_from_inlined $cmd
+    until_advance_lineno_from_non_inlined $cmd
+    until_advance_inline_func $cmd
+}
+
+advance_overload
+until_overload