[pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp)

Message ID 8c7ace90-f0b8-56f0-0033-5b7827796037@redhat.com
State New
Headers show
Series
  • [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp)
Related show

Commit Message

Pedro Alves June 19, 2018, 4:36 p.m.
On 06/14/2018 02:22 PM, Tom de Vries wrote:
> On 06/12/2018 07:38 PM, Pedro Alves wrote:


>> Yes, sounds like it.  But the selftest.exp explicitly asks to stop

>> at "captured_main", not "captured_main_1", so I'm thinking that

>> it's gdb's behavior that might be wrong:

>>


> Agreed, that's a better solution.

> 

>> Note that both captured_main and captured_main_1 resolved to the

>> same address, 0x791339.  

> 

> Right. I played around a bit with this, and set breakpoints on

> captured_main and captured_main_1.

> 

> If I set a breakpoint on captured_main_1, we have captured_main unknown:

> ...

> Breakpoint 2, captured_main_1 (context=<optimized out>)

>     at /home/vries/gdb_versions/devel/src/gdb/main.c:492

> 492       lim_at_start = (char *) sbrk (0);

> (gdb) p captured_main

> No symbol "captured_main" in current context.

> (gdb) p captured_main_1

> $1 = {void (captured_main_args *)} 0x61b959

> <gdb_main(captured_main_args*)+25>

> ...

> 

> But If I set a breakpoint on captured_main instead, we have

> captured_main_1 unknown:

> ...

> Breakpoint 3, captured_main (data=<optimized out>)

>     at /home/vries/gdb_versions/devel/src/gdb/main.c:1147

> 1147      captured_main_1 (context);

> (gdb) p captured_main

> $2 = {void (void *)} 0x61b959 <gdb_main(captured_main_args*)+25>

> (gdb) p captured_main_1

> No symbol "captured_main_1" in current context.

> ...

> 

> And if I set a breakpoint on both, captured_main_1 seems to take

> precedence (independent of the order used to set the breakpoint):

> ...

> Breakpoint 1, captured_main_1 (context=<optimized out>)

>     at /home/vries/gdb_versions/devel/src/gdb/main.c:492

> 492       lim_at_start = (char *) sbrk (0);

> (gdb) p captured_main_1

> $1 = {void (captured_main_args *)} 0x61b959

> <gdb_main(captured_main_args*)+25>

> (gdb) p captured_main

> No symbol "captured_main" in current context.

> ...

> 

> I don't understand the underlying mechanisms well enough to decide

> whether this is a problem or not, but I thought I just mention it.


Can't pinpoint offhand where the problem is, but sounds like
something in the dwarf or elf symbol readers, maybe the breakpoint
you set first changes the order in which symbols are read and are
added to search hashes etc., or something like that.

It's most certainly unrelated to this change though.

> 

>> The gdb.base/inline-break.exp testcase

>> currently does not exercise that, but the new test added by the

>> patch below does.  That new test fails without the patch and passes

>> with the patch.  No regressions on x86-64 GNU/Linux.  WDYT?

>>

> 

> AFAICT, the patch looks ok (just one nit below).

> 

>> +/* A static inlined function that is called by another static inlined

>> +   function.  */

>> +

>> +static inline ATTR int

>> +func_callee (int x)

>> +{

>> +  return x * 23;

>> +}

>> +

>> +/* A static inlined function that calls another static inlined

>> +   function.  The body of the function is a simple as possible so that

>> +   both functions are inlined to the same PC address.  */

>> +

>> +static int

> 

> inline ATTR ?


Hmm, indeed.  If I do that however gcc (7.3) seemingly optimizes out
the functions more aggressively and we can't set a breakpoint anymore:

 (gdb) b func_inline_caller
 Function "func_inline_caller" not defined.
 Breakpoint 1 (func_inline_caller) pending.

