[v5,1/3] Fix handling of DW_AT_entry_pc of inlined subroutines
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
It may happen that the inline entry point is not the
start address of the first sub-range of an inline
function.
But the PC for a breakpoint on an inlined subroutine
is always the start address of the first sub-range.
This patch moves the sub-range starting at the entry
point to the first position of the block list.
Therefore the breakpoint on an inlined function
changes in rare cases from the start address of
the first sub-range to the real entry point.
There should always be a subrange that starts at the
entry point, even if that is an empty sub-range.
---
gdb/dwarf2/read.c | 12 +++++++
gdb/testsuite/gdb.base/inline-entry.c | 39 ++++++++++++++++++++++
gdb/testsuite/gdb.base/inline-entry.exp | 43 +++++++++++++++++++++++++
3 files changed, 94 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/inline-entry.c
create mode 100644 gdb/testsuite/gdb.base/inline-entry.exp
Comments
On 9/2/24 11:57 AM, Bernd Edlinger wrote:
> It may happen that the inline entry point is not the
> start address of the first sub-range of an inline
> function.
>
> But the PC for a breakpoint on an inlined subroutine
> is always the start address of the first sub-range.
>
> This patch moves the sub-range starting at the entry
> point to the first position of the block list.
>
> Therefore the breakpoint on an inlined function
> changes in rare cases from the start address of
> the first sub-range to the real entry point.
>
> There should always be a subrange that starts at the
> entry point, even if that is an empty sub-range.
Hi Bernd,
Sorry for the long time for reviews, and thank you for the persistence.
I have a couple of comments - mostly quality of life - about the test
case. Those comments are just suggestions, regardless, this patch looks
good for what is worth, Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
> ---
> gdb/dwarf2/read.c | 12 +++++++
> gdb/testsuite/gdb.base/inline-entry.c | 39 ++++++++++++++++++++++
> gdb/testsuite/gdb.base/inline-entry.exp | 43 +++++++++++++++++++++++++
> 3 files changed, 94 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/inline-entry.c
> create mode 100644 gdb/testsuite/gdb.base/inline-entry.exp
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 769ca91facc..95155a2ee59 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -11276,6 +11276,14 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
> if (die->tag != DW_TAG_compile_unit)
> ranges_offset += cu->gnu_ranges_base;
>
> + CORE_ADDR entry_pc = (CORE_ADDR) -1;
> + if (die->tag == DW_TAG_inlined_subroutine)
> + {
> + attr = dwarf2_attr (die, DW_AT_entry_pc, cu);
> + if (attr != nullptr && !attr->form_is_constant ())
> + entry_pc = per_objfile->relocate (attr->as_address ());
> + }
> +
> std::vector<blockrange> blockvec;
> dwarf2_ranges_process (ranges_offset, cu, die->tag,
> [&] (unrelocated_addr start, unrelocated_addr end)
> @@ -11285,6 +11293,10 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
> cu->get_builder ()->record_block_range (block, abs_start,
> abs_end - 1);
> blockvec.emplace_back (abs_start, abs_end);
> + /* This ensures that blockvec[0] is the one that starts
> + at entry_pc, see block::entry_pc. */
> + if (entry_pc == abs_start && blockvec.size () > 1)
> + std::swap (blockvec[0], blockvec[blockvec.size () - 1]);
> });
>
> block->set_ranges (make_blockranges (objfile, blockvec));
> diff --git a/gdb/testsuite/gdb.base/inline-entry.c b/gdb/testsuite/gdb.base/inline-entry.c
> new file mode 100644
> index 00000000000..bc36d8f9483
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/inline-entry.c
> @@ -0,0 +1,39 @@
> +/* Copyright 2024 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/>. */
> +
> +volatile int global = 0;
> +
> +__attribute__((noinline, noclone)) void
> +foo (int arg)
> +{
> + global += arg;
> +}
> +
> +inline __attribute__((always_inline)) int
> +bar (int val)
> +{
> + if (__builtin_expect(global == val, 0))
> + return 1;
> + foo (1);
> + return 1;
> +}
> +
> +int
> +main (void)
> +{
> + if ((__builtin_expect(global, 0) && bar (1)) || bar (2))
> + return 1;
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/inline-entry.exp b/gdb/testsuite/gdb.base/inline-entry.exp
> new file mode 100644
> index 00000000000..20e112f7269
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/inline-entry.exp
> @@ -0,0 +1,43 @@
> +# Copyright 2024 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 .c
> +
> +if { [test_compiler_info gcc*] && ![supports_statement_frontiers] } {
> + untested "this test needs gcc with statement frontiers"
> + return -1
> +}
> +
> +global srcfile testfile
> +
> +set options {debug nowarnings optimize=-O2}
> +if { [supports_statement_frontiers] } {
> + lappend options additional_flags=-gstatement-frontiers
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $binfile \
> + $srcfile $options] } {
> + return -1
> +}
> +
> +if ![runto_main] {
> + return
> +}
> +
> +gdb_test "break bar" ".*Breakpoint 2 at .*" "break at bar"
> +gdb_test "break foo" ".*Breakpoint 3 at .*" "break at foo"
These 2 tests could be replaced by gdb_breakpoint "bar" and
gdb_breakpoint "foo".
> +gdb_test "continue" ".*Breakpoint .* bar .*" "continue to bar"
> +gdb_test "continue" ".*Breakpoint .* foo .*" "continue to foo"
And these 2 could be replaced by gdb_continue_to_breakpoint and a unique
regexp identifying the line you'll be reaching.
> +gdb_test "continue" ".* exited .*" "continue to exit"
And this could be replaced to gdb_continue_to_end.
I also have a genuine question. Is it possible to verify that gcc
produced the desired output? I understand that all versions you tested
work, but if it wasn't too much work, I would like it if this test was
future proofed. I am not really sure what I'd be looking for, so I can't
really help with a suggestion, and again, this isn't a hard requirement,
so if you can't think of anything, it's fine.
@@ -11276,6 +11276,14 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
if (die->tag != DW_TAG_compile_unit)
ranges_offset += cu->gnu_ranges_base;
+ CORE_ADDR entry_pc = (CORE_ADDR) -1;
+ if (die->tag == DW_TAG_inlined_subroutine)
+ {
+ attr = dwarf2_attr (die, DW_AT_entry_pc, cu);
+ if (attr != nullptr && !attr->form_is_constant ())
+ entry_pc = per_objfile->relocate (attr->as_address ());
+ }
+
std::vector<blockrange> blockvec;
dwarf2_ranges_process (ranges_offset, cu, die->tag,
[&] (unrelocated_addr start, unrelocated_addr end)
@@ -11285,6 +11293,10 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
cu->get_builder ()->record_block_range (block, abs_start,
abs_end - 1);
blockvec.emplace_back (abs_start, abs_end);
+ /* This ensures that blockvec[0] is the one that starts
+ at entry_pc, see block::entry_pc. */
+ if (entry_pc == abs_start && blockvec.size () > 1)
+ std::swap (blockvec[0], blockvec[blockvec.size () - 1]);
});
block->set_ranges (make_blockranges (objfile, blockvec));
new file mode 100644
@@ -0,0 +1,39 @@
+/* Copyright 2024 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/>. */
+
+volatile int global = 0;
+
+__attribute__((noinline, noclone)) void
+foo (int arg)
+{
+ global += arg;
+}
+
+inline __attribute__((always_inline)) int
+bar (int val)
+{
+ if (__builtin_expect(global == val, 0))
+ return 1;
+ foo (1);
+ return 1;
+}
+
+int
+main (void)
+{
+ if ((__builtin_expect(global, 0) && bar (1)) || bar (2))
+ return 1;
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,43 @@
+# Copyright 2024 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 .c
+
+if { [test_compiler_info gcc*] && ![supports_statement_frontiers] } {
+ untested "this test needs gcc with statement frontiers"
+ return -1
+}
+
+global srcfile testfile
+
+set options {debug nowarnings optimize=-O2}
+if { [supports_statement_frontiers] } {
+ lappend options additional_flags=-gstatement-frontiers
+}
+
+if { [prepare_for_testing "failed to prepare" $binfile \
+ $srcfile $options] } {
+ return -1
+}
+
+if ![runto_main] {
+ return
+}
+
+gdb_test "break bar" ".*Breakpoint 2 at .*" "break at bar"
+gdb_test "break foo" ".*Breakpoint 3 at .*" "break at foo"
+gdb_test "continue" ".*Breakpoint .* bar .*" "continue to bar"
+gdb_test "continue" ".*Breakpoint .* foo .*" "continue to foo"
+gdb_test "continue" ".* exited .*" "continue to exit"