[PATCH] New testcase for PR tui/25126 (staled source cache)
Sergio Durigan Junior
sergiodj@redhat.com
Fri Feb 7 17:07:00 GMT 2020
On Friday, February 07 2020, Luis Machado wrote:
> On 2/6/20 7:59 PM, Sergio Durigan Junior wrote:
>> I'm dealing with a Fedora GDB bug that is related to PR tui/25126, and
>> I thought I'd write a specific testcase for it because I couldn't find
>> one.
>>
>> The idea is to get the simple reproducer from the bug and tweak the
>> testcase around it. This one was a bit hard because, since we need to
>> modify the source file and recompile it, it involved a bit of TCL-foo
>> to do things. Also for this reason, I'm only enabling the test for
>> native boards.
>>
>> I tested this with an upstream GDB and made sure everything is
>> passing. I also tested with a faulty GDB and made sure the test
>> failed.
>>
>> gdb/testsuite/ChangeLog:
>> 2020-02-07 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> PR tui/25126
>> * gdb.base/cached-source-file.c: New file.
>> * gdb.base/cached-source-file.exp: New file.
>>
>> Change-Id: Ib1b074342ebe8613c6d1dfde631691ebdf6d81c6
>> ---
>> gdb/testsuite/gdb.base/cached-source-file.c | 37 ++++++++
>> gdb/testsuite/gdb.base/cached-source-file.exp | 94 +++++++++++++++++++
>> 2 files changed, 131 insertions(+)
>> create mode 100644 gdb/testsuite/gdb.base/cached-source-file.c
>> create mode 100644 gdb/testsuite/gdb.base/cached-source-file.exp
>>
>> diff --git a/gdb/testsuite/gdb.base/cached-source-file.c b/gdb/testsuite/gdb.base/cached-source-file.c
>> new file mode 100644
>> index 0000000000..9f698dcffe
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/cached-source-file.c
>> @@ -0,0 +1,37 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2020 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/>. */
>> +
>
> Instead of pointing at a PR number, wouldn't it be better to state
> what this particular source file plans to achieve or help achieve?
>
> Personally, i find it a bit more useful.
>
> Then again, if it is a simple test and it will be obvious from code
> comments, it shouldn't be necessary.
Thanks for the review. Sure, I can put a more descriptive comment here.
>> +/* Test for PR tui/25126.
>> +
>> + The .exp testcase depends on the line numbers and contents from
>> + this file If you change this file, make sure to double-check the
>> + testcase. */
>> +
>> +#include <stdio.h>
>> +
>> +void
>> +foo (void)
>> +{
>> + printf ("hello\n"); /* break-here */
>> +}
>> +
>> +int
>> +main ()
>> +{
>> + foo ();
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.base/cached-source-file.exp b/gdb/testsuite/gdb.base/cached-source-file.exp
>> new file mode 100644
>> index 0000000000..f98bec09ca
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/cached-source-file.exp
>> @@ -0,0 +1,94 @@
>> +# Copyright (C) 2020 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/>.
>> +
>
> Same as above about explaining what the test wants to achieve.
>
>> +# Test for PR tui/25126.
>
> Would it make the test run fine with remote stubs if the TUI wasn't
> used? Just wondering.
TUI isn't being used. The reason why I chose to restrict the test to
native boards is because of the native TCL commands I use below (to
copy/rename/modify files). I.e., the test doesn't use the
"remote_exec..." functions.
>> +# This bug is reproducible even without using the TUI.
>> +
>> +standard_testfile
>> +
>> +# Only run on native boards.
>> +if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
>> + return -1
>> +}
>> +
>> +# Because we need to modify the source file later, it's better if we
>
> Typo... "just copy it to our..."
Heh, I was trying to find this excerpt in the text above, but then
noticed that you are reviewing things *below* your comments.
Thanks, I'll fix this.
>> +# just copy if to our output directory (instead of messing with the
>> +# user's source directory).
>> +set newsrc [standard_output_file $testfile].c
>> +file copy -force -- $srcdir/$subdir/$srcfile $newsrc
>> +set srcfile $newsrc
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> + return -1
>> +}
>> +
>> +# Get the line number for the line with the "break-here" marker.
>> +set bp_line [gdb_get_line_number "break-here" $srcfile]
>> +
>
> I've learned about it recently, but i think using gdb_assert here
> would be nice instead of the if/else block.
OK.
>> +if { [runto "$srcfile:$bp_line"] } {
>> + pass "run to $srcfile:$bp_line"
>> +} else {
>> + fail "run to $srcfile:$bp_line"
>> +}
>> +
>> +# Do a "list" and check that the printed line matches the line of the
>> +# original source file.
>> +gdb_test_no_output "set listsize 1"
>> +gdb_test "list" "$bp_line\[ \t\]+printf \\(\"hello\\\\n\"\\); /\\* break-here \\*/" \
>> + "check the first version of the source file"
>> +
>> +# Modify the original source file, and add an extra line into it.
>> +# This only works locally because of the TCL commands.
>> +set bkpsrc [standard_output_file $testfile].c.bkp
>> +set bkpsrcfd [open $bkpsrc w]
>> +set srcfd [open $srcfile r]
>> +
>> +while { [gets $srcfd line] != -1 } {
>> + if { [string first "break-here" $line] != -1 } {
>> + # Put a "printf" line before the "break-here" line.
>> + puts $bkpsrcfd " printf (\"foo\\n\");"
>> + }
>> + puts $bkpsrcfd $line
>> +}
>> +
>> +close $bkpsrcfd
>> +close $srcfd
>> +file rename -force $bkpsrc $srcfile
>> +# Give it some time to perform the renaming. For some reason, TCL
>> +# needs some time after "file rename" in order to properly rename the
>> +# file.
>
> In case a system takes longer, should we loop and make sure the
> renamed file really exists, as opposed to sleeping for just a second?
Yeah, that's a good idea.
On a side note, it seems really strange to me that TCL needs this "extra
time". rename (the syscall) is atomic, so I don't know why I am seeing
this behaviour. It'd be great if you (or someone else) could remove the
"sleep 1", run the test a few times and see if it still passes.
>> +sleep 1
>> +
>> +# Recompile the modified source. We use "gdb_compile" here instead of
>> +# "prepare_for_testing" because we don't want to call "clean_restart".
>> +if { [gdb_compile "${srcfile}" "${binfile}" executable {debug}] != "" } {
>> + return -1
>> +}
>> +
>> +# Rerun the program. This should not only force GDB to reload the
>
> I'm guessing line 6 is the one with the "break-here" marker? In that
> case, should we mention that instead of the line number? Unless the
> line number is really important.
Ah, right. Will do that.
>> +# source cache, but also to break at line 6 again, which now has
>> +# different contents.
>> +gdb_test_multiple "run" "rerun program" {
>> + -re {The program being debugged has been started already\.\r\nStart it from the beginning\? \(y or n\) $} {
>> + set binregex [string_to_regexp $binfile]
>> + gdb_test "y" "\\`$binregex\\' has changed; re-reading symbols\.\r\nStarting program: ${binregex}.*" \
>> + "rerun program"
>> + }
>> +}
>> +
>> +# Again, perform the listing and check that the line indeed has
>> +# changed for GDB.
>> +gdb_test "list" "${bp_line}\[ \t\]+printf \\(\"foo\\\\n\"\\);" \
>> + "verify that the source code is properly reloaded"
>>
>
> Otherwise LGTM. Thanks!
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
More information about the Gdb-patches
mailing list