Patchwork [PATCHv4] Make "skip" work on inline frames

login
register
mail settings
Submitter Bernd Edlinger
Date Nov. 24, 2019, 11:22 a.m.
Message ID <VI1PR03MB45287E5B07A2E14ACFD07559E44B0@VI1PR03MB4528.eurprd03.prod.outlook.com>
Download mbox | patch
Permalink /patch/36160/
State New
Headers show

Comments

Bernd Edlinger - Nov. 24, 2019, 11:22 a.m.
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.
>
Bernd Edlinger - Dec. 1, 2019, 8:46 p.m.
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.
>>
Simon Marchi - Dec. 2, 2019, 2:34 a.m.
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

Patch

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

diff --git a/gdb/testsuite/gdb.base/skip2.c b/gdb/testsuite/gdb.base/skip2.c
new file mode 100644
index 0000000..d1abd45
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.c
@@ -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 ();
+}
diff --git a/gdb/testsuite/gdb.base/skip2.exp b/gdb/testsuite/gdb.base/skip2.exp
new file mode 100644
index 0000000..a59dd0f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip2.exp
@@ -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