From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
To: Bruno Larsen <blarsen@redhat.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH v4 03/15] Change gdb.base/skip-solib.exp deal with lack of epilogue information
Date: Sat, 10 Sep 2022 10:53:43 +0100 [thread overview]
Message-ID: <87y1urlfmg.fsf@redhat.com> (raw)
In-Reply-To: <20220720194441.168906-5-blarsen@redhat.com>
Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> When running gdb.base/skip-solib.exp, the backtrace tests could fail with
> compilers that associated epilogue instructions with the last statement
> line of the function, instead of associating it with the closing brace,
> despite the feature being fully functional. As an example, when testing
> skipping the function square, the testsuite would show
>
> Breakpoint 1, main () at (...)/binutils-gdb/gdb/testsuite/gdb.base/skip-solib-main.c:5
> 5 return square(0);
> (gdb) step
> 0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6
> (gdb) PASS: gdb.base/skip-solib.exp: ignoring solib file: step
> bt
> #0 0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6
> #1 0x00007ffff7cef60c in __libc_start_main_impl () from /lib64/libc.so.6
> #2 0x0000000000401065 in _start ()
> (gdb) FAIL: gdb.base/skip-solib.exp: ignoring solib file: bt
>
> Which means that the feature is working, the testsuite is just
> mis-identifying it. To avoid this problem, the skipped function calls
> have been sent to a line before `return`, so epilogues won't factor in.
>
> This commit has also changed a few hardcoded steps to leave functions to
> the newly introduced gdb_step_until to leave those functions.
I think I would like to see the skip-inline.exp change moved into a
separate commit given it's a completely different type of fix.
> ---
> gdb/testsuite/gdb.base/skip-inline.exp | 23 ++++++++++++++++-------
> gdb/testsuite/gdb.base/skip-solib-lib.c | 3 ++-
> gdb/testsuite/gdb.base/skip-solib-main.c | 3 ++-
> gdb/testsuite/gdb.base/skip-solib.exp | 12 ++++++++++--
> 4 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp
> index f6e9926b66c..3fbaa5469dd 100644
> --- a/gdb/testsuite/gdb.base/skip-inline.exp
> +++ b/gdb/testsuite/gdb.base/skip-inline.exp
> @@ -15,6 +15,11 @@
>
> standard_testfile
>
> +set epilogue 1
> +if {![have_epilogue_line_info]} {
> + set epilogue 0
> +}
I think 'set epilogue [have_epilogue_line_info]' would be better.
> +
> if { [prepare_for_testing "failed to prepare" "skip-inline" \
> {skip-inline.c skip1.c } \
> {debug nowarnings}] } {
> @@ -35,16 +40,20 @@ gdb_test "skip function foo" "Function foo will be skipped when stepping\."
> gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
> gdb_test "step" ".*" "step into baz, since foo will be skipped"
> gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
> -gdb_test "step" ".*" "step in the baz"
> -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
> -gdb_test "step" ".*" "step back to main"
> +gdb_step_until ".*x = 0; x = baz \\(foo \\(\\)\\).*"
> gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
> gdb_test "step" ".*" "step again into baz, since foo will be skipped"
> gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
> -gdb_test "step" ".*" "step in the baz, again"
> -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
> -gdb_test "step" ".*" "step back to main, again"
> -gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
> +gdb_step_until "main \\(\\) at .*" "step back to main, again"
> +gdb_test "bt" "\\s*\\#0.*main.*" "again back to main"
> +
> +# Because clang doesn't add epilogue information, having a set number of
> +# steps puts clang more and more out of sync with gcc. It is unlikely that
> +# the effort of keeping both outputs will be useful.
> +if {$epilogue == 0} {
Just 'if { !$epilogue } {' would be better.
> + untested "Multiple steps tests are not supported with this compiler"
> + return
> +}
I notice that there's actually another test at the end of this file that
doesn't rely on multiple steps, which we now end up skipping due to this
early return.
I wonder if this test file would be better structured something like:
proc_with_prefix single_step { } {
# The first block of tests that just does 'step'.
}
proc_with_prefix double_step { } {
# The second block of tests that do 'step 2'.
}
proc_with_prefix triple_step { } {
# The third block of tests that do 'step 3'.
}
proc_with_prefix skip_current_frame { } {
# The final bit of test that sets up a skip of foo.
}
single_step
if { $epilogue } {
double_step
triple_step
}
skip_current_frame
>
> if ![runto_main] {
> return
> diff --git a/gdb/testsuite/gdb.base/skip-solib-lib.c b/gdb/testsuite/gdb.base/skip-solib-lib.c
> index b2c4d86d703..341f1440a3b 100644
> --- a/gdb/testsuite/gdb.base/skip-solib-lib.c
> +++ b/gdb/testsuite/gdb.base/skip-solib-lib.c
> @@ -7,5 +7,6 @@ int multiply(int a, int b)
>
> int square(int num)
> {
> - return multiply(num, num);
> + int res = multiply(num, num);
> + return res;
> }
> diff --git a/gdb/testsuite/gdb.base/skip-solib-main.c b/gdb/testsuite/gdb.base/skip-solib-main.c
> index 746bb5f36bb..a3b6d417935 100644
> --- a/gdb/testsuite/gdb.base/skip-solib-main.c
> +++ b/gdb/testsuite/gdb.base/skip-solib-main.c
> @@ -2,5 +2,6 @@ int square(int num);
>
> int main()
> {
> - return square(0);
> + int s = square(0);
> + return s;
> }
> diff --git a/gdb/testsuite/gdb.base/skip-solib.exp b/gdb/testsuite/gdb.base/skip-solib.exp
> index 0f2ce7e1ad8..8e61725ad1b 100644
> --- a/gdb/testsuite/gdb.base/skip-solib.exp
> +++ b/gdb/testsuite/gdb.base/skip-solib.exp
> @@ -82,7 +82,7 @@ with_test_prefix "ignoring solib file" {
> # We shouldn't step into square(), since we skipped skip-solib-lib.c.
> #
> gdb_test "step" ""
> - gdb_test "bt" "#0\\s+main.*"
> + gdb_test "bt 1" "#0\\s+main.*"
> }
>
> #
> @@ -114,5 +114,13 @@ with_test_prefix "ignoring solib function" {
> # the last line of square.
> #
> gdb_test "step" ""
> - gdb_test "bt" "#0\\s+square.*"
> + gdb_test "bt 1" "#0\\s+square.*" "skipped multiply"
> +# gdb_test_multiple "bt 1" "skipped multiply" {
> +# -re "#0\\s+square.*" {
> +# pass "skipped multiply"
> +# }
> +# -re "#0.*main.*" {
> +# pass "skipped multiply"
> +# }
> +# }
This commented out code should be removed.
In fact, I wonder if any of the changes in skip-solib.exp are actually
needed. Sure, 'bt 1' is maybe a little more specific than 'bt', but
given the pattern we check for doesn't change, I don't think this change
should make any difference.
If this is just a preference/cleanup then this should probably move into
a sperate patch to avoid any confusion. Or just drop this part?
Thanks,
Andrew
> }
> --
> 2.31.1
next prev parent reply other threads:[~2022-09-10 9:54 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-20 19:44 [PATCH v4 00/15] Clean gdb.base when testing with clang Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 01/15] gdb/testsuite: introduce gdb_step_until Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 01/15] gdb/testsuite: introduce gdb_step_until_regexp Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 02/15] gdb/testsuite: Add a proc to test where compiler links the epilogue Bruno Larsen via Gdb-patches
2022-09-13 12:17 ` Andrew Burgess via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 03/15] Change gdb.base/skip-solib.exp deal with lack of epilogue information Bruno Larsen via Gdb-patches
2022-09-10 9:53 ` Andrew Burgess via Gdb-patches [this message]
2022-07-20 19:44 ` [PATCH v4 04/15] gdb/testsuite: change gdb.base/nodebug.exp to not fail with clang Bruno Larsen via Gdb-patches
2022-09-12 9:08 ` Andrew Burgess via Gdb-patches
2022-09-12 12:17 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 05/15] update gdb.base/info-program.exp " Bruno Larsen via Gdb-patches
2022-09-12 9:34 ` Andrew Burgess via Gdb-patches
2022-09-12 12:18 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 06/15] fix gdb.base/access-mem-running.exp for clang testing Bruno Larsen via Gdb-patches
2022-09-12 9:41 ` Andrew Burgess via Gdb-patches
2022-09-12 12:18 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 07/15] Fix gdb.base/call-ar-st to work with Clang Bruno Larsen via Gdb-patches
2022-09-12 10:30 ` Andrew Burgess via Gdb-patches
2022-09-12 12:18 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 08/15] add xfails to gdb.base/complex-parts.exp when testing with clang Bruno Larsen via Gdb-patches
2022-09-12 10:49 ` Andrew Burgess via Gdb-patches
2022-09-12 12:18 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 09/15] gdb/testsuite: fix gdb.base/msym-bp-shl when running with Clang Bruno Larsen via Gdb-patches
2022-09-12 10:58 ` Andrew Burgess via Gdb-patches
2022-09-12 12:30 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 10/15] explicitly test for stderr in gdb.base/dprintf.exp Bruno Larsen via Gdb-patches
2022-09-12 12:20 ` Andrew Burgess via Gdb-patches
2022-09-13 12:08 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 11/15] gdb/testsuite: Update gdb.base/so-impl-ld.exp Bruno Larsen via Gdb-patches
2022-09-12 12:30 ` Andrew Burgess via Gdb-patches
2022-09-13 12:08 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 12/15] [gdb/testsuite]: fix gdb.base/jit-elf.exp when testing with clang Bruno Larsen via Gdb-patches
2022-09-12 12:54 ` Andrew Burgess via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 13/15] gdb/testsuite: fix gdb.base/info-types-c++ " Bruno Larsen via Gdb-patches
2022-09-12 14:35 ` Andrew Burgess via Gdb-patches
2022-09-14 11:31 ` Bruno Larsen via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 14/15] gdb.base/skip.exp: Use finish to exit functions Bruno Larsen via Gdb-patches
2022-09-12 16:57 ` Andrew Burgess via Gdb-patches
2022-07-20 19:44 ` [PATCH v4 15/15] gdb/testsuite: Add test to step through function epilogue Bruno Larsen via Gdb-patches
2022-09-08 12:04 ` Andrew Burgess via Gdb-patches
2022-08-09 16:53 ` [PIING][PATCH v4 00/15] Clean gdb.base when testing with clang Bruno Larsen via Gdb-patches
2022-08-18 7:25 ` [PINGv2][PATCH " Bruno Larsen via Gdb-patches
2022-08-25 7:51 ` [PINGv3][PATCH " Bruno Larsen via Gdb-patches
2022-09-05 14:59 ` [PINGv4][PATCH " Bruno Larsen via Gdb-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y1urlfmg.fsf@redhat.com \
--to=gdb-patches@sourceware.org \
--cc=aburgess@redhat.com \
--cc=blarsen@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox