| Message ID | 20200210200221.14846-1-sergiodj@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2/10/20 5:02 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-10 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 | 43 ++++++++ > gdb/testsuite/gdb.base/cached-source-file.exp | 99 +++++++++++++++++++ > 2 files changed, 142 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..6f62c29799 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/cached-source-file.c > @@ -0,0 +1,43 @@ > +/* 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/>. */ > + > +/* Test for PR tui/25126. > + > + The bug is about a regression that makes GDB not reload its source > + code cache when the inferior's symbols are reloaded, which leads to > + wrong backtraces/listings. > + > + This bug is reproducible even without using the TUI. > + > + 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..f6e5f081f0 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/cached-source-file.exp > @@ -0,0 +1,99 @@ > +# 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/>. > + > +# Test for PR tui/25126. > +# > +# The bug is about a regression that makes GDB not reload its source > +# code cache when the inferior's symbols are reloaded, which leads to > +# wrong backtraces/listings. > +# > +# 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 > +# just copy it 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] > + > +gdb_assert { [runto "$srcfile:$bp_line"] } \ > + "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\"); /* new-marker */" > + } > + puts $bkpsrcfd $line > +} > + > +close $bkpsrcfd > +close $srcfd > +file rename -force -- $bkpsrc $srcfile > +# Here, we have to wait 1 second because of the way GDB keeps track to > +# check whether the binary has changed or not. GDB uses stat(2) and > +# currently checks 'st_mtime', whose precision is measured in > +# seconds. Since the whole file-copying/rename operation can take > +# less than 1 second, GDB can mistakenly assume that the binary is > +# still the same if we don't wait here. > +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 > +# source cache, but also to break at BP_LINE 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\"\\); /\\\* new-marker \\\*/.*" \ > + "verify that the source code is properly reloaded" > Thanks. I have no further comments on this one.
* Sergio Durigan Junior <sergiodj@redhat.com> [2020-02-10 15:02:20 -0500]: > 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-10 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 | 43 ++++++++ > gdb/testsuite/gdb.base/cached-source-file.exp | 99 +++++++++++++++++++ > 2 files changed, 142 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..6f62c29799 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/cached-source-file.c > @@ -0,0 +1,43 @@ > +/* 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/>. */ > + > +/* Test for PR tui/25126. > + > + The bug is about a regression that makes GDB not reload its source > + code cache when the inferior's symbols are reloaded, which leads to > + wrong backtraces/listings. > + > + This bug is reproducible even without using the TUI. > + > + 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..f6e5f081f0 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/cached-source-file.exp > @@ -0,0 +1,99 @@ > +# 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/>. > + > +# Test for PR tui/25126. > +# > +# The bug is about a regression that makes GDB not reload its source > +# code cache when the inferior's symbols are reloaded, which leads to > +# wrong backtraces/listings. > +# > +# 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 > +# just copy it 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] > + > +gdb_assert { [runto "$srcfile:$bp_line"] } \ > + "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\"); /* new-marker */" > + } > + puts $bkpsrcfd $line > +} > + > +close $bkpsrcfd > +close $srcfd > +file rename -force -- $bkpsrc $srcfile > +# Here, we have to wait 1 second because of the way GDB keeps track to > +# check whether the binary has changed or not. GDB uses stat(2) and > +# currently checks 'st_mtime', whose precision is measured in > +# seconds. Since the whole file-copying/rename operation can take > +# less than 1 second, GDB can mistakenly assume that the binary is > +# still the same if we don't wait here. Maybe rewrite this comment as: We have to wait 1 second because of the way GDB checks whether the binary has changed or not. GDB uses stat(2) and currently checks 'st_mtime', whose precision is measured in seconds. Since the copy, rename, and rebuild can take less than 1 second, GDB might mistakenly assume that the binary is unchanged. Otherwise this looks good to me and can be applied. Thanks, Andrew > +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 > +# source cache, but also to break at BP_LINE 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\"\\); /\\\* new-marker \\\*/.*" \ > + "verify that the source code is properly reloaded" > -- > 2.21.0 >
On Tuesday, February 11 2020, Andrew Burgess wrote: > * Sergio Durigan Junior <sergiodj@redhat.com> [2020-02-10 15:02:20 -0500]: [...] >> +close $bkpsrcfd >> +close $srcfd >> +file rename -force -- $bkpsrc $srcfile >> +# Here, we have to wait 1 second because of the way GDB keeps track to >> +# check whether the binary has changed or not. GDB uses stat(2) and >> +# currently checks 'st_mtime', whose precision is measured in >> +# seconds. Since the whole file-copying/rename operation can take >> +# less than 1 second, GDB can mistakenly assume that the binary is >> +# still the same if we don't wait here. > > Maybe rewrite this comment as: > > We have to wait 1 second because of the way GDB checks whether the > binary has changed or not. GDB uses stat(2) and currently checks > 'st_mtime', whose precision is measured in seconds. Since the copy, > rename, and rebuild can take less than 1 second, GDB might mistakenly > assume that the binary is unchanged. > > Otherwise this looks good to me and can be applied. Thanks, I adopted your version of the text and pushed the patch. f6be87130b5b327075a09c05e78532816f186995 Thank you and Luis for the reviews.
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..6f62c29799 --- /dev/null +++ b/gdb/testsuite/gdb.base/cached-source-file.c @@ -0,0 +1,43 @@ +/* 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/>. */ + +/* Test for PR tui/25126. + + The bug is about a regression that makes GDB not reload its source + code cache when the inferior's symbols are reloaded, which leads to + wrong backtraces/listings. + + This bug is reproducible even without using the TUI. + + 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..f6e5f081f0 --- /dev/null +++ b/gdb/testsuite/gdb.base/cached-source-file.exp @@ -0,0 +1,99 @@ +# 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/>. + +# Test for PR tui/25126. +# +# The bug is about a regression that makes GDB not reload its source +# code cache when the inferior's symbols are reloaded, which leads to +# wrong backtraces/listings. +# +# 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 +# just copy it 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] + +gdb_assert { [runto "$srcfile:$bp_line"] } \ + "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\"); /* new-marker */" + } + puts $bkpsrcfd $line +} + +close $bkpsrcfd +close $srcfd +file rename -force -- $bkpsrc $srcfile +# Here, we have to wait 1 second because of the way GDB keeps track to +# check whether the binary has changed or not. GDB uses stat(2) and +# currently checks 'st_mtime', whose precision is measured in +# seconds. Since the whole file-copying/rename operation can take +# less than 1 second, GDB can mistakenly assume that the binary is +# still the same if we don't wait here. +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 +# source cache, but also to break at BP_LINE 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\"\\); /\\\* new-marker \\\*/.*" \ + "verify that the source code is properly reloaded"