From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32862 invoked by alias); 7 Feb 2020 17:07:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 32854 invoked by uid 89); 7 Feb 2020 17:07:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-1.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Feb 2020 17:07:21 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581095239; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FaCcSF1ThWIuicWerQyZFVj+TW+NGNvFDzZN77G+8UU=; b=FlIRPsSA4A3Cg8KSciJITuZs90c5mGn/AhKq5Vn7/Ls+lspjesyy/QGjrsJhxYcyW+gSCa FFVZL7PbrNmfOn7RPdjXBFELSBE/QZV5hhp7wg0JyR0AoWXwIp5U+a8a6yf+sGv6GgSiXL IjCZ9qhln3K0BMn2ZWjSE5PZXqsMjuA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-300-kFzE7QnyMZSjdJVujeWcPg-1; Fri, 07 Feb 2020 12:07:02 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9F01E1088396; Fri, 7 Feb 2020 17:07:01 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 72EB78EA1D; Fri, 7 Feb 2020 17:07:01 +0000 (UTC) From: Sergio Durigan Junior To: Luis Machado Cc: GDB Patches Subject: Re: [PATCH] New testcase for PR tui/25126 (staled source cache) References: <20200206225943.26709-1-sergiodj@redhat.com> <4a3db909-8677-fb84-39cf-84495266fdd8@linaro.org> Date: Fri, 07 Feb 2020 17:07:00 -0000 In-Reply-To: <4a3db909-8677-fb84-39cf-84495266fdd8@linaro.org> (Luis Machado's message of "Fri, 7 Feb 2020 06:41:12 -0300") Message-ID: <87wo8y9vii.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00176.txt.bz2 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 >> >> 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 = . */ >> + > > 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 >> + >> +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/testsui= te/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 . >> + > > 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] =3D=3D "extended-remo= te" } { >> + 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\"\\); /\\* brea= k-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] !=3D -1 } { >> + if { [string first "break-here" $line] !=3D -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}] !=3D ""= } { >> + 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\nSta= rting 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, --=20 Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/