* [PATCH v2] gdb: fix "frame function" issue when call is last instruction @ 2024-07-01 19:53 Sahil Siddiq 2024-07-03 14:24 ` Guinevere Larsen 0 siblings, 1 reply; 3+ messages in thread From: Sahil Siddiq @ 2024-07-01 19:53 UTC (permalink / raw) To: gdb-patches; +Cc: Sahil Siddiq Currently, the "frame function" fails when the last instruction is a call. In such a case, the $rip register points to an address that lies outside the frame. Using "get_frame_address_in_block" instead of "get_frame_pc" resolves this issue. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30929 --- Changes since v1: * frame-selection-last-instr-call.exp: Modify test to fix regressions on arm/aarch64 Patch v1: https://sourceware.org/pipermail/gdb-patches/2024-June/210251.html gdb/stack.c | 4 +- .../frame-selection-last-instr-call.c | 28 ++++ .../frame-selection-last-instr-call.exp | 130 ++++++++++++++++++ 3 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.base/frame-selection-last-instr-call.c create mode 100644 gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp diff --git a/gdb/stack.c b/gdb/stack.c index b36193be2f..8249866468 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -2860,8 +2860,8 @@ find_frame_for_function (const char *function_name) do { for (size_t i = 0; (i < sals.size () && !found); i++) - found = (get_frame_pc (frame) >= func_bounds[i].low - && get_frame_pc (frame) < func_bounds[i].high); + found = (get_frame_address_in_block (frame) >= func_bounds[i].low + && get_frame_address_in_block (frame) < func_bounds[i].high); if (!found) { level = 1; diff --git a/gdb/testsuite/gdb.base/frame-selection-last-instr-call.c b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.c new file mode 100644 index 0000000000..4f056332af --- /dev/null +++ b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.c @@ -0,0 +1,28 @@ +/* Copyright 2024 Free Software Foundation, Inc. + + This file is part of GDB. + + 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/>. */ + +void +frame_1 (void) +{ + __builtin_abort(); +} + +int +main (void) +{ + frame_1 (); +} diff --git a/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp new file mode 100644 index 0000000000..5ba52cc069 --- /dev/null +++ b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp @@ -0,0 +1,130 @@ +# 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/>. + +# This tests GDB's frame selection as used by the 'frame', +# 'select-frame', and 'info frame' commands in the corner case +# when the last instruction in a frame is a call. + +standard_testfile + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} { + return -1 +} + +runto_main +gdb_breakpoint abort +gdb_continue_to_breakpoint abort + +gdb_test "bt" "#0 $hex in abort .*#1 $hex in frame_1 .*#2 $hex in main.*" "backtrace at breakpoint" + +# Perform "info frame" to extract the frame's address. +proc get_frame_address { {testname ""} } { + global hex gdb_prompt + + set frame_address "unknown" + set testname "get_frame_address: ${testname}" + gdb_test_multiple "info frame" $testname { + -re ", frame at ($hex):\r\n.*\r\n$gdb_prompt $" { + set frame_address $expect_out(1,string) + pass $testname + } + } + + return $frame_address +} + +# Check that the current frame is at stack depth LEVEL, at frame +# address ADDRESS, and is in FUNCTION. +proc check_frame { level address function } { + global hex gdb_prompt + + set re [multi_line \ + "Stack level ${level}, frame at ($address):" \ + ".* = $hex in ${function}( \(\[^\r\n\]*\))*; saved .* = $hex" \ + ".*\r\n$gdb_prompt $" ] + + set testname "check frame level ${level}" + gdb_test_multiple "info frame" $testname { + -re $re { + pass $testname + } + } +} + +# Select frame using level, but relying on this being the default +# action, so "frame 0" performs "frame level 0". +gdb_test "frame 0" "#0 $hex in abort.*" +set frame_0_address [ get_frame_address "frame 0" ] +gdb_test "frame 1" "#1 $hex in frame_1.*" +set frame_1_address [ get_frame_address "frame 1" ] +gdb_test "frame 2" "#2 $hex in main.*" +set frame_2_address [ get_frame_address "frame 2" ] + +# Select frame using 'level' specification. +gdb_test "frame level 0" "#0 $hex in abort.*" +gdb_test "frame level 1" "#1 $hex in frame_1.*" +gdb_test "frame level 2" "#2 $hex in main.*" + +# Select frame by address. +gdb_test "frame address ${frame_0_address}" "#0 $hex in abort.*" \ + "select frame 0 by address" +gdb_test "frame address ${frame_1_address}" "#1 $hex in frame_1.*" \ + "select frame 1 by address" +gdb_test "frame address ${frame_2_address}" "#2 $hex in main.*" \ + "select frame 2 by address" + +# Select frame by function. +gdb_test "frame function abort" "#0 $hex in abort.*" +gdb_test "frame function frame_1" "#1 $hex in frame_1.*" +gdb_test "frame function main" "#2 $hex in main.*" + +with_test_prefix "select-frame, no keyword" { + gdb_test_no_output "select-frame 0" + check_frame "0" "${frame_0_address}" "abort" + gdb_test_no_output "select-frame 1" + check_frame "1" "${frame_1_address}" "frame_1" + gdb_test_no_output "select-frame 2" + check_frame "2" "${frame_2_address}" "main" +} + +with_test_prefix "select-frame, keyword=level" { + gdb_test_no_output "select-frame level 0" + check_frame "0" "${frame_0_address}" "abort" + gdb_test_no_output "select-frame level 1" + check_frame "1" "${frame_1_address}" "frame_1" + gdb_test_no_output "select-frame level 2" + check_frame "2" "${frame_2_address}" "main" +} + +with_test_prefix "select-frame, keyword=address" { + gdb_test_no_output "select-frame address ${frame_0_address}" \ + "select frame 0 by address" + check_frame "0" "${frame_0_address}" "abort" + gdb_test_no_output "select-frame address ${frame_1_address}" \ + "select frame 1 by address" + check_frame "1" "${frame_1_address}" "frame_1" + gdb_test_no_output "select-frame address ${frame_2_address}" \ + "select frame 2 by address" + check_frame "2" "${frame_2_address}" "main" +} + +with_test_prefix "select-frame, keyword=function" { + gdb_test_no_output "select-frame function abort" + check_frame "0" "${frame_0_address}" "abort" + gdb_test_no_output "select-frame function frame_1" + check_frame "1" "${frame_1_address}" "frame_1" + gdb_test_no_output "select-frame function main" + check_frame "2" "${frame_2_address}" "main" +} -- 2.45.2 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] gdb: fix "frame function" issue when call is last instruction 2024-07-01 19:53 [PATCH v2] gdb: fix "frame function" issue when call is last instruction Sahil Siddiq @ 2024-07-03 14:24 ` Guinevere Larsen 2024-07-04 11:57 ` Sahil 0 siblings, 1 reply; 3+ messages in thread From: Guinevere Larsen @ 2024-07-03 14:24 UTC (permalink / raw) To: Sahil Siddiq, gdb-patches; +Cc: Sahil Siddiq On 7/1/24 4:53 PM, Sahil Siddiq wrote: > Currently, the "frame function" fails when the last > instruction is a call. In such a case, the $rip > register points to an address that lies outside the > frame. Hello! Thanks for working on this patch. I tested this patch and I can confirm it does fix the issue you linked, but I have a few inline comments. I would prefer if you wrote "the last instruction in a frame is a call" or something similar. I ask this because when I read the commit message, I thought "last instruction" was referring to last instruction executed, and couldn't understand why I couldn't reproduce without __builtin_abort. > > Using "get_frame_address_in_block" instead of > "get_frame_pc" resolves this issue. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30929 > --- > Changes since v1: > * frame-selection-last-instr-call.exp: Modify test to fix regressions on arm/aarch64 > > Patch v1: https://sourceware.org/pipermail/gdb-patches/2024-June/210251.html And responding to your v1 comment about why you created a new test file, you mention creating a new C file for this test. Have you tried extending the original frame-selection.c file with a new function that calls __builtin_abort instead? It seems to me that it should work, and you should be able to extend the existing frame-selection.exp file, but I might be missing something. I would prefer if we just extended the test, instead of added a new one, but if I indeed missed something and it isn't as easy to extend it as I think, we can have the new test :) > > gdb/stack.c | 4 +- > .../frame-selection-last-instr-call.c | 28 ++++ > .../frame-selection-last-instr-call.exp | 130 ++++++++++++++++++ > 3 files changed, 160 insertions(+), 2 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/frame-selection-last-instr-call.c > create mode 100644 gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp > > diff --git a/gdb/stack.c b/gdb/stack.c > index b36193be2f..8249866468 100644 > --- a/gdb/stack.c > +++ b/gdb/stack.c > @@ -2860,8 +2860,8 @@ find_frame_for_function (const char *function_name) > do > { > for (size_t i = 0; (i < sals.size () && !found); i++) > - found = (get_frame_pc (frame) >= func_bounds[i].low > - && get_frame_pc (frame) < func_bounds[i].high); > + found = (get_frame_address_in_block (frame) >= func_bounds[i].low > + && get_frame_address_in_block (frame) < func_bounds[i].high); > if (!found) > { > level = 1; > diff --git a/gdb/testsuite/gdb.base/frame-selection-last-instr-call.c b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.c > new file mode 100644 > index 0000000000..4f056332af > --- /dev/null > +++ b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.c > @@ -0,0 +1,28 @@ > +/* Copyright 2024 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + 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/>. */ > + > +void > +frame_1 (void) > +{ > + __builtin_abort(); > +} > + > +int > +main (void) > +{ > + frame_1 (); > +} > diff --git a/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp > new file mode 100644 > index 0000000000..5ba52cc069 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp > @@ -0,0 +1,130 @@ > +# Copyright 2024 Free Software Foundation, Inc. If we do keep this test, the copyright year should be kept as 2018-2024, since the file is very close to an existing file. > + > +# 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/>. > + > +# This tests GDB's frame selection as used by the 'frame', > +# 'select-frame', and 'info frame' commands in the corner case > +# when the last instruction in a frame is a call. > + > +standard_testfile > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} { > + return -1 > +} > + > +runto_main > +gdb_breakpoint abort > +gdb_continue_to_breakpoint abort > + > +gdb_test "bt" "#0 $hex in abort .*#1 $hex in frame_1 .*#2 $hex in main.*" "backtrace at breakpoint" Similarly, if we keep this test, I would prefer if this backtrace test used the "multi_line". That's because ".*" could eat whole lines, and make the test pass when it shouldn't. Something like: gdb_test "bt" [multi_line "#0 $hex in abort \[^\\r\\n\]*" "#1 $hex in frame_1\[^\\r\\n\]*" "#2 $hex in main \[^\\r\\n\]*"] "backtrace at breakpoint" (with the appropriate line breaks, also I haven't tested the regexes, they probably have some error in them). -- Cheers, Guinevere Larsen She/Her/Hers > + > +# Perform "info frame" to extract the frame's address. > +proc get_frame_address { {testname ""} } { > + global hex gdb_prompt > + > + set frame_address "unknown" > + set testname "get_frame_address: ${testname}" > + gdb_test_multiple "info frame" $testname { > + -re ", frame at ($hex):\r\n.*\r\n$gdb_prompt $" { > + set frame_address $expect_out(1,string) > + pass $testname > + } > + } > + > + return $frame_address > +} > + > +# Check that the current frame is at stack depth LEVEL, at frame > +# address ADDRESS, and is in FUNCTION. > +proc check_frame { level address function } { > + global hex gdb_prompt > + > + set re [multi_line \ > + "Stack level ${level}, frame at ($address):" \ > + ".* = $hex in ${function}( \(\[^\r\n\]*\))*; saved .* = $hex" \ > + ".*\r\n$gdb_prompt $" ] > + > + set testname "check frame level ${level}" > + gdb_test_multiple "info frame" $testname { > + -re $re { > + pass $testname > + } > + } > +} > + > +# Select frame using level, but relying on this being the default > +# action, so "frame 0" performs "frame level 0". > +gdb_test "frame 0" "#0 $hex in abort.*" > +set frame_0_address [ get_frame_address "frame 0" ] > +gdb_test "frame 1" "#1 $hex in frame_1.*" > +set frame_1_address [ get_frame_address "frame 1" ] > +gdb_test "frame 2" "#2 $hex in main.*" > +set frame_2_address [ get_frame_address "frame 2" ] > + > +# Select frame using 'level' specification. > +gdb_test "frame level 0" "#0 $hex in abort.*" > +gdb_test "frame level 1" "#1 $hex in frame_1.*" > +gdb_test "frame level 2" "#2 $hex in main.*" > + > +# Select frame by address. > +gdb_test "frame address ${frame_0_address}" "#0 $hex in abort.*" \ > + "select frame 0 by address" > +gdb_test "frame address ${frame_1_address}" "#1 $hex in frame_1.*" \ > + "select frame 1 by address" > +gdb_test "frame address ${frame_2_address}" "#2 $hex in main.*" \ > + "select frame 2 by address" > + > +# Select frame by function. > +gdb_test "frame function abort" "#0 $hex in abort.*" > +gdb_test "frame function frame_1" "#1 $hex in frame_1.*" > +gdb_test "frame function main" "#2 $hex in main.*" > + > +with_test_prefix "select-frame, no keyword" { > + gdb_test_no_output "select-frame 0" > + check_frame "0" "${frame_0_address}" "abort" > + gdb_test_no_output "select-frame 1" > + check_frame "1" "${frame_1_address}" "frame_1" > + gdb_test_no_output "select-frame 2" > + check_frame "2" "${frame_2_address}" "main" > +} > + > +with_test_prefix "select-frame, keyword=level" { > + gdb_test_no_output "select-frame level 0" > + check_frame "0" "${frame_0_address}" "abort" > + gdb_test_no_output "select-frame level 1" > + check_frame "1" "${frame_1_address}" "frame_1" > + gdb_test_no_output "select-frame level 2" > + check_frame "2" "${frame_2_address}" "main" > +} > + > +with_test_prefix "select-frame, keyword=address" { > + gdb_test_no_output "select-frame address ${frame_0_address}" \ > + "select frame 0 by address" > + check_frame "0" "${frame_0_address}" "abort" > + gdb_test_no_output "select-frame address ${frame_1_address}" \ > + "select frame 1 by address" > + check_frame "1" "${frame_1_address}" "frame_1" > + gdb_test_no_output "select-frame address ${frame_2_address}" \ > + "select frame 2 by address" > + check_frame "2" "${frame_2_address}" "main" > +} > + > +with_test_prefix "select-frame, keyword=function" { > + gdb_test_no_output "select-frame function abort" > + check_frame "0" "${frame_0_address}" "abort" > + gdb_test_no_output "select-frame function frame_1" > + check_frame "1" "${frame_1_address}" "frame_1" > + gdb_test_no_output "select-frame function main" > + check_frame "2" "${frame_2_address}" "main" > +} ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] gdb: fix "frame function" issue when call is last instruction 2024-07-03 14:24 ` Guinevere Larsen @ 2024-07-04 11:57 ` Sahil 0 siblings, 0 replies; 3+ messages in thread From: Sahil @ 2024-07-04 11:57 UTC (permalink / raw) To: gdb-patches, Guinevere Larsen; +Cc: Sahil Siddiq Hi, Thank you for the review. On Wednesday, July 3, 2024 7:54:43 PM GMT+5:30 Guinevere Larsen wrote: > [...] > I would prefer if you wrote "the last instruction in a frame is a call" > or something similar. I ask this because when I read the commit message, > I thought "last instruction" was referring to last instruction executed, > and couldn't understand why I couldn't reproduce without __builtin_abort. > Sorry about that. I should have been clearer. I'll modify the commit message. > > Using "get_frame_address_in_block" instead of > > "get_frame_pc" resolves this issue. > > > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30929 > > --- > > Changes since v1: > > * frame-selection-last-instr-call.exp: Modify test to fix regressions on > > arm/aarch64 > > > > Patch v1: > > https://sourceware.org/pipermail/gdb-patches/2024-June/210251.html > And responding to your v1 comment about why you created a new test file, > you mention creating a new C file for this test. Have you tried > extending the original frame-selection.c file with a new function that > calls __builtin_abort instead? I tried extending frame_selection.c initially but I was uncertain about adding a function that calls __builtin_abort since "main" would never return. I thought the last statement in main - "return i + j" - also plays a role in the test. After taking a look at the test again, I don't think that should cause a hindrance. I'll experiment with this. > It seems to me that it should work, and > you should be able to extend the existing frame-selection.exp file, but > I might be missing something. > > I would prefer if we just extended the test, instead of added a new one, > but if I indeed missed something and it isn't as easy to extend it as I > think, we can have the new test :) > I don't think extending frame-selection.exp should be difficult. I was mainly unsure about frame-selection.c. > [...] > > diff --git a/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp > > b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp new file > > mode 100644 > > index 0000000000..5ba52cc069 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/frame-selection-last-instr-call.exp > > @@ -0,0 +1,130 @@ > > +# Copyright 2024 Free Software Foundation, Inc. > > If we do keep this test, the copyright year should be kept as 2018-2024, > since the file is very close to an existing file. Ok, understood. > [...] > > +runto_main > > +gdb_breakpoint abort > > +gdb_continue_to_breakpoint abort > > + > > +gdb_test "bt" "#0 $hex in abort .*#1 $hex in frame_1 .*#2 $hex in > > main.*" "backtrace at breakpoint" > Similarly, if we keep this test, I would prefer if this backtrace test > used the "multi_line". That's because ".*" could eat whole lines, and > make the test pass when it shouldn't. Something like: > > gdb_test "bt" [multi_line "#0 $hex in abort \[^\\r\\n\]*" "#1 $hex in > frame_1\[^\\r\\n\]*" "#2 $hex in main \[^\\r\\n\]*"] "backtrace at > breakpoint" > > (with the appropriate line breaks, also I haven't tested the regexes, > they probably have some error in them). > Got it. I'll modify this. Thanks, Sahil ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-07-04 11:58 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-07-01 19:53 [PATCH v2] gdb: fix "frame function" issue when call is last instruction Sahil Siddiq 2024-07-03 14:24 ` Guinevere Larsen 2024-07-04 11:57 ` Sahil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox