* [PATCH v5 0/3] Improve debugging of optimized code
@ 2024-09-02 14:55 Bernd Edlinger
2024-09-02 14:57 ` [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines Bernd Edlinger
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Bernd Edlinger @ 2024-09-02 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Guinevere Larsen
These patches improve the debug experience with optimized
gcc code significantly, as can be seen by the test cases.
The problem is that there are cases where an inline function
has line table entries at the end of the inline subrange,
and it is unclear by the dwarf-5 debug info if these line
table entries belong to the called inline function or to
the calling outer function, while there are at least some
corner cases where by stepping into these lines e.g. the
return value might be inspected. Also the call stack is
often displayed wrong because the line table entries
do not match with the block structure i.e. are obviously
not in the correct function.
Therefore this patch series tries to improve the issue by
introducing a heuristic, that marks some of the line table
entries that happen to be at the exact end of an inline
function range as weak. This new line table flag can be
used to avoid to step into the inline function when a
step over was requested, and display the line belonging
to the correct function in the callstack.
I'm sorry, that this solution has quite some complexity,
but I promise it is worth it. Please give this patch
a try and look for yourself how the debug experience is
improved in the test cases, and certainly also a number of
other programs, especially with C++, where inline functions
are pretty much used everywhere.
v3: minor update to fix regressions in gdb.cp/empty-inline.exp
This update fixes only one test case, that turned out, not to
work properly with arm and aarch64 targets (many thanks to Linaro
for reporting this to me). The reason was that the call stack
looks slightly different when the trap is on a statement boundary
or not. So the test expectation was changed to ignore that bit.
Additionally the test case was failing on riscv simulator targets
because assignment to NULL is not enough to cause an exception
on a simulator. However assigning to (char*)-1 triggers always
an exception.
v4: Added a simple test case for part 1,
improved the commit message of part 2,
improved test cases for part 3, avoiding issues with clang.
I finally was able to find a realistic but still quite simple example
where the inline entry_pc is not at the first subrange, and where
this actually makes a user-visible difference. Fortunately it is
reproducible with almost all gcc versions I tried so far, therefore
I did not have to create an artificial dwarf test for this issue.
Other than that I also added a check that the entry_pc is not in the
form of a constant, it will usually be an address.
As requested in the review I've added an example how the new line
table will look like in the commit message of part 2,
and revised the test cases of part 3, to avoid possible issues
with clang.
There are some issues with clang but they are different than gcc's,
furthermore the clang debug info is not affected by these patches,
therefore the test cases are better skipped with clang.
v5: Fixed an issue with subroutines containing inner lexical blocks.
Bernd Edlinger (3):
Fix handling of DW_AT_entry_pc of inlined subroutines
Introduce a new line table flag is_weak
Fix range end handling of inlined subroutines
gdb/block.c | 15 +-
gdb/buildsym.c | 106 +++++++--
gdb/buildsym.h | 3 +
gdb/dwarf2/read.c | 88 +++-----
gdb/infcmd.c | 3 +-
gdb/infrun.c | 33 ++-
gdb/jit.c | 1 +
gdb/symmisc.c | 6 +-
gdb/symtab.c | 17 +-
gdb/symtab.h | 5 +
gdb/testsuite/gdb.base/empty-inline.c | 39 ++++
gdb/testsuite/gdb.base/empty-inline.exp | 51 +++++
gdb/testsuite/gdb.base/inline-entry.c | 39 ++++
gdb/testsuite/gdb.base/inline-entry.exp | 43 ++++
gdb/testsuite/gdb.cp/empty-inline.cc | 33 +++
gdb/testsuite/gdb.cp/empty-inline.exp | 51 +++++
gdb/testsuite/gdb.cp/step-and-next-inline.cc | 6 +-
gdb/testsuite/gdb.cp/step-and-next-inline.exp | 201 +++++++-----------
gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp | 2 +-
gdb/xcoffread.c | 1 +
20 files changed, 524 insertions(+), 219 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/empty-inline.c
create mode 100644 gdb/testsuite/gdb.base/empty-inline.exp
create mode 100644 gdb/testsuite/gdb.base/inline-entry.c
create mode 100644 gdb/testsuite/gdb.base/inline-entry.exp
create mode 100644 gdb/testsuite/gdb.cp/empty-inline.cc
create mode 100644 gdb/testsuite/gdb.cp/empty-inline.exp
--
2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines
2024-09-02 14:55 [PATCH v5 0/3] Improve debugging of optimized code Bernd Edlinger
@ 2024-09-02 14:57 ` Bernd Edlinger
2024-09-10 20:34 ` Guinevere Larsen
2024-10-16 15:49 ` Andrew Burgess
2024-09-02 14:58 ` [PATCH v5 2/3] Introduce a new line table flag is_weak Bernd Edlinger
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Bernd Edlinger @ 2024-09-02 14:57 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Guinevere Larsen
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
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"
+gdb_test "continue" ".*Breakpoint .* bar .*" "continue to bar"
+gdb_test "continue" ".*Breakpoint .* foo .*" "continue to foo"
+gdb_test "continue" ".* exited .*" "continue to exit"
--
2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 2/3] Introduce a new line table flag is_weak
2024-09-02 14:55 [PATCH v5 0/3] Improve debugging of optimized code Bernd Edlinger
2024-09-02 14:57 ` [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines Bernd Edlinger
@ 2024-09-02 14:58 ` Bernd Edlinger
2024-09-13 16:04 ` Guinevere Larsen
2024-09-02 15:00 ` [PATCH v5 3/3] Fix range end handling of inlined subroutines Bernd Edlinger
2024-09-26 17:40 ` [PATCH v5 0/3] Improve debugging of optimized code Andrew Burgess
3 siblings, 1 reply; 9+ messages in thread
From: Bernd Edlinger @ 2024-09-02 14:58 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Guinevere Larsen
This introduces a new line table flag is_weak.
The line entries at the end of a subroutine range,
use this to indicate that they may possibly
be part of the previous subroutine.
When there is a sequence of line entries at the
same address where an inline range ends, and the
last item has is_stmt = 0, we force all previous
items to have is_weak = 1.
Additionally this adds a "fake" end sequence to the
record_line function, that is line number -1.
That will be used in the next patch.
Finally this adds a handling for empty ranges to
record_block_range. Currently this function is
not called with empty ranges, but that will be used
in the next patch.
There should be no functional changes after this commit.
Well, except the new is-weak flag in the line table of course.
As an example consider the following test code:
$ cat test.c
inline int test ()
{
asm ("nop");
return 0;
}
int main ()
{
int x = test ();
return x;
}
$ gcc -g -O2 test.c
This will receive the following line table:
(gdb) maintenance info line-table
INDEX LINE REL-ADDRESS UNREL-ADDRESS IS-STMT IS-WEAK PROLOGUE-END EPILOGUE-BEGIN
0 8 0x0000555555555040 0x0000000000001040 Y
1 9 0x0000555555555040 0x0000000000001040 Y
2 1 0x0000555555555040 0x0000000000001040 Y
3 3 0x0000555555555040 0x0000000000001040 Y
4 4 0x0000555555555041 0x0000000000001041 Y Y
5 4 0x0000555555555041 0x0000000000001041 Y <---+ set is_weak
6 10 0x0000555555555041 0x0000000000001041 Y Y ^
7 11 0x0000555555555041 0x0000000000001041 <----+ no is-stmt
8 END 0x0000555555555044 0x0000000000001044 Y
---
gdb/buildsym.c | 106 ++++++++++++++++---
gdb/buildsym.h | 3 +
gdb/jit.c | 1 +
gdb/symmisc.c | 6 +-
gdb/symtab.h | 5 +
gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp | 2 +-
gdb/xcoffread.c | 1 +
7 files changed, 109 insertions(+), 15 deletions(-)
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 02d6848f3a4..2e552fa9046 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -413,6 +413,16 @@ buildsym_compunit::record_block_range (struct block *block,
|| end_inclusive + 1 != block->end ())
m_pending_addrmap_interesting = true;
+ if (block->inlined_p ())
+ {
+ m_inline_end_vector.push_back (end_inclusive + 1);
+ if (end_inclusive + 1 == start)
+ {
+ end_inclusive = start;
+ m_pending_addrmap_interesting = true;
+ }
+ }
+
m_pending_addrmap.set_empty (start, end_inclusive, block);
}
@@ -627,19 +637,16 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
{
m_have_line_numbers = true;
- /* Normally, we treat lines as unsorted. But the end of sequence
- marker is special. We sort line markers at the same PC by line
- number, so end of sequence markers (which have line == 0) appear
- first. This is right if the marker ends the previous function,
- and there is no padding before the next function. But it is
- wrong if the previous line was empty and we are now marking a
- switch to a different subfile. We must leave the end of sequence
- marker at the end of this group of lines, not sort the empty line
- to after the marker. The easiest way to accomplish this is to
- delete any empty lines from our table, if they are followed by
- end of sequence markers. All we lose is the ability to set
- breakpoints at some lines which contain no instructions
- anyway. */
+ /* The end of sequence marker is special. We need to delete any
+ previous lines at the same PC, otherwise these lines may cause
+ problems since they might be at the same address as the following
+ function. For instance suppose a function calls abort there is no
+ reason to emit a ret after that point (no joke).
+ So the label may be at the same address where the following
+ function begins. There is also a fake end of sequence marker (-1)
+ that we emit internally when switching between different CUs
+ In this case, duplicate line table entries shall not be deleted.
+ We simply set the is_weak marker in this case. */
if (line == 0)
{
std::optional<int> last_line;
@@ -659,15 +666,84 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
if (!last_line.has_value () || *last_line == 0)
return;
}
+ else if (line == -1)
+ {
+ line = 0;
+ auto e = subfile->line_vector_entries.end ();
+ while (e > subfile->line_vector_entries.begin ())
+ {
+ e--;
+ if (e->unrelocated_pc () != pc)
+ break;
+ e->is_weak = 1;
+ }
+ }
linetable_entry &e = subfile->line_vector_entries.emplace_back ();
e.line = line;
e.is_stmt = (flags & LEF_IS_STMT) != 0;
+ e.is_weak = false;
e.set_unrelocated_pc (pc);
e.prologue_end = (flags & LEF_PROLOGUE_END) != 0;
e.epilogue_begin = (flags & LEF_EPILOGUE_BEGIN) != 0;
}
+\f
+/* Patch the is_stmt bits at the given inline end address.
+ The line table has to be already sorted. */
+
+static void
+patch_inline_end_pos (struct subfile *subfile, struct objfile *objfile,
+ CORE_ADDR end)
+{
+ std::vector<linetable_entry> &items = subfile->line_vector_entries;
+ int a = 2, b = items.size () - 1;
+
+ /* We need at least two items with pc = end in the table.
+ The lowest usable items are at pos 0 and 1, the highest
+ usable items are at pos b - 2 and b - 1. */
+ if (a > b
+ || end < items[1].pc (objfile)
+ || end > items[b - 2].pc (objfile))
+ return;
+
+ /* Look for the first item with pc > end in the range [a,b].
+ The previous element has pc = end or there is no match.
+ We set a = 2, since we need at least two consecutive elements
+ with pc = end to do anything useful.
+ We set b = items.size () - 1, since we are not interested
+ in the last element which should be an end of sequence
+ marker with line = 0 and is_stmt = true. */
+ while (a < b)
+ {
+ int c = (a + b) / 2;
+
+ if (end < items[c].pc (objfile))
+ b = c;
+ else
+ a = c + 1;
+ }
+
+ a--;
+ if (items[a].pc (objfile) != end || items[a].is_stmt)
+ return;
+
+ /* When there is a sequence of line entries at the same address
+ where an inline range ends, and the last item has is_stmt = 0,
+ we force all previous items to have is_weak = true as well. */
+ do
+ {
+ /* We stop at the first line entry with a different address,
+ or when we see an end of sequence marker. */
+ a--;
+ if (items[a].pc (objfile) != end || items[a].line == 0)
+ break;
+
+ items[a].is_weak = true;
+ }
+ while (a > 0);
+}
+
\f
/* Subroutine of end_compunit_symtab to simplify it. Look for a subfile that
matches the main source file's basename. If there is only one, and
@@ -892,6 +968,10 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
relationships, this is why std::stable_sort is used. */
std::stable_sort (subfile->line_vector_entries.begin (),
subfile->line_vector_entries.end ());
+
+ for (int i = 0; i < m_inline_end_vector.size (); i++)
+ patch_inline_end_pos (subfile, m_objfile,
+ m_inline_end_vector[i]);
}
/* Allocate a symbol table if necessary. */
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index c1eed247d25..edf76c8b17c 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -446,6 +446,9 @@ struct buildsym_compunit
/* Pending symbols that are local to the lexical context. */
struct pending *m_local_symbols = nullptr;
+
+ /* Pending inline end range addresses. */
+ std::vector<CORE_ADDR> m_inline_end_vector;
};
\f
diff --git a/gdb/jit.c b/gdb/jit.c
index 89c967db615..3e9353e206b 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -495,6 +495,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
(unrelocated_addr (map[i].pc));
stab->linetable->item[i].line = map[i].line;
stab->linetable->item[i].is_stmt = true;
+ stab->linetable->item[i].is_weak = false;
}
}
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 7f8141588b7..eb1487b39b0 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -265,6 +265,8 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
gdb_puts (paddress (gdbarch, l->item[i].pc (objfile)), outfile);
if (l->item[i].is_stmt)
gdb_printf (outfile, "\t(stmt)");
+ if (l->item[i].is_weak)
+ gdb_printf (outfile, "\t(weak)");
gdb_printf (outfile, "\n");
}
}
@@ -972,12 +974,13 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
/* Leave space for 6 digits of index and line number. After that the
tables will just not format as well. */
struct ui_out *uiout = current_uiout;
- ui_out_emit_table table_emitter (uiout, 7, -1, "line-table");
+ ui_out_emit_table table_emitter (uiout, 8, -1, "line-table");
uiout->table_header (6, ui_left, "index", _("INDEX"));
uiout->table_header (6, ui_left, "line", _("LINE"));
uiout->table_header (18, ui_left, "rel-address", _("REL-ADDRESS"));
uiout->table_header (18, ui_left, "unrel-address", _("UNREL-ADDRESS"));
uiout->table_header (7, ui_left, "is-stmt", _("IS-STMT"));
+ uiout->table_header (7, ui_left, "is-weak", _("IS-WEAK"));
uiout->table_header (12, ui_left, "prologue-end", _("PROLOGUE-END"));
uiout->table_header (14, ui_left, "epilogue-begin", _("EPILOGUE-BEGIN"));
uiout->table_body ();
@@ -998,6 +1001,7 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
uiout->field_core_addr ("unrel-address", objfile->arch (),
CORE_ADDR (item->unrelocated_pc ()));
uiout->field_string ("is-stmt", item->is_stmt ? "Y" : "");
+ uiout->field_string ("is-weak", item->is_weak ? "Y" : "");
uiout->field_string ("prologue-end", item->prologue_end ? "Y" : "");
uiout->field_string ("epilogue-begin", item->epilogue_begin ? "Y" : "");
uiout->text ("\n");
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 4197a3a164c..188d7e68923 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1656,6 +1656,9 @@ struct linetable_entry
/* True if this PC is a good location to place a breakpoint for LINE. */
bool is_stmt : 1;
+ /* True if this PC is at a subroutine range end. */
+ bool is_weak : 1;
+
/* True if this location is a good location to place a breakpoint after a
function prologue. */
bool prologue_end : 1;
@@ -2412,6 +2415,8 @@ struct symtab_and_line
/* If the line number information is valid, then this indicates if this
line table entry had the is-stmt flag set or not. */
bool is_stmt = false;
+ /* True if this PC is at a subroutine range end. */
+ bool is_weak = false;
/* The probe associated with this symtab_and_line. */
probe *prob = NULL;
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp b/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
index 1a3d53c2116..403bc6b3f72 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
@@ -168,7 +168,7 @@ gdb_test_multiple "maint info line-table gdb.dwarf2/dw2-ranges-base.c" \
-re ".*linetable: \\(\\(struct linetable \\*\\) 0x0\\):\r\nNo line table.\r\n" {
exp_continue
}
- -re ".*linetable: \\(\\(struct linetable \\*\\) $hex\\):\r\nINDEX\[ \t\]+LINE\[ \t\]+REL-ADDRESS\[ \t\]+UNREL-ADDRESS\[ \t\]+IS-STMT\[ \t\]PROLOGUE-END\[ \t\]EPILOGUE-BEGIN *\r\n" {
+ -re ".*linetable: \\(\\(struct linetable \\*\\) $hex\\):\r\nINDEX\[ \t\]+LINE\[ \t\]+REL-ADDRESS\[ \t\]+UNREL-ADDRESS\[ \t\]+IS-STMT\[ \t\]IS-WEAK\[ \t\]PROLOGUE-END\[ \t\]EPILOGUE-BEGIN *\r\n" {
exp_continue
}
}
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 639dd5b8adc..1642bc12dfb 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -431,6 +431,7 @@ arrange_linetable (std::vector<linetable_entry> &old_linetable)
linetable_entry &e = fentries.emplace_back ();
e.line = ii;
e.is_stmt = true;
+ e.is_weak = false;
e.set_unrelocated_pc (old_linetable[ii].unrelocated_pc ());
}
}
--
2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 3/3] Fix range end handling of inlined subroutines
2024-09-02 14:55 [PATCH v5 0/3] Improve debugging of optimized code Bernd Edlinger
2024-09-02 14:57 ` [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines Bernd Edlinger
2024-09-02 14:58 ` [PATCH v5 2/3] Introduce a new line table flag is_weak Bernd Edlinger
@ 2024-09-02 15:00 ` Bernd Edlinger
2024-10-18 10:47 ` Andrew Burgess
2024-09-26 17:40 ` [PATCH v5 0/3] Improve debugging of optimized code Andrew Burgess
3 siblings, 1 reply; 9+ messages in thread
From: Bernd Edlinger @ 2024-09-02 15:00 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Guinevere Larsen
Currently there is a problem when debugging
optimized code when the inferior stops at an inline
sub-range end PC. It is unclear if that location
is from the inline function or from the calling
function. Therefore the call stack is often
wrong.
This patch detects the "weak" line table entries
which are likely part of the previous inline block,
and if we have such a location, it assumes the
location belongs to the previous block.
Additionally it may happen that the infrun machinery
steps from one inline range to another inline range
of the same inline function. That can look like
jumping back and forth from the calling program
to the inline function, while really the inline
function just jumps from a hot to a cold section
of the code, i.e. error handling.
Additionally it may happen that one inline sub-range
is empty or the inline is completely empty. But
filtering that information away is not the right
solution, since although there is no actual code
from the inline, it is still possible that variables
from an inline function can be inspected here.
The issue with the empty ranges is also discussed here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94474
Conceptually this patch uses a heuristic to work around
a deficiency in the dwarf-4 and dwarf-5 rnglists structure.
There should be a location view number for each inline
sub-range begin PC and end PC, similar to the DW_AT_GNU_entry_view
which is the location view for the inline entry point.
---
gdb/block.c | 15 +-
gdb/dwarf2/read.c | 76 ++-----
gdb/infcmd.c | 3 +-
gdb/infrun.c | 33 ++-
gdb/symtab.c | 17 +-
gdb/testsuite/gdb.base/empty-inline.c | 39 ++++
gdb/testsuite/gdb.base/empty-inline.exp | 51 +++++
gdb/testsuite/gdb.cp/empty-inline.cc | 33 +++
gdb/testsuite/gdb.cp/empty-inline.exp | 51 +++++
gdb/testsuite/gdb.cp/step-and-next-inline.cc | 6 +-
gdb/testsuite/gdb.cp/step-and-next-inline.exp | 201 +++++++-----------
11 files changed, 321 insertions(+), 204 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/empty-inline.c
create mode 100644 gdb/testsuite/gdb.base/empty-inline.exp
create mode 100644 gdb/testsuite/gdb.cp/empty-inline.cc
create mode 100644 gdb/testsuite/gdb.cp/empty-inline.exp
diff --git a/gdb/block.c b/gdb/block.c
index 6f608777854..b2fff0c2b6b 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -196,7 +196,20 @@ blockvector_for_pc_sect (CORE_ADDR pc, struct obj_section *section,
return NULL;
if (pblock)
- *pblock = b;
+ {
+ struct symtab_and_line sal = find_pc_sect_line (pc, section, 0);
+ if (sal.line != 0 && sal.pc == pc && sal.is_weak)
+ {
+ const struct block *b2 = find_block_in_blockvector (bl, pc - 1);
+ const struct block *b0 = b;
+ while (b0->superblock () && !b0->function ())
+ b0 = b0->superblock ();
+ if (b0->contains (b2))
+ b = b2;
+ }
+ *pblock = b;
+ }
+
return bl;
}
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 95155a2ee59..39a058c595a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -10763,10 +10763,6 @@ dwarf2_rnglists_process (unsigned offset, struct dwarf2_cu *cu,
return false;
}
- /* Empty range entries have no effect. */
- if (range_beginning == range_end)
- continue;
-
/* Only DW_RLE_offset_pair needs the base address added. */
if (rlet == DW_RLE_offset_pair)
{
@@ -10885,10 +10881,6 @@ dwarf2_ranges_process (unsigned offset, struct dwarf2_cu *cu, dwarf_tag tag,
return 0;
}
- /* Empty range entries have no effect. */
- if (range_beginning == range_end)
- continue;
-
range_beginning = (unrelocated_addr) ((CORE_ADDR) range_beginning
+ (CORE_ADDR) *base);
range_end = (unrelocated_addr) ((CORE_ADDR) range_end
@@ -11110,8 +11102,7 @@ dwarf2_get_pc_bounds (struct die_info *die, unrelocated_addr *lowpc,
if (ret == PC_BOUNDS_NOT_PRESENT || ret == PC_BOUNDS_INVALID)
return ret;
- /* partial_die_info::read has also the strict LOW < HIGH requirement. */
- if (high <= low)
+ if (low > high || (low == high && die->tag != DW_TAG_inlined_subroutine))
return PC_BOUNDS_INVALID;
/* When using the GNU linker, .gnu.linkonce. sections are used to
@@ -18062,21 +18053,9 @@ class lnp_state_machine
/* Additional bits of state we need to track. */
- /* The last file that we called dwarf2_start_subfile for.
- This is only used for TLLs. */
- unsigned int m_last_file = 0;
/* The last file a line number was recorded for. */
struct subfile *m_last_subfile = NULL;
- /* The address of the last line entry. */
- unrelocated_addr m_last_address;
-
- /* Set to true when a previous line at the same address (using
- m_last_address) had LEF_IS_STMT set in m_flags. This is reset to false
- when a line entry at a new address (m_address different to
- m_last_address) is processed. */
- bool m_stmt_at_address = false;
-
/* When true, record the lines we decode. */
bool m_currently_recording_lines = true;
@@ -18234,7 +18213,8 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
static void
dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
- unrelocated_addr address, struct dwarf2_cu *cu)
+ unrelocated_addr address, struct dwarf2_cu *cu,
+ bool end_sequence)
{
if (subfile == NULL)
return;
@@ -18247,7 +18227,8 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
paddress (gdbarch, (CORE_ADDR) address));
}
- dwarf_record_line_1 (gdbarch, subfile, 0, address, LEF_IS_STMT, cu);
+ dwarf_record_line_1 (gdbarch, subfile, end_sequence ? 0 : -1, address,
+ LEF_IS_STMT, cu);
}
void
@@ -18275,38 +18256,17 @@ lnp_state_machine::record_line (bool end_sequence)
/* For now we ignore lines not starting on an instruction boundary.
But not when processing end_sequence for compatibility with the
previous version of the code. */
- else if (m_op_index == 0 || end_sequence)
- {
- /* When we switch files we insert an end maker in the first file,
- switch to the second file and add a new line entry. The
- problem is that the end marker inserted in the first file will
- discard any previous line entries at the same address. If the
- line entries in the first file are marked as is-stmt, while
- the new line in the second file is non-stmt, then this means
- the end marker will discard is-stmt lines so we can have a
- non-stmt line. This means that there are less addresses at
- which the user can insert a breakpoint.
-
- To improve this we track the last address in m_last_address,
- and whether we have seen an is-stmt at this address. Then
- when switching files, if we have seen a stmt at the current
- address, and we are switching to create a non-stmt line, then
- discard the new line. */
- bool file_changed
- = m_last_subfile != m_cu->get_builder ()->get_current_subfile ();
- bool ignore_this_line
- = ((file_changed && !end_sequence && m_last_address == m_address
- && ((m_flags & LEF_IS_STMT) == 0)
- && m_stmt_at_address)
- || (!end_sequence && m_line == 0));
-
- if ((file_changed && !ignore_this_line) || end_sequence)
+ else if ((m_op_index == 0 && m_line != 0) || end_sequence)
+ {
+ if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
+ || end_sequence)
{
dwarf_finish_line (m_gdbarch, m_last_subfile, m_address,
- m_currently_recording_lines ? m_cu : nullptr);
+ m_currently_recording_lines ? m_cu : nullptr,
+ end_sequence || (m_flags & LEF_IS_STMT) != 0);
}
- if (!end_sequence && !ignore_this_line)
+ if (!end_sequence)
{
linetable_entry_flags lte_flags = m_flags;
if (producer_is_codewarrior (m_cu))
@@ -18326,15 +18286,6 @@ lnp_state_machine::record_line (bool end_sequence)
m_last_line = m_line;
}
}
-
- /* Track whether we have seen any IS_STMT true at m_address in case we
- have multiple line table entries all at m_address. */
- if (m_last_address != m_address)
- {
- m_stmt_at_address = false;
- m_last_address = m_address;
- }
- m_stmt_at_address |= (m_flags & LEF_IS_STMT) != 0;
}
lnp_state_machine::lnp_state_machine (struct dwarf2_cu *cu, gdbarch *arch,
@@ -18348,8 +18299,7 @@ lnp_state_machine::lnp_state_machine (struct dwarf2_cu *cu, gdbarch *arch,
This is currently used by MIPS code,
cf. `mips_adjust_dwarf2_line'. */
m_address ((unrelocated_addr) gdbarch_adjust_dwarf2_line (arch, 0, 0)),
- m_flags (lh->default_is_stmt ? LEF_IS_STMT : (linetable_entry_flags) 0),
- m_last_address (m_address)
+ m_flags (lh->default_is_stmt ? LEF_IS_STMT : (linetable_entry_flags) 0)
{
}
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 90fa4f6c969..d4304c87ee4 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -996,7 +996,8 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
if (sym->aclass () == LOC_BLOCK)
{
const block *block = sym->value_block ();
- if (block->end () < tp->control.step_range_end)
+ if (block->end () < tp->control.step_range_end
+ && block->end () > tp->control.step_range_start)
tp->control.step_range_end = block->end ();
}
}
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4ca15450afe..ee4a05c02f6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8114,6 +8114,34 @@ process_event_stop_test (struct execution_control_state *ecs)
return;
}
+ /* Handle the case when subroutines have multiple ranges.
+ When we step from one part to the next part of the same subroutine,
+ all subroutine levels are skipped again which begin here.
+ Compensate for this by removing all skipped subroutines,
+ which were already executing from the user's perspective. */
+
+ if (get_stack_frame_id (frame)
+ == ecs->event_thread->control.step_stack_frame_id
+ && inline_skipped_frames (ecs->event_thread)
+ && ecs->event_thread->control.step_frame_id.artificial_depth > 0
+ && ecs->event_thread->control.step_frame_id.code_addr_p)
+ {
+ const struct block *prev, *curr;
+ int depth = 0;
+ prev = block_for_pc (ecs->event_thread->control.step_frame_id.code_addr);
+ curr = block_for_pc (ecs->event_thread->stop_pc ());
+ while (curr != nullptr && !curr->contains (prev))
+ {
+ if (curr->inlined_p ())
+ depth ++;
+ else if (curr->function () != nullptr)
+ break;
+ curr = curr->superblock ();
+ }
+ while (inline_skipped_frames (ecs->event_thread) > depth)
+ step_into_inline_frame (ecs->event_thread);
+ }
+
/* Look for "calls" to inlined functions, part one. If the inline
frame machinery detected some skipped call sites, we have entered
a new inline function. */
@@ -8172,6 +8200,8 @@ process_event_stop_test (struct execution_control_state *ecs)
infrun_debug_printf ("stepping through inlined function");
if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+ || ecs->event_thread->stop_pc () != stop_pc_sal.pc
+ || !stop_pc_sal.is_stmt
|| inline_frame_is_marked_for_skip (false, ecs->event_thread))
keep_going (ecs);
else
@@ -8220,7 +8250,8 @@ process_event_stop_test (struct execution_control_state *ecs)
end_stepping_range (ecs);
return;
}
- else if (*curr_frame_id == original_frame_id)
+ else if (get_stack_frame_id (frame)
+ == ecs->event_thread->control.step_stack_frame_id)
{
/* We are not at the start of a statement, and we have not changed
frame.
diff --git a/gdb/symtab.c b/gdb/symtab.c
index ce5e2520bd1..1af7f185148 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3290,7 +3290,10 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
0) instead of a real line. */
if (prev && prev->line
- && (!best || prev->unrelocated_pc () > best->unrelocated_pc ()))
+ && (!best || prev->unrelocated_pc () > best->unrelocated_pc ()
+ || (prev->unrelocated_pc () == best->unrelocated_pc ()
+ && (best->pc (objfile) == pc
+ ? !best->is_stmt : best->is_weak))))
{
best = prev;
best_symtab = iter_s;
@@ -3309,7 +3312,7 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
&& (tmp - 1)->unrelocated_pc () == tmp->unrelocated_pc ()
&& (tmp - 1)->line != 0 && !tmp->is_stmt)
--tmp;
- if (tmp->is_stmt)
+ if (tmp->is_stmt && (tmp->pc (objfile) == pc || !tmp->is_weak))
best = tmp;
}
@@ -3333,18 +3336,14 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
We used to return alt->line - 1 here, but that could be
anywhere; if we don't have line number info for this PC,
don't make some up. */
- val.pc = pc;
- }
- else if (best->line == 0)
- {
- /* If our best fit is in a range of PC's for which no line
- number info is available (line number is zero) then we didn't
- find any valid line information. */
+ if (notcurrent)
+ pc++;
val.pc = pc;
}
else
{
val.is_stmt = best->is_stmt;
+ val.is_weak = best->is_weak;
val.symtab = best_symtab;
val.line = best->line;
val.pc = best->pc (objfile);
diff --git a/gdb/testsuite/gdb.base/empty-inline.c b/gdb/testsuite/gdb.base/empty-inline.c
new file mode 100644
index 00000000000..4ecb3ff14a3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/empty-inline.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ 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/>. */
+
+static int test0 (void)
+{
+ asm (""); /* line 20 */
+ return 1; /* line 21 */
+}
+
+int __attribute__((noinline, noclone))
+#ifdef __CET__
+ __attribute__((nocf_check))
+#endif
+test1 (int x)
+{
+ asm ("");
+ return x+1; /* line 31 */
+}
+
+int
+main()
+{ test1 (test0 ()); /* line 36 */
+ test1 (test0 ()); /* line 37 */
+ return 0; /* line 38 */
+}
diff --git a/gdb/testsuite/gdb.base/empty-inline.exp b/gdb/testsuite/gdb.base/empty-inline.exp
new file mode 100644
index 00000000000..ae6300e7c47
--- /dev/null
+++ b/gdb/testsuite/gdb.base/empty-inline.exp
@@ -0,0 +1,51 @@
+# 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}
+lappend options additional_flags=-gstatement-frontiers
+
+if { [prepare_for_testing "failed to prepare" $binfile \
+ $srcfile $options] } {
+ return -1
+}
+
+if ![runto_main] {
+ return
+}
+
+gdb_test "frame 0" "\\s*\\#0\\s+main.*${srcfile}:36.*" "in main"
+gdb_test_multiple "step" "step into test0" {
+ -re ".*test0.*${srcfile}:20.*$::gdb_prompt $" {
+ gdb_test "step" ".*line 21.*" $gdb_test_name
+ }
+ -re ".*test0.*${srcfile}:21.*$::gdb_prompt $" {
+ pass $gdb_test_name
+ }
+}
+gdb_test "frame 1" "\\s*\\#1\\s+main.*${srcfile}:36.*" "frame1"
+gdb_test "step" ".*test1.*${srcfile}:31.*" "step into test1"
+gdb_test "frame 1" "\\s*\\#1.*in main.*${srcfile}:36.*" "frame2"
+gdb_test "step" ".*main.*${srcfile}:37.*" "step back to main"
+gdb_test "next" ".*return 0;.*" "step over test0+1"
+gdb_test "frame 0" "\\s*\\#0\\s+main.*${srcfile}:38.*" "in main again"
diff --git a/gdb/testsuite/gdb.cp/empty-inline.cc b/gdb/testsuite/gdb.cp/empty-inline.cc
new file mode 100644
index 00000000000..a960d5f7ec0
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/empty-inline.cc
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ 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/>. */
+
+/* PR 25987 */
+struct MyClass;
+struct ptr {
+ MyClass* get() { return t; } /* line 21 */
+ MyClass* t;
+};
+struct MyClass { void call(); };
+void MyClass::call() {
+ *(volatile char*)-1 = 1; /* line 26 */
+}
+static void intermediate(ptr p) {
+ p.get()->call(); /* line 29 */
+}
+int main() {
+ intermediate(ptr{new MyClass});
+}
diff --git a/gdb/testsuite/gdb.cp/empty-inline.exp b/gdb/testsuite/gdb.cp/empty-inline.exp
new file mode 100644
index 00000000000..5ae1e2d878c
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/empty-inline.exp
@@ -0,0 +1,51 @@
+# 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/>.
+
+# PR 25987
+standard_testfile .cc
+
+if { ![test_compiler_info gcc*] || ![supports_statement_frontiers] } {
+ untested "this test needs gcc with statement frontiers"
+ return -1
+}
+
+set options {c++ debug nowarnings optimize=-Og}
+lappend options additional_flags=-gstatement-frontiers
+if { [prepare_for_testing "failed to prepare" $testfile \
+ $srcfile $options] } {
+ return -1
+}
+
+if ![runto_main] {
+ return
+}
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in main"
+#break at the empty inline function ptr::get
+gdb_test "b get" ".*" "break at get"
+gdb_test "c" ".*" "continue to get"
+#call frame 1 is at line 29
+gdb_test "bt" [multi_line "\\s*\\#0\\s+ptr::get\[^\r\]*${srcfile}:21" \
+ "\\s*\\#1\\s+intermediate\[^\r\]*${srcfile}:29" \
+ ".*"] \
+ "at get"
+#print a local value here
+gdb_test "p t" ".*(\\\$1 = \\(MyClass \\*\\) 0x|value has been optimized out).*" "print t"
+gdb_test "c" ".*SIGSEGV.*" "continue to SIGSEGV"
+#call frame 1 is at line 29
+gdb_test "bt" [multi_line "\\s*\\#0\\s+\[^\r\]*MyClass::call\[^\r\]*${srcfile}:26" \
+ "\\s*\\#1\\s+0x\[^\r\]*intermediate\[^\r\]*${srcfile}:29" \
+ ".*"] \
+ "at call"
diff --git a/gdb/testsuite/gdb.cp/step-and-next-inline.cc b/gdb/testsuite/gdb.cp/step-and-next-inline.cc
index ac92206fd7f..7f8ead878ec 100644
--- a/gdb/testsuite/gdb.cp/step-and-next-inline.cc
+++ b/gdb/testsuite/gdb.cp/step-and-next-inline.cc
@@ -47,8 +47,7 @@ tree_check (tree *t, int i)
int __attribute__((noinline, noclone))
get_alias_set (tree *t)
-{
- if (t != NULL
+{ if (t != NULL
&& TREE_TYPE (t).z != 1
&& TREE_TYPE (t).z != 2
&& TREE_TYPE (t).z != 3)
@@ -60,7 +59,6 @@ tree xx;
int
main()
-{
- get_alias_set (&xx); /* Beginning of main */
+{ get_alias_set (&xx);
return 0;
} // main
diff --git a/gdb/testsuite/gdb.cp/step-and-next-inline.exp b/gdb/testsuite/gdb.cp/step-and-next-inline.exp
index 446cd825947..52d10b15e6a 100644
--- a/gdb/testsuite/gdb.cp/step-and-next-inline.exp
+++ b/gdb/testsuite/gdb.cp/step-and-next-inline.exp
@@ -15,7 +15,8 @@
standard_testfile .cc
-if {[test_compiler_info gcc*] && ![supports_statement_frontiers] } {
+if { ![test_compiler_info gcc*] || ![supports_statement_frontiers] } {
+ untested "this test needs gcc with statement frontiers"
return -1
}
@@ -24,17 +25,8 @@ if {[test_compiler_info gcc*] && ![supports_statement_frontiers] } {
proc do_test { use_header } {
global srcfile testfile
- if { $use_header } {
- # This test will not pass due to poor debug information
- # generated by GCC (at least upto 10.x). See
- # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94474
- return
- }
-
set options {c++ debug nowarnings optimize=-O2}
- if { [supports_statement_frontiers] } {
- lappend options additional_flags=-gstatement-frontiers
- }
+ lappend options additional_flags=-gstatement-frontiers
if { $use_header } {
lappend options additional_flags=-DUSE_NEXT_INLINE_H
set executable "$testfile-with-header"
@@ -53,131 +45,28 @@ proc do_test { use_header } {
with_test_prefix $prefix {
- set main_location [gdb_get_line_number "Beginning of main" $srcfile]
-
- if ![runto $main_location qualified] {
+ if ![runto_main] {
return
}
gdb_test "bt" "\\s*\\#0\\s+main.*" "in main"
- set line1 {\t\{}
- set line2 {\t if \(t != NULL}
- gdb_test_multiple "step" "step into get_alias_set" {
- -re -wrap $line1 {
- gdb_test "next" $line2 $gdb_test_name
- }
- -re -wrap $line2 {
- pass $gdb_test_name
- }
- }
+ gdb_test "step" ".*" "step into get_alias_set"
gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
"not in inline 1"
-
- # It's possible that this first failure (when not using a header
- # file) is GCC's fault, though the remaining failures would best
- # be fixed by adding location views support (though it could be
- # that some easier heuristic could be figured out). Still, it is
- # not certain that the first failure wouldn't also be fixed by
- # having location view support, so for now it is tagged as such.
- set have_kfail [expr [test_compiler_info gcc*] && !$use_header]
-
- set ok 1
- gdb_test_multiple "next" "next step 1" {
- -re -wrap "if \\(t->x != i\\)" {
- set ok 0
- send_gdb "next\n"
- exp_continue
- }
- -re -wrap ".*TREE_TYPE.* != 1" {
- if { $ok } {
- pass $gdb_test_name
- } else {
- if { $have_kfail } {
- setup_kfail "*-*-*" symtab/25507
- }
- fail $gdb_test_name
- }
- }
- }
+ gdb_test "next" ".*TREE_TYPE.*" "next step 1"
gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
"not in inline 2"
-
- set ok 1
- gdb_test_multiple "next" "next step 2" {
- -re -wrap "return x;" {
- set ok 0
- send_gdb "next\n"
- exp_continue
- }
- -re -wrap ".*TREE_TYPE.* != 2" {
- if { $ok } {
- pass $gdb_test_name
- } else {
- if { $have_kfail } {
- setup_kfail "*-*-*" symtab/25507
- }
- fail $gdb_test_name
- }
- }
- }
+ gdb_test "next" ".*TREE_TYPE.*" "next step 2"
gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
"not in inline 3"
-
- set ok 1
- gdb_test_multiple "next" "next step 3" {
- -re -wrap "return x;" {
- set ok 0
- send_gdb "next\n"
- exp_continue
- }
- -re -wrap ".*TREE_TYPE.* != 3\\)" {
- if { $ok } {
- pass $gdb_test_name
- } else {
- if { $have_kfail } {
- setup_kfail "*-*-*" symtab/25507
- }
- fail $gdb_test_name
- }
- }
- }
+ gdb_test "next" ".*TREE_TYPE.*" "next step 3"
gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
"not in inline 4"
-
- set ok 1
- gdb_test_multiple "next" "next step 4" {
- -re -wrap "(if \\(t != NULL|\} // get_alias_set)" {
- send_gdb "next\n"
- exp_continue
- }
- -re -wrap "return x;" {
- set ok 0
- send_gdb "next\n"
- exp_continue
- }
- -re -wrap "return 0.*" {
- if { $ok } {
- pass $gdb_test_name
- } else {
- if { $have_kfail } {
- setup_kfail "*-*-*" symtab/25507
- }
- fail $gdb_test_name
- }
- }
- }
+ gdb_test "next" "return 0.*" "next step 4"
gdb_test "bt" \
"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
"not in inline 5"
- if {!$use_header} {
- # With the debug from GCC 10.x (and earlier) GDB is currently
- # unable to successfully complete the following tests when we
- # are not using a header file.
- kfail symtab/25507 "stepping tests"
- return
- }
-
clean_restart ${executable}
if ![runto_main] {
@@ -194,22 +83,84 @@ proc do_test { use_header } {
gdb_test "step" ".*if \\(t->x != i\\).*" "step 2"
gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
"in inline 1 pass 2"
- gdb_test "step" ".*TREE_TYPE.*" "step 3"
+ gdb_test "step" ".*return x.*" "step 3"
+ gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+ "return from inline 1 pass 2"
+ gdb_test "step" ".*TREE_TYPE.*" "step 4"
gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
"not in inline 2 pass 2"
- gdb_test "step" ".*if \\(t->x != i\\).*" "step 4"
+ gdb_test "step" ".*if \\(t->x != i\\).*" "step 5"
gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
"in inline 2 pass 2"
- gdb_test "step" ".*TREE_TYPE.*" "step 5"
+ gdb_test "step" ".*return x.*" "step 6"
+ gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+ "return from inline 2 pass 2"
+ gdb_test "step" ".*TREE_TYPE.*" "step 7"
gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
"not in inline 3 pass 2"
- gdb_test "step" ".*if \\(t->x != i\\).*" "step 6"
+ gdb_test "step" ".*if \\(t->x != i\\).*" "step 8"
gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
"in inline 3 pass 2"
- gdb_test "step" "return 0.*" "step 7"
+ gdb_test "step" ".*return x.*" "step 9"
+ gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+ "return from inline 3 pass 2"
+ gdb_test "step" "return 0.*" "step 10"
gdb_test "bt" \
"\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
"not in inline 4 pass 2"
+
+ clean_restart ${executable}
+
+ if ![runto_main] {
+ return
+ }
+
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "in main pass 3"
+ gdb_test "step" ".*" "step into get_alias_set pass 3"
+ gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+ "in get_alias_set pass 3"
+ gdb_test "step" ".*TREE_TYPE.*" "step 1 pass 3"
+ gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+ "not in inline 1 pass 3"
+ gdb_test "step" ".*if \\(t->x != i\\).*" "step 2 pass 3"
+ gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+ "in inline 1 pass 3"
+ gdb_test_multiple "p t->x = 2" "change value pass 3" {
+ -re ".*value has been optimized out.*$::gdb_prompt $" {
+ gdb_test "p xx.x = 2" ".* = 2.*" $gdb_test_name
+ }
+ -re ".* = 2.*$::gdb_prompt $" {
+ pass $gdb_test_name
+ }
+ }
+ gdb_test "step" ".*abort.*" "step 3, pass 3"
+ gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*tree_check\[^\r\]*${hdrfile}:.*" \
+ "abort from inline 1 pass 3"
+
+ clean_restart ${executable}
+
+ if ![runto_main] {
+ return
+ }
+
+ gdb_test "bt" "\\s*\\#0\\s+main.*" "in main pass 4"
+ gdb_test "skip tree_check" ".*" "skip tree_check pass 4"
+ gdb_test "step" ".*" "step into get_alias_set pass 4"
+ gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+ "in get_alias_set pass 4"
+ gdb_test "step" ".*TREE_TYPE.*" "step 1 pass 4"
+ gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+ "not in inline 1 pass 4"
+ gdb_test "step" ".*TREE_TYPE.*" "step 2 pass 4"
+ gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+ "not in inline 2 pass 4"
+ gdb_test "step" ".*TREE_TYPE.*" "step 3 pass 4"
+ gdb_test "bt" "\\s*\\#0\\s+get_alias_set\[^\r\]*${srcfile}:.*" \
+ "not in inline 3 pass 4"
+ gdb_test "step" "return 0.*" "step 4 pass 4"
+ gdb_test "bt" \
+ "\\s*\\#0\\s+(main|get_alias_set)\[^\r\]*${srcfile}:.*" \
+ "not in inline 4 pass 4"
}
}
--
2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines
2024-09-02 14:57 ` [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines Bernd Edlinger
@ 2024-09-10 20:34 ` Guinevere Larsen
2024-10-16 15:49 ` Andrew Burgess
1 sibling, 0 replies; 9+ messages in thread
From: Guinevere Larsen @ 2024-09-10 20:34 UTC (permalink / raw)
To: Bernd Edlinger, gdb-patches; +Cc: Andrew Burgess
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 2/3] Introduce a new line table flag is_weak
2024-09-02 14:58 ` [PATCH v5 2/3] Introduce a new line table flag is_weak Bernd Edlinger
@ 2024-09-13 16:04 ` Guinevere Larsen
0 siblings, 0 replies; 9+ messages in thread
From: Guinevere Larsen @ 2024-09-13 16:04 UTC (permalink / raw)
To: Bernd Edlinger, gdb-patches; +Cc: Andrew Burgess
On 9/2/24 11:58 AM, Bernd Edlinger wrote:
> This introduces a new line table flag is_weak.
> The line entries at the end of a subroutine range,
> use this to indicate that they may possibly
> be part of the previous subroutine.
>
> When there is a sequence of line entries at the
> same address where an inline range ends, and the
> last item has is_stmt = 0, we force all previous
> items to have is_weak = 1.
>
> Additionally this adds a "fake" end sequence to the
> record_line function, that is line number -1.
> That will be used in the next patch.
>
> Finally this adds a handling for empty ranges to
> record_block_range. Currently this function is
> not called with empty ranges, but that will be used
> in the next patch.
>
> There should be no functional changes after this commit.
>
> Well, except the new is-weak flag in the line table of course.
> As an example consider the following test code:
>
> $ cat test.c
> inline int test ()
> {
> asm ("nop");
> return 0;
> }
>
> int main ()
> {
> int x = test ();
> return x;
> }
> $ gcc -g -O2 test.c
>
> This will receive the following line table:
>
> (gdb) maintenance info line-table
> INDEX LINE REL-ADDRESS UNREL-ADDRESS IS-STMT IS-WEAK PROLOGUE-END EPILOGUE-BEGIN
> 0 8 0x0000555555555040 0x0000000000001040 Y
> 1 9 0x0000555555555040 0x0000000000001040 Y
> 2 1 0x0000555555555040 0x0000000000001040 Y
> 3 3 0x0000555555555040 0x0000000000001040 Y
> 4 4 0x0000555555555041 0x0000000000001041 Y Y
> 5 4 0x0000555555555041 0x0000000000001041 Y <---+ set is_weak
> 6 10 0x0000555555555041 0x0000000000001041 Y Y ^
> 7 11 0x0000555555555041 0x0000000000001041 <----+ no is-stmt
> 8 END 0x0000555555555044 0x0000000000001044 Y
I've been trying to review this patch, but I've come to the conclusion
that the conceptual side is out of my depth. I will leave you and Andrew
to hash out the high level overview.
If you decide to go with this approach, I have a couple comments about
the specifics, but I wouldn't recommend that you act on these comments
until a decision is made if this is the direction to solve inlined
functions.
> ---
> gdb/buildsym.c | 106 ++++++++++++++++---
> gdb/buildsym.h | 3 +
> gdb/jit.c | 1 +
> gdb/symmisc.c | 6 +-
> gdb/symtab.h | 5 +
> gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp | 2 +-
> gdb/xcoffread.c | 1 +
> 7 files changed, 109 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index 02d6848f3a4..2e552fa9046 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -413,6 +413,16 @@ buildsym_compunit::record_block_range (struct block *block,
> || end_inclusive + 1 != block->end ())
> m_pending_addrmap_interesting = true;
>
> + if (block->inlined_p ())
> + {
> + m_inline_end_vector.push_back (end_inclusive + 1);
> + if (end_inclusive + 1 == start)
> + {
> + end_inclusive = start;
> + m_pending_addrmap_interesting = true;
> + }
> + }
> +
> m_pending_addrmap.set_empty (start, end_inclusive, block);
> }
>
> @@ -627,19 +637,16 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
> {
> m_have_line_numbers = true;
>
> - /* Normally, we treat lines as unsorted. But the end of sequence
> - marker is special. We sort line markers at the same PC by line
> - number, so end of sequence markers (which have line == 0) appear
> - first. This is right if the marker ends the previous function,
> - and there is no padding before the next function. But it is
> - wrong if the previous line was empty and we are now marking a
> - switch to a different subfile. We must leave the end of sequence
> - marker at the end of this group of lines, not sort the empty line
> - to after the marker. The easiest way to accomplish this is to
> - delete any empty lines from our table, if they are followed by
> - end of sequence markers. All we lose is the ability to set
> - breakpoints at some lines which contain no instructions
> - anyway. */
> + /* The end of sequence marker is special. We need to delete any
> + previous lines at the same PC, otherwise these lines may cause
> + problems since they might be at the same address as the following
> + function. For instance suppose a function calls abort there is no
> + reason to emit a ret after that point (no joke).
> + So the label may be at the same address where the following
> + function begins. There is also a fake end of sequence marker (-1)
> + that we emit internally when switching between different CUs
> + In this case, duplicate line table entries shall not be deleted.
> + We simply set the is_weak marker in this case. */
> if (line == 0)
> {
> std::optional<int> last_line;
> @@ -659,15 +666,84 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
> if (!last_line.has_value () || *last_line == 0)
> return;
> }
> + else if (line == -1)
> + {
> + line = 0;
> + auto e = subfile->line_vector_entries.end ();
> + while (e > subfile->line_vector_entries.begin ())
> + {
> + e--;
> + if (e->unrelocated_pc () != pc)
> + break;
> + e->is_weak = 1;
> + }
> + }
This part of the change (and the comment above) should definitely go on
patch 3.
If record_line is only called with line == -1 in patch 3, not having
this hunk in that patch makes it very confusing to review those changes,
and could make anyone in the future very confused when trying to
understand the commit, especially if finding these email threads turns
out to be difficult.
>
> linetable_entry &e = subfile->line_vector_entries.emplace_back ();
> e.line = line;
> e.is_stmt = (flags & LEF_IS_STMT) != 0;
> + e.is_weak = false;
> e.set_unrelocated_pc (pc);
> e.prologue_end = (flags & LEF_PROLOGUE_END) != 0;
> e.epilogue_begin = (flags & LEF_EPILOGUE_BEGIN) != 0;
> }
>
> +\f
> +/* Patch the is_stmt bits at the given inline end address.
> + The line table has to be already sorted. */
Firstly, this comment is incorrect, you're patching is_weak, not
is_stmt, but more importantly below:
> +
> +static void
> +patch_inline_end_pos (struct subfile *subfile, struct objfile *objfile,
> + CORE_ADDR end)
I think this comment+function name only make sense because we're deep in
this conversation. Someone who hasn't had this context hitting this
function will be very confused.
There is nowhere in the code that concisely explains is_weak. Since this
seems like the main place is_weak is set, the function comment could
explain it some more. I would suggest the following:
/* Patch all the line table entries at the final PC of an inline
function, marking them as "weak".
weak statements are used to identify line table entries that likely
belong to an inner block,
while not weak statements likely below to the outermost block
containing that PC.
The line table has to be sorted already. */
static void
patch_weak_linetable_entries (...)
This way, in 5-10+ years it will still be easy to understand what this
code is aiming to do, even if we decide to change version control
systems or lose access to mailing list archives (yes, I've been having
trouble with lost history, how can you tell?)
> +{
> + std::vector<linetable_entry> &items = subfile->line_vector_entries;
> + int a = 2, b = items.size () - 1;
> +
> + /* We need at least two items with pc = end in the table.
> + The lowest usable items are at pos 0 and 1, the highest
> + usable items are at pos b - 2 and b - 1. */
> + if (a > b
> + || end < items[1].pc (objfile)
> + || end > items[b - 2].pc (objfile))
> + return;
> +
> + /* Look for the first item with pc > end in the range [a,b].
> + The previous element has pc = end or there is no match.
> + We set a = 2, since we need at least two consecutive elements
> + with pc = end to do anything useful.
> + We set b = items.size () - 1, since we are not interested
> + in the last element which should be an end of sequence
> + marker with line = 0 and is_stmt = true. */
> + while (a < b)
> + {
> + int c = (a + b) / 2;
> +
> + if (end < items[c].pc (objfile))
> + b = c;
> + else
> + a = c + 1;
> + }
> +
> + a--;
> + if (items[a].pc (objfile) != end || items[a].is_stmt)
> + return;
> +
> + /* When there is a sequence of line entries at the same address
> + where an inline range ends, and the last item has is_stmt = 0,
> + we force all previous items to have is_weak = true as well. */
> + do
> + {
> + /* We stop at the first line entry with a different address,
> + or when we see an end of sequence marker. */
> + a--;
> + if (items[a].pc (objfile) != end || items[a].line == 0)
> + break;
> +
> + items[a].is_weak = true;
> + }
> + while (a > 0);
> +}
> +
> \f
> /* Subroutine of end_compunit_symtab to simplify it. Look for a subfile that
> matches the main source file's basename. If there is only one, and
> @@ -892,6 +968,10 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
> relationships, this is why std::stable_sort is used. */
> std::stable_sort (subfile->line_vector_entries.begin (),
> subfile->line_vector_entries.end ());
> +
> + for (int i = 0; i < m_inline_end_vector.size (); i++)
> + patch_inline_end_pos (subfile, m_objfile,
> + m_inline_end_vector[i]);
> }
>
> /* Allocate a symbol table if necessary. */
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index c1eed247d25..edf76c8b17c 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -446,6 +446,9 @@ struct buildsym_compunit
>
> /* Pending symbols that are local to the lexical context. */
> struct pending *m_local_symbols = nullptr;
> +
> + /* Pending inline end range addresses. */
> + std::vector<CORE_ADDR> m_inline_end_vector;
> };
>
> \f
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 89c967db615..3e9353e206b 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -495,6 +495,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
> (unrelocated_addr (map[i].pc));
> stab->linetable->item[i].line = map[i].line;
> stab->linetable->item[i].is_stmt = true;
> + stab->linetable->item[i].is_weak = false;
> }
> }
>
> diff --git a/gdb/symmisc.c b/gdb/symmisc.c
> index 7f8141588b7..eb1487b39b0 100644
> --- a/gdb/symmisc.c
> +++ b/gdb/symmisc.c
> @@ -265,6 +265,8 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile)
> gdb_puts (paddress (gdbarch, l->item[i].pc (objfile)), outfile);
> if (l->item[i].is_stmt)
> gdb_printf (outfile, "\t(stmt)");
> + if (l->item[i].is_weak)
> + gdb_printf (outfile, "\t(weak)");
> gdb_printf (outfile, "\n");
> }
> }
> @@ -972,12 +974,13 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
> /* Leave space for 6 digits of index and line number. After that the
> tables will just not format as well. */
> struct ui_out *uiout = current_uiout;
> - ui_out_emit_table table_emitter (uiout, 7, -1, "line-table");
> + ui_out_emit_table table_emitter (uiout, 8, -1, "line-table");
> uiout->table_header (6, ui_left, "index", _("INDEX"));
> uiout->table_header (6, ui_left, "line", _("LINE"));
> uiout->table_header (18, ui_left, "rel-address", _("REL-ADDRESS"));
> uiout->table_header (18, ui_left, "unrel-address", _("UNREL-ADDRESS"));
> uiout->table_header (7, ui_left, "is-stmt", _("IS-STMT"));
> + uiout->table_header (7, ui_left, "is-weak", _("IS-WEAK"));
> uiout->table_header (12, ui_left, "prologue-end", _("PROLOGUE-END"));
> uiout->table_header (14, ui_left, "epilogue-begin", _("EPILOGUE-BEGIN"));
> uiout->table_body ();
> @@ -998,6 +1001,7 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
> uiout->field_core_addr ("unrel-address", objfile->arch (),
> CORE_ADDR (item->unrelocated_pc ()));
> uiout->field_string ("is-stmt", item->is_stmt ? "Y" : "");
> + uiout->field_string ("is-weak", item->is_weak ? "Y" : "");
> uiout->field_string ("prologue-end", item->prologue_end ? "Y" : "");
> uiout->field_string ("epilogue-begin", item->epilogue_begin ? "Y" : "");
> uiout->text ("\n");
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 4197a3a164c..188d7e68923 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1656,6 +1656,9 @@ struct linetable_entry
> /* True if this PC is a good location to place a breakpoint for LINE. */
> bool is_stmt : 1;
>
> + /* True if this PC is at a subroutine range end. */
> + bool is_weak : 1;
> +
> /* True if this location is a good location to place a breakpoint after a
> function prologue. */
> bool prologue_end : 1;
> @@ -2412,6 +2415,8 @@ struct symtab_and_line
> /* If the line number information is valid, then this indicates if this
> line table entry had the is-stmt flag set or not. */
> bool is_stmt = false;
> + /* True if this PC is at a subroutine range end. */
> + bool is_weak = false;
>
> /* The probe associated with this symtab_and_line. */
> probe *prob = NULL;
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp b/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
> index 1a3d53c2116..403bc6b3f72 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp
> @@ -168,7 +168,7 @@ gdb_test_multiple "maint info line-table gdb.dwarf2/dw2-ranges-base.c" \
> -re ".*linetable: \\(\\(struct linetable \\*\\) 0x0\\):\r\nNo line table.\r\n" {
> exp_continue
> }
> - -re ".*linetable: \\(\\(struct linetable \\*\\) $hex\\):\r\nINDEX\[ \t\]+LINE\[ \t\]+REL-ADDRESS\[ \t\]+UNREL-ADDRESS\[ \t\]+IS-STMT\[ \t\]PROLOGUE-END\[ \t\]EPILOGUE-BEGIN *\r\n" {
> + -re ".*linetable: \\(\\(struct linetable \\*\\) $hex\\):\r\nINDEX\[ \t\]+LINE\[ \t\]+REL-ADDRESS\[ \t\]+UNREL-ADDRESS\[ \t\]+IS-STMT\[ \t\]IS-WEAK\[ \t\]PROLOGUE-END\[ \t\]EPILOGUE-BEGIN *\r\n" {
> exp_continue
> }
> }
> diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
> index 639dd5b8adc..1642bc12dfb 100644
> --- a/gdb/xcoffread.c
> +++ b/gdb/xcoffread.c
> @@ -431,6 +431,7 @@ arrange_linetable (std::vector<linetable_entry> &old_linetable)
> linetable_entry &e = fentries.emplace_back ();
> e.line = ii;
> e.is_stmt = true;
> + e.is_weak = false;
> e.set_unrelocated_pc (old_linetable[ii].unrelocated_pc ());
> }
> }
--
Cheers,
Guinevere Larsen
She/Her/Hers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 0/3] Improve debugging of optimized code
2024-09-02 14:55 [PATCH v5 0/3] Improve debugging of optimized code Bernd Edlinger
` (2 preceding siblings ...)
2024-09-02 15:00 ` [PATCH v5 3/3] Fix range end handling of inlined subroutines Bernd Edlinger
@ 2024-09-26 17:40 ` Andrew Burgess
3 siblings, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2024-09-26 17:40 UTC (permalink / raw)
To: Bernd Edlinger, gdb-patches; +Cc: Guinevere Larsen
Hi Bernd,
Just wanted to let you know that I am currently looking through these
patches, though I'm not going to finish this week. I'll try to make
time to write up my thoughts next week.
But I do want to say thank you for continuing to work on these patches,
I'm looking forward to getting some improvements in this area.
Thanks,
Andrew
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> These patches improve the debug experience with optimized
> gcc code significantly, as can be seen by the test cases.
> The problem is that there are cases where an inline function
> has line table entries at the end of the inline subrange,
> and it is unclear by the dwarf-5 debug info if these line
> table entries belong to the called inline function or to
> the calling outer function, while there are at least some
> corner cases where by stepping into these lines e.g. the
> return value might be inspected. Also the call stack is
> often displayed wrong because the line table entries
> do not match with the block structure i.e. are obviously
> not in the correct function.
>
> Therefore this patch series tries to improve the issue by
> introducing a heuristic, that marks some of the line table
> entries that happen to be at the exact end of an inline
> function range as weak. This new line table flag can be
> used to avoid to step into the inline function when a
> step over was requested, and display the line belonging
> to the correct function in the callstack.
>
> I'm sorry, that this solution has quite some complexity,
> but I promise it is worth it. Please give this patch
> a try and look for yourself how the debug experience is
> improved in the test cases, and certainly also a number of
> other programs, especially with C++, where inline functions
> are pretty much used everywhere.
>
> v3: minor update to fix regressions in gdb.cp/empty-inline.exp
>
> This update fixes only one test case, that turned out, not to
> work properly with arm and aarch64 targets (many thanks to Linaro
> for reporting this to me). The reason was that the call stack
> looks slightly different when the trap is on a statement boundary
> or not. So the test expectation was changed to ignore that bit.
> Additionally the test case was failing on riscv simulator targets
> because assignment to NULL is not enough to cause an exception
> on a simulator. However assigning to (char*)-1 triggers always
> an exception.
>
> v4: Added a simple test case for part 1,
> improved the commit message of part 2,
> improved test cases for part 3, avoiding issues with clang.
>
> I finally was able to find a realistic but still quite simple example
> where the inline entry_pc is not at the first subrange, and where
> this actually makes a user-visible difference. Fortunately it is
> reproducible with almost all gcc versions I tried so far, therefore
> I did not have to create an artificial dwarf test for this issue.
> Other than that I also added a check that the entry_pc is not in the
> form of a constant, it will usually be an address.
>
> As requested in the review I've added an example how the new line
> table will look like in the commit message of part 2,
> and revised the test cases of part 3, to avoid possible issues
> with clang.
>
> There are some issues with clang but they are different than gcc's,
> furthermore the clang debug info is not affected by these patches,
> therefore the test cases are better skipped with clang.
>
> v5: Fixed an issue with subroutines containing inner lexical blocks.
>
>
> Bernd Edlinger (3):
> Fix handling of DW_AT_entry_pc of inlined subroutines
> Introduce a new line table flag is_weak
> Fix range end handling of inlined subroutines
>
> gdb/block.c | 15 +-
> gdb/buildsym.c | 106 +++++++--
> gdb/buildsym.h | 3 +
> gdb/dwarf2/read.c | 88 +++-----
> gdb/infcmd.c | 3 +-
> gdb/infrun.c | 33 ++-
> gdb/jit.c | 1 +
> gdb/symmisc.c | 6 +-
> gdb/symtab.c | 17 +-
> gdb/symtab.h | 5 +
> gdb/testsuite/gdb.base/empty-inline.c | 39 ++++
> gdb/testsuite/gdb.base/empty-inline.exp | 51 +++++
> gdb/testsuite/gdb.base/inline-entry.c | 39 ++++
> gdb/testsuite/gdb.base/inline-entry.exp | 43 ++++
> gdb/testsuite/gdb.cp/empty-inline.cc | 33 +++
> gdb/testsuite/gdb.cp/empty-inline.exp | 51 +++++
> gdb/testsuite/gdb.cp/step-and-next-inline.cc | 6 +-
> gdb/testsuite/gdb.cp/step-and-next-inline.exp | 201 +++++++-----------
> gdb/testsuite/gdb.dwarf2/dw2-ranges-base.exp | 2 +-
> gdb/xcoffread.c | 1 +
> 20 files changed, 524 insertions(+), 219 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/empty-inline.c
> create mode 100644 gdb/testsuite/gdb.base/empty-inline.exp
> create mode 100644 gdb/testsuite/gdb.base/inline-entry.c
> create mode 100644 gdb/testsuite/gdb.base/inline-entry.exp
> create mode 100644 gdb/testsuite/gdb.cp/empty-inline.cc
> create mode 100644 gdb/testsuite/gdb.cp/empty-inline.exp
>
> --
> 2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines
2024-09-02 14:57 ` [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines Bernd Edlinger
2024-09-10 20:34 ` Guinevere Larsen
@ 2024-10-16 15:49 ` Andrew Burgess
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2024-10-16 15:49 UTC (permalink / raw)
To: Bernd Edlinger, gdb-patches; +Cc: Guinevere Larsen
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 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.
I've posted a possible alternative approach here:
https://inbox.sourceware.org/gdb-patches/1484e0b351a60ab171896de1ca52d8da89b337d0.1729093223.git.aburgess@redhat.com/
I'm continuing to look at the other two patches in this series.
Thanks,
Andrew
> ---
> 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"
> +gdb_test "continue" ".*Breakpoint .* bar .*" "continue to bar"
> +gdb_test "continue" ".*Breakpoint .* foo .*" "continue to foo"
> +gdb_test "continue" ".* exited .*" "continue to exit"
> --
> 2.39.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 3/3] Fix range end handling of inlined subroutines
2024-09-02 15:00 ` [PATCH v5 3/3] Fix range end handling of inlined subroutines Bernd Edlinger
@ 2024-10-18 10:47 ` Andrew Burgess
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2024-10-18 10:47 UTC (permalink / raw)
To: Bernd Edlinger, gdb-patches; +Cc: Guinevere Larsen
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> Additionally it may happen that the infrun machinery
> steps from one inline range to another inline range
> of the same inline function. That can look like
> jumping back and forth from the calling program
> to the inline function, while really the inline
> function just jumps from a hot to a cold section
> of the code, i.e. error handling.
I've taken this part of the patch and written some tests using the DWARF
assembler and posted it here:
https://inbox.sourceware.org/gdb-patches/0e7a6b004bf671a4ccf4d63935a37aa0ee3769dd.1729248287.git.aburgess@redhat.com/
As I've not changed the GDB code at all I've left you (Bernd) as the
patch author.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-18 10:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-02 14:55 [PATCH v5 0/3] Improve debugging of optimized code Bernd Edlinger
2024-09-02 14:57 ` [PATCH v5 1/3] Fix handling of DW_AT_entry_pc of inlined subroutines Bernd Edlinger
2024-09-10 20:34 ` Guinevere Larsen
2024-10-16 15:49 ` Andrew Burgess
2024-09-02 14:58 ` [PATCH v5 2/3] Introduce a new line table flag is_weak Bernd Edlinger
2024-09-13 16:04 ` Guinevere Larsen
2024-09-02 15:00 ` [PATCH v5 3/3] Fix range end handling of inlined subroutines Bernd Edlinger
2024-10-18 10:47 ` Andrew Burgess
2024-09-26 17:40 ` [PATCH v5 0/3] Improve debugging of optimized code Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).