[PATCHv5] Make "skip" work on inline frames
Commit Message
On 12/2/19 3:34 AM, Simon Marchi wrote:
> On 2019-11-24 6:22 a.m., Bernd Edlinger wrote:
>> This is just a minor update on the patch
>> since the function SYMBOL_PRINT_NAME was removed with
>> commit 987012b89bce7f6385ed88585547f852a8005a3f
>> I replaced it with sym->print_name (), otherwise the
>> patch is unchanged.
>
> Hi Bernd,
>
> Sorry, I had lost this in the mailing list noise.
>
> I played a bit with the patch and different cases of figure. I am not able to understand
> the purpose of each of your changes (due to the complexity of that particular code), but
> I didn't find anything that stood out as wrong to me. Pedro might be able to do a more
> in-depth review of the event handling code.
>
> If the test tests specifically skipping of inline functions, I'd name it something more
> descriptive than "skip2.exp", maybe "skip-inline.exp"?
>
> Unfortunately, your test doesn't pass on my computer (gcc 9.2.0), but neither does the
> gdb.base/skip.exp. I am attaching the gdb.log when running your test, if it can help.
>
> Simon
>
Hi Simon,
I only tested that with gcc-4.8, and both test cases worked with that gcc version.
I tried now with gcc-trunk version from a few days ago, and I think I see
what you mean.
skip2.c (now skip-inline.c) can be fixed by removing the assignment
to x in the first line, which is superfluous (and copied from skip.c).
But skip.c cannot be fixed this way. I only see a chance to allow
the stepping back to main and then to foo happen.
Does this modified test case work for you?
Thanks
Bernd.
Comments
On 2019-12-02 11:47 a.m., Bernd Edlinger wrote:
> I only tested that with gcc-4.8, and both test cases worked with that gcc version.
>
> I tried now with gcc-trunk version from a few days ago, and I think I see
> what you mean.
>
> skip2.c (now skip-inline.c) can be fixed by removing the assignment
> to x in the first line, which is superfluous (and copied from skip.c).
> But skip.c cannot be fixed this way. I only see a chance to allow
> the stepping back to main and then to foo happen.
>
> Does this modified test case work for you?
Yes, I confirm it passes now, thanks!
Simon
Ping...
The latest version of this patch can be found here:
https://sourceware.org/ml/gdb-patches/2019-12/msg00047.html
Thanks
Bernd.
On 12/2/19 5:47 PM, Bernd Edlinger wrote:
> On 12/2/19 3:34 AM, Simon Marchi wrote:
>> On 2019-11-24 6:22 a.m., Bernd Edlinger wrote:
>>> This is just a minor update on the patch
>>> since the function SYMBOL_PRINT_NAME was removed with
>>> commit 987012b89bce7f6385ed88585547f852a8005a3f
>>> I replaced it with sym->print_name (), otherwise the
>>> patch is unchanged.
>>
>> Hi Bernd,
>>
>> Sorry, I had lost this in the mailing list noise.
>>
>> I played a bit with the patch and different cases of figure. I am not able to understand
>> the purpose of each of your changes (due to the complexity of that particular code), but
>> I didn't find anything that stood out as wrong to me. Pedro might be able to do a more
>> in-depth review of the event handling code.
>>
>> If the test tests specifically skipping of inline functions, I'd name it something more
>> descriptive than "skip2.exp", maybe "skip-inline.exp"?
>>
>> Unfortunately, your test doesn't pass on my computer (gcc 9.2.0), but neither does the
>> gdb.base/skip.exp. I am attaching the gdb.log when running your test, if it can help.
>>
>> Simon
>>
>
> Hi Simon,
>
> I only tested that with gcc-4.8, and both test cases worked with that gcc version.
>
> I tried now with gcc-trunk version from a few days ago, and I think I see
> what you mean.
>
> skip2.c (now skip-inline.c) can be fixed by removing the assignment
> to x in the first line, which is superfluous (and copied from skip.c).
> But skip.c cannot be fixed this way. I only see a chance to allow
> the stepping back to main and then to foo happen.
>
> Does this modified test case work for you?
>
>
>
> Thanks
> Bernd.
>
On 2019-12-02 11:47 a.m., Bernd Edlinger wrote:
> On 12/2/19 3:34 AM, Simon Marchi wrote:
>> On 2019-11-24 6:22 a.m., Bernd Edlinger wrote:
>>> This is just a minor update on the patch
>>> since the function SYMBOL_PRINT_NAME was removed with
>>> commit 987012b89bce7f6385ed88585547f852a8005a3f
>>> I replaced it with sym->print_name (), otherwise the
>>> patch is unchanged.
>>
>> Hi Bernd,
>>
>> Sorry, I had lost this in the mailing list noise.
>>
>> I played a bit with the patch and different cases of figure. I am not able to understand
>> the purpose of each of your changes (due to the complexity of that particular code), but
>> I didn't find anything that stood out as wrong to me. Pedro might be able to do a more
>> in-depth review of the event handling code.
>>
>> If the test tests specifically skipping of inline functions, I'd name it something more
>> descriptive than "skip2.exp", maybe "skip-inline.exp"?
>>
>> Unfortunately, your test doesn't pass on my computer (gcc 9.2.0), but neither does the
>> gdb.base/skip.exp. I am attaching the gdb.log when running your test, if it can help.
>>
>> Simon
>>
>
> Hi Simon,
>
> I only tested that with gcc-4.8, and both test cases worked with that gcc version.
>
> I tried now with gcc-trunk version from a few days ago, and I think I see
> what you mean.
>
> skip2.c (now skip-inline.c) can be fixed by removing the assignment
> to x in the first line, which is superfluous (and copied from skip.c).
> But skip.c cannot be fixed this way. I only see a chance to allow
> the stepping back to main and then to foo happen.
>
> Does this modified test case work for you?
>
>
>
> Thanks
> Bernd.
>
Hi Bernd,
Thanks for fixing the skip.exp test at the same time. I'd prefer if this was done as a
separate patch though, since it's an issue separate from the inline stepping issue you
were originally tackling.
So the patch looks good to me if you remove those bits, and fix the following nits:
- Remove "load_lib completion-support.exp" from the test.
- The indentation in the .exp should use tabs for multiple of 8 columns, instead of just spaces (like you did in the .c).
A comment I would have on the bits in skip.exp:
# with recent gcc we jump once back to main before entering foo here
# if that happens try to step a second time
gdb_test "step" "foo \\(\\) at.*" "step 3" "main \\(\\) at .*" "step"
It's usually not helpful to say "with recent gcc", since it doesn't mean much, especially
when reading this 10 years from now. Instead, mention the specific gcc version this was
observed with. Also, begin the sentence with a capital letter and finish with a period.
Thanks!
Simon
From 32627e64b9b7b437c5f6d46d1138ecba09816273 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH 2/2] Add a test case for skip with inlined functions
Fixed an issue in skip.exp that made it fail with recent
gcc versions, while already at it.
---
gdb/testsuite/gdb.base/skip-inline.c | 64 +++++++++++++++++++++++++++
gdb/testsuite/gdb.base/skip-inline.exp | 80 ++++++++++++++++++++++++++++++++++
gdb/testsuite/gdb.base/skip.exp | 12 +++--
3 files changed, 153 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/skip-inline.c
create mode 100644 gdb/testsuite/gdb.base/skip-inline.exp
new file mode 100644
@@ -0,0 +1,64 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2019 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/>. */
+
+#include <stdlib.h>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+__attribute__((__always_inline__)) static inline int
+foo (void)
+{
+ return bar ();
+}
+
+int
+main ()
+{
+ volatile int x;
+
+ /* step immediately into the inlined code */
+ baz (foo ());
+
+ /* step first over non-inline code, this involves a different code path */
+ x = 0; x = baz (foo ());
+
+ test_skip_file_and_function ();
+
+ return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+ abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+ test_skip ();
+ skip1_test_skip_file_and_function ();
+ end_test_skip_file_and_function ();
+}
new file mode 100644
@@ -0,0 +1,80 @@
+# Copyright 2019 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/>.
+
+load_lib completion-support.exp
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "skip-inline" \
+ {skip-inline.c skip1.c } \
+ {debug nowarnings}] } {
+ return -1
+}
+
+set srcfile skip-inline.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+ fail "can't run to main"
+ return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+gdb_test "step" ".*" "step again into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+gdb_test "step" ".*" "step in the baz, again"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
+gdb_test "step" ".*" "step back to main, again"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+
+if ![runto_main] {
+ fail "can't run to main"
+ return
+}
+
+with_test_prefix "double step" {
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+ gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+ gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+ gdb_test "step" ".*" "step back to main"
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+ gdb_test "step 2" ".*" "step again into baz, since foo will be skipped"
+ gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+ gdb_test "step" ".*" "step back to main, again"
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
+
+if ![runto_main] {
+ fail "can't run to main"
+ return
+}
+
+with_test_prefix "triple step" {
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+ gdb_test "step 3" ".*" "step over baz"
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+ gdb_test "step 3" ".*" "step over baz, again"
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
@@ -142,7 +142,9 @@ with_test_prefix "step after disabling 3" {
gdb_test "step" "bar \\(\\) at.*" "step 1"
gdb_test "step" ".*" "step 2"; # Return from foo()
- gdb_test "step" "foo \\(\\) at.*" "step 3"
+ # with recent gcc we jump once back to main before entering foo here
+ # if that happens try to step a second time
+ gdb_test "step" "foo \\(\\) at.*" "step 3" "main \\(\\) at .*" "step"
gdb_test "step" ".*" "step 4"; # Return from bar()
gdb_test "step" "main \\(\\) at.*" "step 5"
}
@@ -261,7 +263,9 @@ with_test_prefix "step using -fu for baz" {
gdb_test_no_output "skip enable 7"
gdb_test "step" "bar \\(\\) at.*" "step 1"
gdb_test "step" ".*" "step 2"; # Return from bar()
- gdb_test "step" "foo \\(\\) at.*" "step 3"
+ # with recent gcc we jump once back to main before entering foo here
+ # if that happens try to step a second time
+ gdb_test "step" "foo \\(\\) at.*" "step 3" "main \\(\\) at.*" "step"
gdb_test "step" ".*" "step 4"; # Return from foo()
gdb_test "step" "main \\(\\) at.*" "step 5"
}
@@ -276,7 +280,9 @@ with_test_prefix "step using -rfu for baz" {
gdb_test_no_output "skip enable 8"
gdb_test "step" "bar \\(\\) at.*" "step 1"
gdb_test "step" ".*" "step 2"; # Return from bar()
- gdb_test "step" "foo \\(\\) at.*" "step 3"
+ # with recent gcc we jump once back to main before entering foo here
+ # if that happens try to step a second time
+ gdb_test "step" "foo \\(\\) at.*" "step 3" "main \\(\\) at.*" "step"
gdb_test "step" ".*" "step 4"; # Return from foo()
gdb_test "step" "main \\(\\) at.*" "step 5"
}
--
1.9.1