[PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines

[Guinevere Larsen]

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.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers