[RFA] Allow setting breakpoints on inline functions (PR 10738)

[RFA] Allow setting breakpoints on inline functions (PR 10738)

[Gary Benson]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

Hi all,

This patch, which applies on top of Tom's ambiguous linespec work,
allows you to set breakpoints on inlined functions.  Although it
can't be committed until Tom's stuff goes in, I'm posting it for
feedback now.

I'm particularly interested in how this affects users of languages
other than C.  I'm also interested in feedback about the tests I've
written, since this is the first time I've worked on the testsuite.

Thanks,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 8831 bytes --]

2011-11-29  Gary Benson  <gbenson@redhat.com>

	PR breakpoints/10738
	* dwarf2read.c (struct partial_die_info): New member
	may_be_inlined.
	(read_partial_die): Set may_be_inlined where appropriate.
	(add_partial_subprogram): Add partial symbols for partial
	DIEs that may be inlined.
	(new_symbol_full): Add inlined subroutines to the static
	symbol table.

2011-11-29  Gary Benson  <gbenson@redhat.com>

	PR breakpoints/10738
	* gdb.opt/inline-break.exp: New file.
	* gdb.opt/inline-break.c: Likewise.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 145c8d0..ee89286 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -578,6 +578,7 @@ struct partial_die_info
     unsigned int has_type : 1;
     unsigned int has_specification : 1;
     unsigned int has_pc_info : 1;
+    unsigned int may_be_inlined : 1;
 
     /* Flag set if the SCOPE field of this structure has been
        computed.  */
@@ -4285,6 +4286,10 @@ add_partial_subprogram (struct partial_die_info *pdi,
 				 pdi->highpc - 1 + baseaddr,
 				 cu->per_cu->v.psymtab);
 	    }
