From 98a617f8d5cf9bfb53b2e1d5cf3f0e0f188d9e53 Mon Sep 17 00:00:00 2001 From: Kevin Buettner Date: Sat, 13 Jul 2019 15:59:44 -0700 Subject: [PATCH] Fix stepping bug associated with non-contiguous blocks I recently noticed the following behavior while debugging dw2-ranges-func-low-cold. This is one of the test programs associated with the test gdb.dwarf2/dw2-ranges-func.exp. (gdb) b 70 Breakpoint 1 at 0x401129: file dw2-ranges-func-lo-cold.c, line 70. (gdb) run Starting program: dw2-ranges-func-lo-cold Breakpoint 1, foo () at dw2-ranges-func-lo-cold.c:70 70 if (e) foo_cold (); /* foo foo_cold call */ (gdb) set var e=1 (gdb) step [Inferior 1 (process 12545) exited normally] This is incorrect. When stepping, we expect a step to occur. We do not expect the program to exit. Instead, we should see the following behavior: ... (gdb) set var e=1 (gdb) step foo () at dw2-ranges-func-lo-cold.c:54 54 baz (); /* foo_cold baz call */ (Note that I've shortened the paths in the above sessions to improve readability.) The bug is in fill_in_stop_func() in infrun.c. While working on non-contiguous address range improvements in 2018, I replaced the call to find_pc_partial_function() with a call to find_function_entry_range_from_pc(). Although this seemed like the right thing to do at the time, I now think that calling find_pc_partial_function (along with some other tweaks) is the right thing to do. For blocks with a single contiguous range, these functions do pretty much the same thing: when the function succeeds, the function name, start address, and end address are all filled in. Additionally, find_pc_partial_function contains an additional output parameter which is set to the block containing that PC. For blocks with non-contiguous ranges, find_pc_partial_function sets the start and end addresses to the start and end addresses of the range containing the pc. find_function_entry_range_from_pc does what it says; it sets the start and end addresses to those of the range containing the entry pc. The reason that I had thought that using the entry pc range was correct is due to the fact that fill_in_stop_func() contains some code for advancing past the function start and entry point. To do this, we'd need the range that contains the entry pc. However, when stepping, we actually want the range that contains the stop pc. If that range also contains the entry pc, we should then attempt to advance stop_func_start past the start offset and entry point. (I haven't thought very hard about the reason for advancing the stop_func_start in this manner. Since it's been there for quite a while, I'm assuming that it's still a good idea.) Back when I wrote the test case, I had included a test for doing the step shown in the example above. I had problems with it, however. At the time, I thought it was due to differing compiler versions, so I disabled that portion of the test. I have now reenabled those tests, but have left in place the logic which may be used to disable it. The changes to dw2-ranges-func.exp depend on my other recent changes to the file which have not been pushed yet. Finally, I'll note that the only caller of find_function_entry_range_from_pc() is/was fill_in_stop_func(). Once this commit goes in, it'll be dead code. I considered removing it, but I think that it ought to be used (instead of find_pc_partial_function) for determining the correct range to scan for prologue analysis, so I'm going to leave it in place for now. gdb/ChangeLog: * infrun.c (fill_in_stop_func): Use find_pc_partial_function instead of find_function_entry_range_from_pc. testsuite/ChangeLog: * gdb.dwarf2/dw2-ranges-func.exp (enable_foo_cold_stepping): Enable tests associated with this flag. Adjust regex referencing "foo_low" to now refer to "foo_cold" instead. --- gdb/ChangeLog | 5 +++ gdb/infrun.c | 37 ++++++++++++++------ gdb/testsuite/ChangeLog | 6 ++++ gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp | 37 +++++++++++++------- 4 files changed, 62 insertions(+), 23 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index a3c6bcfeaf4..bca9b72e0fb 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2019-07-27 Kevin Buettner + + * infrun.c (fill_in_stop_func): Use find_pc_partial_function + instead of find_function_entry_range_from_pc. + 2019-07-27 Kevin Buettner * stack.c (find_frame_funname): Remove code which preferred diff --git a/gdb/infrun.c b/gdb/infrun.c index 2dc41892365..a9588f896a5 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4115,18 +4115,35 @@ fill_in_stop_func (struct gdbarch *gdbarch, { if (!ecs->stop_func_filled_in) { + const block *block; + /* Don't care about return value; stop_func_start and stop_func_name will both be 0 if it doesn't work. */ - find_function_entry_range_from_pc (ecs->event_thread->suspend.stop_pc, - &ecs->stop_func_name, - &ecs->stop_func_start, - &ecs->stop_func_end); - ecs->stop_func_start - += gdbarch_deprecated_function_start_offset (gdbarch); - - if (gdbarch_skip_entrypoint_p (gdbarch)) - ecs->stop_func_start = gdbarch_skip_entrypoint (gdbarch, - ecs->stop_func_start); + find_pc_partial_function (ecs->event_thread->suspend.stop_pc, + &ecs->stop_func_name, + &ecs->stop_func_start, + &ecs->stop_func_end, + &block); + + /* The call to find_pc_partial_function, above, will set + stop_func_start and stop_func_end to the start and end + of the range containing the stop pc. If this range + contains the entry pc for the block (which is always the + case for contiguous blocks), advance stop_func_start past + the function's start offset and entrypoint. Note that + stop_func_start is NOT advanced when in a range of a + non-contiguous block that does not contain the entry pc. */ + if (block != nullptr + && ecs->stop_func_start <= BLOCK_ENTRY_PC (block) + && BLOCK_ENTRY_PC (block) < ecs->stop_func_end) + { + ecs->stop_func_start + += gdbarch_deprecated_function_start_offset (gdbarch); + + if (gdbarch_skip_entrypoint_p (gdbarch)) + ecs->stop_func_start + = gdbarch_skip_entrypoint (gdbarch, ecs->stop_func_start); + } ecs->stop_func_filled_in = 1; } diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index bd887c3191a..7cd3f9c7974 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2019-07-27 Kevin Buettner + + * gdb.dwarf2/dw2-ranges-func.exp (enable_foo_cold_stepping): + Enable tests associated with this flag. Adjust regex + referencing "foo_low" to now refer to "foo_cold" instead. + 2019-07-27 Kevin Buettner * gdb.dwarf2/dw2-ranges-func.c: Rename to... diff --git a/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp b/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp index fdc488ae926..bd0564f1886 100644 --- a/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp +++ b/gdb/testsuite/gdb.dwarf2/dw2-ranges-func.exp @@ -395,20 +395,31 @@ proc do_test {suffix} { "foo(_label2)? \\(\\).*foo_cold \\(\\);.*foo foo_cold call.*" \ "step out of bar to foo" - # The tests in the "enable_foo_cold_stepping" section, below, work - # with some versions of gcc, though it's not clear that they - # should. This test case causes foo_cold, originally a separate - # function invoked via a subroutine call, to be considered as part - # of foo via use of DW_AT_ranges. Real code that I've looked at - # uses a branch instruction to cause code in the "cold" range to - # be executed. + # Tests in the "enable_foo_cold_stepping" section, below, did + # not work prior to July, 2019. They had been disabled via + # use of the "enable_foo_cold_stepping" flag. + # + # As noted elsewhere, this test case causes foo_cold, + # originally a separate function invoked via a subroutine + # call, to be considered as part of foo via use of + # DW_AT_ranges. Real code that I've looked at uses a branch + # instruction to cause code in the "cold" range to be + # executed. These tests used to fail which is why they were + # disabled. # - # For the moment though, these tests have been left in place, but - # disabled, in case we decide that making such a subroutine call - # is a reasonable thing to do that should also be supported by - # GDB. + # After adding a "hi" cold test, I found that we were able to + # step into foo_cold from foo for the "hi" version, but for + # the "lo" version, GDB would run to either the next + # breakpoint or until the inferior exited when there were no + # breakpoints. Not being able to step is definitely a bug + # even if it's unlikely that this problem would ever be hit in + # a real program. Therefore, the bug was fixed in GDB and + # these tests are now enabled. + # + # I've left in place the flag (and test) which may be used to + # disable these tests. - set enable_foo_cold_stepping false + set enable_foo_cold_stepping true if { $enable_foo_cold_stepping } { gdb_test_no_output "set variable e=1" @@ -422,7 +433,7 @@ proc do_test {suffix} { "step to baz call in foo_cold" } - -re "foo(_low)? \\(\\).*baz \\(\\);.*foo_cold baz call.*${gdb_prompt}" { + -re "foo(_cold)? \\(\\).*baz \\(\\);.*foo_cold baz call.*${gdb_prompt}" { pass $test } } -- 2.30.2