A quick look at the debug info reveals that the debug info
does mention the functions, so this may be another gdb bug:

 <1><2c3>: Abbrev Number: 12 (DW_TAG_subprogram)
    <2c4>   DW_AT_name        : (indirect string, offset: 0x16f): func_inline_caller
    <2c8>   DW_AT_decl_file   : 1
    <2c9>   DW_AT_decl_line   : 197
    <2ca>   DW_AT_prototyped  : 1
    <2ca>   DW_AT_type        : <0x2a4>
    <2ce>   DW_AT_inline      : 3       (declared as inline and inlined)
    <2cf>   DW_AT_sibling     : <0x2dd>
...
 <1><2dd>: Abbrev Number: 12 (DW_TAG_subprogram)
    <2de>   DW_AT_name        : (indirect string, offset: 0x112): func_inline_callee
    <2e2>   DW_AT_decl_file   : 1
    <2e3>   DW_AT_decl_line   : 187
    <2e4>   DW_AT_prototyped  : 1
    <2e4>   DW_AT_type        : <0x2a4>
    <2e8>   DW_AT_inline      : 3       (declared as inline and inlined)
    <2e9>   DW_AT_sibling     : <0x2f7>

I haven't investigated that one.
In order to move forward with the frame skipping patch, I'm
adding a non-inline caller level in the testcase instead.

Below's what I've now merged.

From 4de1f9e76460db0f6b97762ff368e5b8f0da16b0 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Tue, 19 Jun 2018 16:30:13 +0100
Subject: [PATCH] Change inline frame breakpoint skipping logic (fix
 gdb.gdb/selftest.exp)

Currently, gdb.gdb/selftest.exp fails if you build GDB with
optimization (-O2, etc.).

The reason is that after setting a breakpoint in captured_main, we
stop at:
 ...
 Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492
 ...
while selftest_setup expects a stop at captured_main.

Here, captured_main_1 has been inlined into captured_main, and
captured_main has been inlined into gdb_main:

 ...
 $ nm ./build/gdb/gdb | egrep ' [tT] .*captured_main|gdb_main' | c++filt
 000000000061b950 T gdb_main(captured_main_args*)
 ...

Indeed, the two inlined functions show up in the backtrace:

 ...
 (gdb) bt
 #0  captured_main_1 (context=<optimized out>) at main.c:492
 #1  captured_main (data=<optimized out>) at main.c:1147
 #2  gdb_main (args=args@entry=0x7fffffffdb80) at main.c:1173
 #3  0x000000000040fea5 in main (argc=<optimized out>, argv=<optimized out>)
     at gdb.c:32
 ...

We're now stopping at captured_main_1 because commit ddfe970e6bec
("Don't elide all inlined frames") makes GDB present a stop at the
innermost inlined frame if the program stopped by a user breakpoint.

Now, the selftest.exp testcase explicitly asks to stop at
"captured_main", not "captured_main_1", so I'm thinking that it's
GDB'S behavior that should be improved.  That is what this commit
does, by only showing a stop at an inline frame if the user breakpoint
was set in that frame's block.

Before this commit:

 (top-gdb) b captured_main
 Breakpoint 1 at 0x792f99: file src/gdb/main.c, line 492.
 (top-gdb) r
 Starting program: build/gdb/gdb

 Breakpoint 1, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492
 492       lim_at_start = (char *) sbrk (0);
 (top-gdb)

After this commit, we now instead get:

 (top-gdb) b captured_main
 Breakpoint 1 at 0x791339: file src/gdb/main.c, line 492.
 (top-gdb) r
 Starting program: build/gdb/gdb

 Breakpoint 1, captured_main (data=<optimized out>) at src/gdb/main.c:1147
 1147      captured_main_1 (context);
 (top-gdb)

and:

 (top-gdb) b captured_main_1
 Breakpoint 2 at 0x791339: file src/gdb/main.c, line 492.
 (top-gdb) r
 Starting program: build/gdb/gdb
 Breakpoint 2, captured_main_1 (context=<optimized out>) at src/gdb/main.c:492
 492       lim_at_start = (char *) sbrk (0);
 (top-gdb)

