[PATCHv4] Make "skip" work on inline frames
Commit Message
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.
On 10/31/19 8:19 PM, Bernd Edlinger wrote:
> On 10/31/19 7:00 PM, Pedro Alves wrote:
>> On 10/31/19 4:53 PM, Simon Marchi wrote:
>>> On 2019-10-31 12:42 p.m., Pedro Alves wrote:
>>>> On 10/30/19 9:56 PM, Bernd Edlinger wrote:
>>>>> +if { [prepare_for_testing "failed to prepare" "skip2" \
>>>>> + {skip2.c skip1.c } \
>>>>> + {debug nowarnings optimize=-O2}] } {
>>>>
>>>> Instead of -O2, could you make this use -O0 (the default),
>>>> and then use attribute((always_inline)) to force inlining?
>>>> We do that in some tests. E.g., gdb.opt/inline-locals.c.
>>>
>>> I think that's a good suggestion, but just be aware that there used to be
>>> some problems with always_inline, e.g.:
>>
>> I don't think always_inline changes anything in the debug info special,
>> it just tells the compiler to inline the function even at -O0, which is
>> what we're after.
>>
>>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=13263
>>
>> This one seems to be about attribute((artificial)), and the desire
>> to not step into such functions automatically:
>>
>> "Given that I marked the function always_inline and artificial, I
>> was expecting not to step into its body."
>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=12429
>>>
>>
>> This one seems like a generic inlining issue.
>>
>>> I'm not sure if those are still valid. If they are, it might be more difficult
>>> that expected to use always_inline.
>> I don't think always_inline is anything special compared to inlining
>> because of -O2.
>>
>> Thanks,
>> Pedro Alves
>>
>
> Ah, thanks for that hint!
>
> always_inline works quite well.
>
> The debug session started (using gcc 4.8.4) with -O2 -g on the line with "{" in main,
> and with -O0 -g at the first real statement, so I had to remove one step.
> I did not worry about it initially, but now I agree that would have caused trouble.
>
>
> I attached both parts of the patch, the fist part unchanged from previous.
> The test case now makes sure to not repeat the names.
>
>
> Thanks
> Bernd.
>
Comments
Ping...
On 11/24/19 12:22 PM, 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.
>
> On 10/31/19 8:19 PM, Bernd Edlinger wrote:
>> On 10/31/19 7:00 PM, Pedro Alves wrote:
>>> On 10/31/19 4:53 PM, Simon Marchi wrote:
>>>> On 2019-10-31 12:42 p.m., Pedro Alves wrote:
>>>>> On 10/30/19 9:56 PM, Bernd Edlinger wrote:
>>>>>> +if { [prepare_for_testing "failed to prepare" "skip2" \
>>>>>> + {skip2.c skip1.c } \
>>>>>> + {debug nowarnings optimize=-O2}] } {
>>>>>
>>>>> Instead of -O2, could you make this use -O0 (the default),
>>>>> and then use attribute((always_inline)) to force inlining?
>>>>> We do that in some tests. E.g., gdb.opt/inline-locals.c.
>>>>
>>>> I think that's a good suggestion, but just be aware that there used to be
>>>> some problems with always_inline, e.g.:
>>>
>>> I don't think always_inline changes anything in the debug info special,
>>> it just tells the compiler to inline the function even at -O0, which is
>>> what we're after.
>>>
>>>>
>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=13263
>>>
>>> This one seems to be about attribute((artificial)), and the desire
>>> to not step into such functions automatically:
>>>
>>> "Given that I marked the function always_inline and artificial, I
>>> was expecting not to step into its body."
>>>
>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=12429
>>>>
>>>
>>> This one seems like a generic inlining issue.
>>>
>>>> I'm not sure if those are still valid. If they are, it might be more difficult
>>>> that expected to use always_inline.
>>> I don't think always_inline is anything special compared to inlining
>>> because of -O2.
>>>
>>> Thanks,
>>> Pedro Alves
>>>
>>
>> Ah, thanks for that hint!
>>
>> always_inline works quite well.
>>
>> The debug session started (using gcc 4.8.4) with -O2 -g on the line with "{" in main,
>> and with -O0 -g at the first real statement, so I had to remove one step.
>> I did not worry about it initially, but now I agree that would have caused trouble.
>>
>>
>> I attached both parts of the patch, the fist part unchanged from previous.
>> The test case now makes sure to not repeat the names.
>>
>>
>> Thanks
>> Bernd.
>>
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
From 4b8c278b30898ab37db3dcfb73d2b8d1f1773569 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/skip2.c | 64 ++++++++++++++++++++++++++++++++
gdb/testsuite/gdb.base/skip2.exp | 80 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 144 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/skip2.c
create mode 100644 gdb/testsuite/gdb.base/skip2.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 */
+ x = 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" "skip2" \
+ {skip2.c skip1.c } \
+ {debug nowarnings}] } {
+ return -1
+}
+
+set srcfile skip2.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