+        }
+
+      if (pdi->has_pc_info || (!pdi->is_external && pdi->may_be_inlined))
+	{
           if (!pdi->is_declaration)
 	    /* Ignore subprogram DIEs that do not have a name, they are
 	       illegal.  Do not emit a complaint at this point, we will
@@ -9925,6 +9930,11 @@ read_partial_die (struct partial_die_info *part_die,
 	      language_of_main = language_fortran;
 	    }
 	  break;
+	case DW_AT_inline:
+	  if (DW_UNSND (&attr) == DW_INL_inlined
+	      || DW_UNSND (&attr) == DW_INL_declared_inlined)
+	    part_die->may_be_inlined = 1;
+	  break;
 	default:
 	  break;
 	}
@@ -11800,8 +11810,7 @@ new_symbol_full (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	     finish_block.  */
 	  SYMBOL_CLASS (sym) = LOC_BLOCK;
 	  SYMBOL_INLINED (sym) = 1;
-	  /* Do not add the symbol to any lists.  It will be found via
-	     BLOCK_FUNCTION from the blockvector.  */
+	  list_to_add = &file_symbols;
 	  break;
 	case DW_TAG_template_value_param:
 	  suppress_add = 1;
diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c
new file mode 100644
index 0000000..7e1807f
--- /dev/null
+++ b/gdb/testsuite/gdb.opt/inline-break.c
@@ -0,0 +1,154 @@
+/* Copyright (C) 2011 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/>.  */
+
+#ifdef __GNUC__
+#define ATTR __attribute__((always_inline))
+#else
+#define ATTR
+#endif
+
+/* A static inlined function that is called once.  */
+
+static inline ATTR int
+func1 (int x)
+{
+  return x * 23;
+}
+
+/* A non-static inlined function that is called once.  */
+
+inline ATTR int
+func2 (int x)
+{
+  return x * 17;
+}
+
+/* A static inlined function that calls another static inlined
+   function.  */
+
+static inline ATTR int
+func3b (int x)
+{
+  return x < 14 ? 1 : 2;
+}
+
+static inline ATTR int
+func3a (int x)
+{
+  return func3b (x * 23);
+}
+
+/* A non-static inlined function that calls a static inlined
+   function.  */
+
+static inline ATTR int
+func4b (int x)
+{
+  return x < 13 ? 1 : 2;
+}
+
+inline ATTR int
+func4a (int x)
+{
+  return func4b (x * 17);
+}
+
+/* A static inlined function that calls a non-static inlined
+   function.  */
+
+inline ATTR int
+func5b (int x)
+{
+  return x < 12 ? 1 : 2;
+}
+
+static inline ATTR int
+func5a (int x)
+{
+  return func5b (x * 23);
+}
+
+/* A non-static inlined function that calls another non-static inlined
+   function.  */
+
+inline ATTR int
+func6b (int x)
+{
+  return x < 14 ? 3 : 2;
+}
+
+inline ATTR int
+func6a (int x)
+{
+  return func6b (x * 17);
+}
+
+/* A static inlined function that is called more than once.  */
+
+static inline ATTR int
+func7b (int x)
+{
+  return x < 23 ? 1 : 4;
+}
+
+static inline ATTR int
+func7a (int x)
+{
+  return func7b (x * 29);
+}
+
+/* A non-static inlined function that is called more than once.  */
+
+inline ATTR int
+func8b (int x)
+{
+  return x < 7 ? 11 : 9;
+}
+
+static inline ATTR int
+func8a (int x)
+{
+  return func8b (x * 31);
+}
+
+/* Entry point.  */
+
+int
+main (int argc, char *argv[])
+{
+  /* Declaring x as volatile here prevents GCC from combining calls.
+     If GCC is allowed to combine calls then some of them end up with
+     no instructions at all, so there is no specific address for GDB
+     to set a breakpoint at.  */
+  volatile int x = argc;
+
+  x = func1 (x);
+
+  x = func2 (x);
+
+  x = func3a (x);
+
+  x = func4a (x);
+
+  x = func5a (x);
+
+  x = func6a (x);
+
+  x = func7a (x) + func7b (x);
+
+  x = func8a (x) + func8b (x);
+
+  return x;
+}
diff --git a/gdb/testsuite/gdb.opt/inline-break.exp b/gdb/testsuite/gdb.opt/inline-break.exp
new file mode 100644
index 0000000..63d1fad
--- /dev/null
+++ b/gdb/testsuite/gdb.opt/inline-break.exp
@@ -0,0 +1,91 @@
+# Copyright 2011 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/>.
+
+set testfile "inline-break"
+
+if { [prepare_for_testing $testfile.exp $testfile $testfile.c \
+          {debug optimize=-O2 additional_flags=-Winline}] } {
+    return -1
+}
+
+#
+# func1 is a static inlined function that is called once.
+# The result should be a single-location breakpoint.
+#
+gdb_test "break func1" \
+    "Breakpoint.*at.* file .*$testfile\.c, line.*"
+
+#
+# func2 is a non-static inlined function that is called once.
+# The result should be a breakpoint with two locations: the
+# out-of-line function and the single inlined instance.
+#
+gdb_test "break func2" \
+    "Breakpoint.*at.*func2.*(2 locations).*"
+
+#
+# func3b is a static inlined function that is called once from
+# within another static inlined function.  The result should be
+# a single-location breakpoint.
+#
+gdb_test "break func3b" \
+    "Breakpoint.*at.* file .*$testfile\.c, line.*"
+
+#
+# func4b is a static inlined function that is called once from
+# within a non-static inlined function.  The result should be
+# a breakpoint with two locations: the inlined instance within
+# the inlined call to func4a in main, and the inlined instance
+# within the out-of-line func4a.
+#
+gdb_test "break func4b" \
+    "Breakpoint.*at.*func4b.*(2 locations).*"
+
+#
+# func5b is a non-static inlined function that is called once
+# from within a static inlined function.  The result should be a
+# breakpoint with two locations: the out-of-line function and the
+# inlined instance within the inlined call to func5a in main.
+#
+gdb_test "break func5b" \
+    "Breakpoint.*at.*func5b.*(2 locations).*"
+#
+# func6b is a non-static inlined function that is called once from
+# within another non-static inlined function.  The result should be
+# a breakpoint with three locations: the out-of-line function, the
+# inlined instance within the out-of-line func6a, and the inlined
+# instance within the inlined call to func6a in main,
+#
+gdb_test "break func6b" \
+    "Breakpoint.*at.*func6b.*(3 locations).*"
+
+#
+# func7b is a static inlined function that is called twice: once from
+# func7a, and once from main.  The result should be a breakpoint with
+# two locations: the inlined instance within the inlined instance of
+# func7a, and the inlined instance within main.
+#
+gdb_test "break func7b" \
+    "Breakpoint.*at.*func7b.*(2 locations).*"
+
+#
+# func8b is a non-static inlined function that is called twice: once
+# func8a, and once from main.  The result should be a breakpoint with
+# three locations: the out-of-line function, the inlined instance
+# within the inlined instance of func7a, and the inlined instance
+# within main.
+#
+gdb_test "break func8b" \
+    "Breakpoint.*at.*func8b.*(3 locations).*"

Re: [RFA] Allow setting breakpoints on inline functions (PR 10738)

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> This patch, which applies on top of Tom's ambiguous linespec work,
Gary> allows you to set breakpoints on inlined functions.  Although it
Gary> can't be committed until Tom's stuff goes in, I'm posting it for
Gary> feedback now.

This is super.  Thanks.

I noticed that the manual node "Inline Functions" says:

       There are some ways that GDB does not pretend that inlined function
    calls are the same as normal calls:

       * You cannot set breakpoints on inlined functions.  GDB either
         reports that there is no symbol with that name, or else sets the
         breakpoint only on non-inlined copies of the function.  This
         limitation will be removed in a future version of GDB; until then,
         set a breakpoint by line number on the first line of the inlined
         function instead.
    [...]

I think this needs a small update.

Also I think this feature deserves a NEWS entry.

I checked it out and played with it a little.  I found one little bug.
Using the inline-break test case from the patch:

(gdb) p &func1
$1 = (int (*)(int)) 0x4003d8 <main+8>

That is, it chooses the location of the inline function as the address
of the function when evaluating an expression.  I think this is wrong.
Instead, it should ignore inline instances here, returning the address
of the out-of-line instance.  And, if there is no out-of-line (as in
this case), it should error.

I think one possible way to do this would be to put a flag on symbols,
marking inline instances, and then have ordinary symbol lookup ignore
such symbols.  I am not sure how hard this would be.  There might also
be other approaches.

Tom

Re: [RFA] Allow setting breakpoints on inline functions (PR 10738)

[Jan Kratochvil]

On Tue, 29 Nov 2011 16:02:00 +0100, Gary Benson wrote:
> I'm also interested in feedback about the tests I've
> written, since this is the first time I've worked on the testsuite.

I would prefer definitely a copy in gdb.dwarf2/ .  gdb.opt/ I find a bad idea
as it breaks too much across GCC changes.  just gcc -S -dA is probably OK.

If there is gdb.dwarf2/ then I would even drop the gdb.opt/ one.  Not sure if
it makes sense for non-DWARF targets, the functionality gets tested by the
DWARF targets anyway.


> --- /dev/null
> +++ b/gdb/testsuite/gdb.opt/inline-break.exp
...
> +if { [prepare_for_testing $testfile.exp $testfile $testfile.c \
> +          {debug optimize=-O2 additional_flags=-Winline}] } {

This -Winline is questionable.

You make the .c file compatible even if __GNUC__ is not defined but then you
use GCC-specific -Winline option.  Either just make the testcase whole
GCC-specific or make a fallback if the compilation with -Winline fails try
also non-Winline.  Or maybe just drop that -Winline, there is no -Werror
anyway so it was more just for the testcase development.

As I suggested the gdb.dwarf2/ way this whole -Winline is offtopic then.


> +#
> +# func1 is a static inlined function that is called once.
> +# The result should be a single-location breakpoint.
> +#
> +gdb_test "break func1" \
> +    "Breakpoint.*at.* file .*$testfile\.c, line.*"

As you use "" and not {} you should use \\.c and not \.c .  This way it is the
same as .c .

It is there several times.


> +# func2 is a non-static inlined function that is called once.
> +# The result should be a breakpoint with two locations: the
> +# out-of-line function and the single inlined instance.
> +#
> +gdb_test "break func2" \
> +    "Breakpoint.*at.*func2.*(2 locations).*"

You wanted to use \\( and \\) here.

It is there several times.



Thanks,
Jan

Re: [RFA] Allow setting breakpoints on inline functions (PR 10738)

[Gary Benson]

Jan Kratochvil wrote:
> On Tue, 29 Nov 2011 16:02:00 +0100, Gary Benson wrote:
> > I'm also interested in feedback about the tests I've written,
> > since this is the first time I've worked on the testsuite.
> 
> I would prefer definitely a copy in gdb.dwarf2/ .  gdb.opt/ I find a
> bad idea as it breaks too much across GCC changes.  just gcc -S -dA
> is probably OK.
> 
> If there is gdb.dwarf2/ then I would even drop the gdb.opt/ one.
> Not sure if it makes sense for non-DWARF targets, the functionality
> gets tested by the DWARF targets anyway.

Are you saying I should move the testcase from gdb.opt and into
gdb.dwarf2?  I can do that.

Is there some difference between the way the various directories
of tests are treated?  And, is there something I should read to help
decide where to put tests?

> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.opt/inline-break.exp
> ...
> > +if { [prepare_for_testing $testfile.exp $testfile $testfile.c \
> > +          {debug optimize=-O2 additional_flags=-Winline}] } {
> 
> This -Winline is questionable.
> 
> You make the .c file compatible even if __GNUC__ is not defined but
> then you use GCC-specific -Winline option.  Either just make the
> testcase whole GCC-specific or make a fallback if the compilation
> with -Winline fails try also non-Winline.  Or maybe just drop that
> -Winline, there is no -Werror anyway so it was more just for the
> testcase development.

As I understand it the -Winline is there to cause the test to fail
if the methods don't get inlined.  I may very well be wrong, I don't
understand the syntax 100%, but as I read it any unexpected compiler
output causes gdb_compile to assume the compilation failed.

I copied this (somewhat) from the other gdb.opt/inline* testcases,
so if there is a problem with it then I guess they should be fixed
too.

> As I suggested the gdb.dwarf2/ way this whole -Winline is offtopic
> then.
> 
> > +#
> > +# func1 is a static inlined function that is called once.
> > +# The result should be a single-location breakpoint.
> > +#
> > +gdb_test "break func1" \
> > +    "Breakpoint.*at.* file .*$testfile\.c, line.*"
> 
> As you use "" and not {} you should use \\.c and not \.c .  This way
> it is the same as .c .
> 
> It is there several times.
> 
> > +# func2 is a non-static inlined function that is called once.
> > +# The result should be a breakpoint with two locations: the
> > +# out-of-line function and the single inlined instance.
> > +#
> > +gdb_test "break func2" \
> > +    "Breakpoint.*at.*func2.*(2 locations).*"
> 
> You wanted to use \\( and \\) here.
> 
> It is there several times.

Thanks, I added the extra '\'s on my branch.  Would it be better
to to use {} here, or does that make other changes?

Cheers,
Gary

-- 
http://gbenson.net/

Re: [RFA] Allow setting breakpoints on inline functions (PR 10738)

[Jan Kratochvil]

On Thu, 01 Dec 2011 14:40:58 +0100, Gary Benson wrote:
> Are you saying I should move the testcase from gdb.opt and into
> gdb.dwarf2?  I can do that.
> 
> Is there some difference between the way the various directories
> of tests are treated?  And, is there something I should read to help
> decide where to put tests?

It is not just about the directory name.  The goal is that in gdb.dwarf2/ you
provide .S files and not .c files.  That is they are precompiled, and
therefore independent from possible compiler changes.  The tests should test
GDB, not the compiler.

The drawback is if you generate that .S files by `gcc -S -dA' it starts to be
arch-dependent.  Typically the tests there are either i386 or x86_64 that way.
There is a way to make them arch-independent (such as
gdb.dwarf2/dw2-skip-prologue.* and others) but it is a lot of hand coding
which is not required, it is only voluntary (f.e. I do them, Tom does not).


> As I understand it the -Winline is there to cause the test to fail
> if the methods don't get inlined.  I may very well be wrong, I don't
> understand the syntax 100%, but as I read it any unexpected compiler
> output causes gdb_compile to assume the compilation failed.

OK, thanks for info, I see now.


> Thanks, I added the extra '\'s on my branch.  Would it be better
> to to use {} here, or does that make other changes?

Some people prefer it that way some the other way.

By using {} you prevent one level of interpretation.  Therefore it halves the
number of backslashes.  But it also prevents you from using \r\n or $variable
insertions, in those cases you still have to use "" and double the number of
backslashes there.


Thanks,
Jan

Re: [RFA] Allow setting breakpoints on inline functions (PR 10738)

[Gary Benson]

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> This patch, which applies on top of Tom's ambiguous linespec work,
> Gary> allows you to set breakpoints on inlined functions.  Although it
> Gary> can't be committed until Tom's stuff goes in, I'm posting it for
> Gary> feedback now.
> 
> I noticed that the manual node "Inline Functions" says:
> 
>        There are some ways that GDB does not pretend that inlined function
>     calls are the same as normal calls:
> 
>        * You cannot set breakpoints on inlined functions.  GDB
>          either reports that there is no symbol with that name, or
>          else sets the breakpoint only on non-inlined copies of the
>          function.  This limitation will be removed in a future
>          version of GDB; until then, set a breakpoint by line number
>          on the first line of the inlined function instead.
>     [...]
> 
> I think this needs a small update.

I removed that entire bullet point.

> Also I think this feature deserves a NEWS entry.

I added "* GDB can now set breakpoints on inlined functions." on the
branch.

> I checked it out and played with it a little.  I found one little
> bug.  Using the inline-break test case from the patch:
> 
> (gdb) p &func1
> $1 = (int (*)(int)) 0x4003d8 <main+8>
> 
> That is, it chooses the location of the inline function as the
> address of the function when evaluating an expression.  I think
> this is wrong.  Instead, it should ignore inline instances here,
> returning the address of the out-of-line instance.  And, if there
> is no out-of-line (as in this case), it should error.
> 
> I think one possible way to do this would be to put a flag on
> symbols, marking inline instances, and then have ordinary symbol
> lookup ignore such symbols.  I am not sure how hard this would be.
> There might also be other approaches.

There already is such a flag: SYMBOL_INLINED (sym).  I've pushed
a fix to the branch to make lookup_block_symbol skip over inlined
symbols.  It works as you ask with no regressions.

I won't make a new patch yet as I need to look at some of Jan's
suggestions.

Thanks,
Gary

-- 
http://gbenson.net/