Note that both captured_main and captured_main_1 resolved to the same
address, 0x791339.  That is necessary to trigger the issue in
question.  The gdb.base/inline-break.exp testcase currently does not
exercise that, but the new test added by this commit does.  That new
test fails without the GDB fix and passes with the fix.  No
regressions on x86-64 GNU/Linux.

While at it, the THIS_PC comparison in stopped_by_user_bp_inline_frame
is basically a nop, so just remove it -- if a software or hardware
breakpoint explains the stop, then it must be that it was installed at
the current PC.

gdb/ChangeLog:
2018-06-19  Pedro Alves  <palves@redhat.com>

	* inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC
	parameter with a block parameter.  Compare location's block symbol
	with the frame's block instead of addresses.
	(skip_inline_frames): Pass the current block instead of the
	frame's address.  Break out as soon as we determine the frame
	should not be skipped.

gdb/testsuite/ChangeLog:
2018-06-19  Pedro Alves  <palves@redhat.com>

	* gdb.opt/inline-break.c (func_inline_callee, func_inline_caller)
	(func_extern_caller): New.
	(main): Call func_extern_caller.
	* gdb.opt/inline-break.exp: Add tests for inline frame skipping
	logic change.
---
 gdb/inline-frame.c                     | 23 +++++++++++------------
 gdb/testsuite/gdb.opt/inline-break.c   | 34 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.opt/inline-break.exp | 25 +++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 12 deletions(-)

-- 
2.14.3

Comments

Joel Brobecker June 25, 2018, 9:04 p.m. | #1
Hello,

> gdb/ChangeLog:

> 2018-06-19  Pedro Alves  <palves@redhat.com>

>

> 	* inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC

> 	parameter with a block parameter.  Compare location's block symbol

> 	with the frame's block instead of addresses.

> 	(skip_inline_frames): Pass the current block instead of the

> 	frame's address.  Break out as soon as we determine the frame

> 	should not be skipped.

>

> gdb/testsuite/ChangeLog:

> 2018-06-19  Pedro Alves  <palves@redhat.com>

>

> 	* gdb.opt/inline-break.c (func_inline_callee, func_inline_caller)

> 	(func_extern_caller): New.

> 	(main): Call func_extern_caller.

> 	* gdb.opt/inline-break.exp: Add tests for inline frame skipping

> 	logic change.


it looks like this patch is causing a crash with the following
example program:

    $ cat -n r.h
         1	/* r.h */
         2	int counter = 42;
         3
         4	inline void
         5	callee () {
         6	  counter = 0;  /* break here */
         7	}
    $ cat -n r.c
         1	/* r.c */
         2	#include "r.h"
         3
         4	int
         5	main ()
         6	{
         7	  callee ();
         8	}

I compiled it using the following commands:

    $ gcc -c -g -O2 r.c
    $ gcc -o r r.o

Then, trying to put a breakpoint on r.h:6 (inside "callee") causes
a SEGV for me:

    $ gdb -q r
    Reading symbols from r...done.
    (gdb) b r.h:6
    Breakpoint 1 at 0x4003c0: file r.h, line 6.
    (gdb) run
    Starting program: /[...]/r
    [1]    75618 segmentation fault  /[...]/gdb -q r

Prior to this commit, the behavior is the following for the "run"
command:

    (gdb) run
    Starting program: /[...]/r

    Breakpoint 1, callee () at r.h:6
    6	  counter = 0;  /* break here */

The problem occurs because we apparently assume that a bp_location's
symbols is not NULL:

    Program received signal SIGSEGV, Segmentation fault.
    0x00000000006f42bb in stopped_by_user_bp_inline_frame (
        stop_chain=<optimized out>, frame_block=<optimized out>)
        at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305
    305		      && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))
    (gdb) p loc->symbol
    $1 = (const symbol *) 0x0

