[PATCHv6] Make "skip" work on inline frames
Commit Message
On 12/15/19 1:46 AM, Simon Marchi wrote:
> 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.
Okay, I split that out as a separate patch now.
>
> 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).
>
Done. Also added changelog text, which I forgot previously.
> 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.
>
Done.
Is it OK for trunk?
Thanks
Bernd.
gdb:
2019-12-15 Bernd Edlinger <bernd.edlinger@hotmail.de>
* infcmd.c (prepare_one_step): Step over skipped inline functions.
* infrun.c (inline_frame_is_marked_for_skip): New helper function.
(process_event_stop_test): Keep stepping over skipped inline functions.
gdb/testsuite:
2019-12-15 Bernd Edlinger <bernd.edlinger@hotmail.de>
* gdb.base/skip-inline.c: New file.
* gdb.base/skip-inline.exp: New file.
Comments
On 2019-12-15 6:25 a.m., Bernd Edlinger wrote:
> On 12/15/19 1:46 AM, Simon Marchi wrote:
>> 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.
>
> Okay, I split that out as a separate patch now.
>
>>
>> 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).
>>
>
> Done. Also added changelog text, which I forgot previously.
>
>> 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.
>>
>
> Done.
>
>
> Is it OK for trunk?
That LGTM. I just remembered that your copyright assignment status was unclear, but I looked
up your name and saw that you filed one recently.
Would you like me to continue pushing your patches for you, or would you prefer to get push
access, so you are able to do so when they are approved?
Simon
On 12/15/19 2:12 PM, Simon Marchi wrote:
> On 2019-12-15 6:25 a.m., Bernd Edlinger wrote:
>> On 12/15/19 1:46 AM, Simon Marchi wrote:
>>> 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.
>>
>> Okay, I split that out as a separate patch now.
>>
>>>
>>> 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).
>>>
>>
>> Done. Also added changelog text, which I forgot previously.
>>
>>> 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.
>>>
>>
>> Done.
>>
>>
>> Is it OK for trunk?
>
> That LGTM. I just remembered that your copyright assignment status was unclear, but I looked
> up your name and saw that you filed one recently.
>
> Would you like me to continue pushing your patches for you, or would you prefer to get push
> access, so you are able to do so when they are approved?
>
Either way is fine for me.
Thanks
Bernd.
> Simon
>
On 2019-12-15 1:18 p.m., Bernd Edlinger wrote:
> On 12/15/19 2:12 PM, Simon Marchi wrote:
>> On 2019-12-15 6:25 a.m., Bernd Edlinger wrote:
>>> On 12/15/19 1:46 AM, Simon Marchi wrote:
>>>> 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.
>>>
>>> Okay, I split that out as a separate patch now.
>>>
>>>>
>>>> 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).
>>>>
>>>
>>> Done. Also added changelog text, which I forgot previously.
>>>
>>>> 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.
>>>>
>>>
>>> Done.
>>>
>>>
>>> Is it OK for trunk?
>>
>> That LGTM. I just remembered that your copyright assignment status was unclear, but I looked
>> up your name and saw that you filed one recently.
>>
>> Would you like me to continue pushing your patches for you, or would you prefer to get push
>> access, so you are able to do so when they are approved?
>>
>
> Either way is fine for me.
Ok, I'll push this one. In case you want an account, the form is there, you
can mention me as the person who approved your access.
https://sourceware.org/cgi-bin/pdw/ps_form.cgi
Simon
https://sourceware.org/cgi-bin/pdw/ps_form.cgi
On 12/17/19 3:00 AM, Simon Marchi wrote:
>
> Ok, I'll push this one. In case you want an account, the form is there, you
> can mention me as the person who approved your access.
>
Will do.
Thanks a lot for you review.
Bernd.
From 7bee2af24e4f21ae449f604ac549492d5fcab1a4 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
---
gdb/testsuite/gdb.base/skip-inline.c | 64 ++++++++++++++++++++++++++++
gdb/testsuite/gdb.base/skip-inline.exp | 78 ++++++++++++++++++++++++++++++++++
2 files changed, 142 insertions(+)
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,78 @@
+# 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/>.
+
+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"
+}
--
1.9.1