I don't know yet whether that's a valid assumption or something
occurred earlier in the process. Any thoughts on this before I start
looking deeper?

I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to
reproduce.

-- 
Joel
Pedro Alves June 26, 2018, 7:02 p.m. | #2
On 06/25/2018 10:04 PM, Joel Brobecker wrote:
> Hello,

> 

>> gdb/ChangeLog:

>> 2018-06-19  Pedro Alves  <palves@redhat.com>

>>

>> 	* inline-frame.c (stopped_by_user_bp_inline_frame): Replace PC

>> 	parameter with a block parameter.  Compare location's block symbol

>> 	with the frame's block instead of addresses.

>> 	(skip_inline_frames): Pass the current block instead of the

>> 	frame's address.  Break out as soon as we determine the frame

>> 	should not be skipped.

>>

>> gdb/testsuite/ChangeLog:

>> 2018-06-19  Pedro Alves  <palves@redhat.com>

>>

>> 	* gdb.opt/inline-break.c (func_inline_callee, func_inline_caller)

>> 	(func_extern_caller): New.

>> 	(main): Call func_extern_caller.

>> 	* gdb.opt/inline-break.exp: Add tests for inline frame skipping

>> 	logic change.

> 

> it looks like this patch is causing a crash with the following

> example program:

> 

>     $ cat -n r.h

>          1	/* r.h */

>          2	int counter = 42;

>          3

>          4	inline void

>          5	callee () {

>          6	  counter = 0;  /* break here */

>          7	}

>     $ cat -n r.c

>          1	/* r.c */

>          2	#include "r.h"

>          3

>          4	int

>          5	main ()

>          6	{

>          7	  callee ();

>          8	}

> 

> I compiled it using the following commands:

> 

>     $ gcc -c -g -O2 r.c

>     $ gcc -o r r.o

> 

> Then, trying to put a breakpoint on r.h:6 (inside "callee") causes

> a SEGV for me:

> 

>     $ gdb -q r

>     Reading symbols from r...done.

>     (gdb) b r.h:6

>     Breakpoint 1 at 0x4003c0: file r.h, line 6.

>     (gdb) run

>     Starting program: /[...]/r

>     [1]    75618 segmentation fault  /[...]/gdb -q r

> 

> Prior to this commit, the behavior is the following for the "run"

> command:

> 

>     (gdb) run

>     Starting program: /[...]/r

> 

>     Breakpoint 1, callee () at r.h:6

>     6	  counter = 0;  /* break here */

> 

> The problem occurs because we apparently assume that a bp_location's

> symbols is not NULL:

> 

>     Program received signal SIGSEGV, Segmentation fault.

>     0x00000000006f42bb in stopped_by_user_bp_inline_frame (

>         stop_chain=<optimized out>, frame_block=<optimized out>)

>         at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305

>     305		      && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))

>     (gdb) p loc->symbol

>     $1 = (const symbol *) 0x0


Whoops.  I remember actually thinking about loc->symbol potentially
being null, but somehow convinced myself that SYMBOL_BLOCK_VALUE
would return null in that case...  :-P

> 

> I don't know yet whether that's a valid assumption or something

> occurred earlier in the process. Any thoughts on this before I start

> looking deeper?

> 

> I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to

> reproduce.


If we just add a "loc->symbol != NULL" check, then we end up
presenting the stop at the caller of the inline function, where
the inline function was inlined, which is not what we want, since that's
not where the user set the breakpoint.

Recording the symbol in the location (it is copied from the sal
that linespec.c creates into the location by
add_location_to_breakpoint), like in the patch below, makes gdb present
the stop at  the "break here" line successfully.

I tried to come up with a more complicated testcase that included
more nested blocks inside the inlined function, to see if we would
need to record the inner inlined block in the sal instead of the
function's symbol (does that actually make sense for inlined functions?),
but all I came up with worked with this patch as is.  So maybe we
can defer thinking about that until we find a testcase?

From ac746927c3a8078098729dc3256010b2c1e617f8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Tue, 26 Jun 2018 19:14:41 +0100
Subject: [PATCH] inline

---
 gdb/inline-frame.c | 1 +
 gdb/linespec.c     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 896b0004e4a..3d07f8d0970 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -302,6 +302,7 @@ stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain)
 
 	  if ((t == bp_loc_software_breakpoint
 	       || t == bp_loc_hardware_breakpoint)
+	      && loc->symbol != nullptr
 	      && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))
 	    return true;
 	}
diff --git a/gdb/linespec.c b/gdb/linespec.c
index ae0200b8133..93e66c389f7 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2196,6 +2196,7 @@ create_sals_line_offset (struct linespec_state *self,
 
 	    if (self->funfirstline)
 	      skip_prologue_sal (&intermediate_results[i]);
+	    intermediate_results[i].symbol = sym;
 	    add_sal_to_sals (self, &values, &intermediate_results[i],
 			     sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);
 	  }
-- 
2.14.4

Thanks,
Pedro Alves
Joel Brobecker June 26, 2018, 10:02 p.m. | #3
Hi Pedro,

> >     Program received signal SIGSEGV, Segmentation fault.

> >     0x00000000006f42bb in stopped_by_user_bp_inline_frame (

> >         stop_chain=<optimized out>, frame_block=<optimized out>)

> >         at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305

> >     305		      && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))

> >     (gdb) p loc->symbol

> >     $1 = (const symbol *) 0x0

> 

> Whoops.  I remember actually thinking about loc->symbol potentially

> being null, but somehow convinced myself that SYMBOL_BLOCK_VALUE

> would return null in that case...  :-P

> 

> > 

> > I don't know yet whether that's a valid assumption or something

> > occurred earlier in the process. Any thoughts on this before I start

> > looking deeper?

> > 

> > I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to

> > reproduce.

> 

> If we just add a "loc->symbol != NULL" check, then we end up

> presenting the stop at the caller of the inline function, where

> the inline function was inlined, which is not what we want, since that's

> not where the user set the breakpoint.

> 

> Recording the symbol in the location (it is copied from the sal

> that linespec.c creates into the location by

> add_location_to_breakpoint), like in the patch below, makes gdb present

> the stop at  the "break here" line successfully.

> 

> I tried to come up with a more complicated testcase that included

> more nested blocks inside the inlined function, to see if we would

> need to record the inner inlined block in the sal instead of the

> function's symbol (does that actually make sense for inlined functions?),

> but all I came up with worked with this patch as is.  So maybe we

> can defer thinking about that until we find a testcase?


Just a quick message that the patch makes sense to me, and that
I was just able to run it through AdaCore's testsuite with succes.
Or, I should qualify that - there is one tiny change that I haven't
had time to analyze, but from the surface, it is exactly what you
explained about why you need the second hunk.

I haven't had a chance to run it through the official testsuite,
however, as I have to go ... I am so laaaaate!

I can do that tomorrow, or if you prefer to just finish the patch
up and push it, it'd be perfect. I think the patch is good.

Thanks again!

> 

> >From ac746927c3a8078098729dc3256010b2c1e617f8 Mon Sep 17 00:00:00 2001

> From: Pedro Alves <palves@redhat.com>

> Date: Tue, 26 Jun 2018 19:14:41 +0100

> Subject: [PATCH] inline

> 

> ---

>  gdb/inline-frame.c | 1 +

>  gdb/linespec.c     | 1 +

>  2 files changed, 2 insertions(+)

> 

> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c

> index 896b0004e4a..3d07f8d0970 100644

> --- a/gdb/inline-frame.c

> +++ b/gdb/inline-frame.c

> @@ -302,6 +302,7 @@ stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain)

>  

>  	  if ((t == bp_loc_software_breakpoint

>  	       || t == bp_loc_hardware_breakpoint)

> +	      && loc->symbol != nullptr

>  	      && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))

>  	    return true;

>  	}

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

> index ae0200b8133..93e66c389f7 100644

> --- a/gdb/linespec.c

> +++ b/gdb/linespec.c

> @@ -2196,6 +2196,7 @@ create_sals_line_offset (struct linespec_state *self,

>  

>  	    if (self->funfirstline)

>  	      skip_prologue_sal (&intermediate_results[i]);

> +	    intermediate_results[i].symbol = sym;

>  	    add_sal_to_sals (self, &values, &intermediate_results[i],

>  			     sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);

>  	  }

> -- 

> 2.14.4

> 

> Thanks,

> Pedro Alves


-- 
Joel
Pedro Alves June 27, 2018, 4:28 p.m. | #4
Hi Joel,

On 06/26/2018 11:02 PM, Joel Brobecker wrote:

> Just a quick message that the patch makes sense to me, and that

> I was just able to run it through AdaCore's testsuite with succes.

> Or, I should qualify that - there is one tiny change that I haven't

> had time to analyze, but from the surface, it is exactly what you

> explained about why you need the second hunk.

> 

> I haven't had a chance to run it through the official testsuite,

> however, as I have to go ... I am so laaaaate!

> 

> I can do that tomorrow, or if you prefer to just finish the patch

> up and push it, it'd be perfect. I think the patch is good.

> 

> Thanks again!


FYI, I'm starting to look at this now.

Thanks,
Pedro Alves
Pedro Alves June 28, 2018, 2:48 p.m. | #5
On 06/27/2018 05:28 PM, Pedro Alves wrote:
> Hi Joel,

> 

> On 06/26/2018 11:02 PM, Joel Brobecker wrote:

> 

>> Just a quick message that the patch makes sense to me, and that

>> I was just able to run it through AdaCore's testsuite with succes.

>> Or, I should qualify that - there is one tiny change that I haven't

>> had time to analyze, but from the surface, it is exactly what you

>> explained about why you need the second hunk.

>>

>> I haven't had a chance to run it through the official testsuite,

>> however, as I have to go ... I am so laaaaate!

>>

>> I can do that tomorrow, or if you prefer to just finish the patch

>> up and push it, it'd be perfect. I think the patch is good.

>>

>> Thanks again!

> 

> FYI, I'm starting to look at this now.


So while poking some more at this, noticed that setting a breakpoint
by address crashes in the same way, like "b *ADDRESS".  So I thought
that maybe it would be better to make stopped_by_user_bp_inline_frame
return true if the location has no symbol instead of returning
false like in the version I sent before.  That preserves the previous
behavior of showing the stop at the inline function if we miss
setting the sal's symbol somewhere.

However, playing with that made me notice something else
unrelated to my "Change inline frame breakpoint skipping"
patch:

  (gdb) b *0x40062f
  Breakpoint 2 at 0x40062f: file inline-break.c, line 32.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   0x000000000040062f in main at inline-break.c:32
   (gdb) r
   ....
  Breakpoint 2, func1 (x=1) at inline-break.c:32
  32        return x * 23; /* break here */

Notice that above "info break" says "in main":

   in main at inline-break.c:32
      ^^^^

Since we say "inline-break.c:32" everywhere, and present the
stop at the inline function, I think that "info break" should say instead:

   in func1 at inline-break.c:32
      ^^^^^

Fixing that ends up going back to setting the symbol in the sal
again, but I decided to do that in a separate patch, and still
make "loc->symbol == nullptr" in stopped_by_user_bp_inline_frame return
true, unlike the previous version of the patch.

I'll be sending two patches in response to this email.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
index 1ac5835438d..3edd5b2b20b 100644
--- a/gdb/inline-frame.c
+++ b/gdb/inline-frame.c
@@ -286,11 +286,10 @@  block_starting_point_at (CORE_ADDR pc, const struct block *block)
 }
 
 /* Loop over the stop chain and determine if execution stopped in an
-   inlined frame because of a user breakpoint.  THIS_PC is the current
-   frame's PC.  */
+   inlined frame because of a user breakpoint set at FRAME_BLOCK.  */
 
 static bool
-stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain)
+stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain)
 {
   for (bpstat s = stop_chain; s != NULL; s = s->next)
     {
@@ -301,9 +300,9 @@  stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain)
 	  bp_location *loc = s->bp_location_at;
 	  enum bp_loc_type t = loc->loc_type;
 
-	  if (loc->address == this_pc
-	      && (t == bp_loc_software_breakpoint
-		  || t == bp_loc_hardware_breakpoint))
+	  if ((t == bp_loc_software_breakpoint
+	       || t == bp_loc_hardware_breakpoint)
+	      && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))
 	    return true;
 	}
     }
@@ -340,12 +339,12 @@  skip_inline_frames (ptid_t ptid, bpstat stop_chain)
 		{
 		  /* Do not skip the inlined frame if execution
 		     stopped in an inlined frame because of a user
-		     breakpoint.  */
-		  if (!stopped_by_user_bp_inline_frame (this_pc, stop_chain))
-		    {
-		      skip_count++;
-		      last_sym = BLOCK_FUNCTION (cur_block);
-		    }
+		     breakpoint for this inline function.  */
+		  if (stopped_by_user_bp_inline_frame (cur_block, stop_chain))
+		    break;
+
+		  skip_count++;
+		  last_sym = BLOCK_FUNCTION (cur_block);
 		}
 	      else
 		break;
diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
index 922102debb6..f64a81af939 100644
--- a/gdb/testsuite/gdb.opt/inline-break.c
+++ b/gdb/testsuite/gdb.opt/inline-break.c
@@ -176,6 +176,38 @@  not_inline_func3 (int x)
   return y + inline_func3 (x);
 }
 
+/* The following three functions serve to exercise GDB's inline frame
+   skipping logic when setting a user breakpoint on an inline function
+   by name.  */
+
+/* A static inlined function that is called by another static inlined
+   function.  */
+
+static inline ATTR int
+func_inline_callee (int x)
+{
+  return x * 23;
+}
+
+/* A static inlined function that calls another static inlined
+   function.  The body of the function is as simple as possible so
+   that both functions are inlined to the same PC address.  */
+
+static inline ATTR int
+func_inline_caller (int x)
+{
+  return func_inline_callee (x);
+}
+
+/* An extern not-inline function that calls a static inlined
+   function.  */
+
+int
+func_extern_caller (int x)
+{
+  return func_inline_caller (x);
+}
+
 /* Entry point.  */
 
 int
@@ -205,5 +237,7 @@  main (int argc, char *argv[])
 
   x = not_inline_func3 (-21);
 
+  func_extern_caller (1);
+
   return x;
 }
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
index 008ff1ac33a..bae76254905 100644
--- a/gdb/testsuite/gdb.opt/inline-break.exp
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -231,4 +231,29 @@  foreach_with_prefix cmd [list "break" "tbreak"] {
     }
 }
 
+# func_extern_caller calls func_inline_caller which calls
+# func_inline_callee.  The latter two are both inline functions.  Test
+# that setting a breakpoint on each of the functions reports a stop at
+# that function.  This exercises the inline frame skipping logic.  If
+# we set a breakpoint at function A, we want to present the stop at A,
+# even if A's entry code is an inlined call to another inline function
+# B.
+
+foreach_with_prefix func {
+    "func_extern_caller"
+    "func_inline_caller"
+    "func_inline_callee"
+} {
+    clean_restart $binfile
+
+    if {![runto main]} {
+	untested "could not run to main"
+	continue
+    }
+
+    gdb_breakpoint $func
+    gdb_test "continue" "Breakpoint .* $func .*at .*$srcfile.*" \
+	"breakpoint hit presents stop at breakpointed function"
+}
+
 unset -nocomplain results