From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11999 invoked by alias); 8 May 2012 07:26:54 -0000 Received: (qmail 11980 invoked by uid 22791); 8 May 2012 07:26:51 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,URIBL_BLACK X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 08 May 2012 07:26:22 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1SReoP-00012x-W3 from Hui_Zhu@mentor.com for gdb-patches@sourceware.org; Tue, 08 May 2012 00:26:22 -0700 Received: from SVR-ORW-FEM-03.mgc.mentorg.com ([147.34.97.39]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 8 May 2012 00:26:12 -0700 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-03.mgc.mentorg.com (147.34.97.39) with Microsoft SMTP Server id 14.1.289.1; Tue, 8 May 2012 00:26:20 -0700 Message-ID: <4FA8CA98.6010503@mentor.com> Date: Tue, 08 May 2012 07:26:00 -0000 From: Hui Zhu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120426 Thunderbird/13.0 MIME-Version: 1.0 To: Yao Qi CC: Subject: Re: [PATCH v2] Add autoload-breakpoints [2/6] ReportAsync-test References: <4F8562B7.20305@mentor.com> <4F9B98E2.9050208@mentor.com> <4F9C17CD.3040901@codesourcery.com> In-Reply-To: <4F9C17CD.3040901@codesourcery.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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 X-SW-Source: 2012-05/txt/msg00215.txt.bz2 On 04/29/12 00:16, Yao Qi wrote: Thanks for your review. > Hi, > Looks a mini-server-for-test is created. Why not using GDBserver? > Unless we have a strong justification to do this, I prefer testing GDB > remote feature with GDBserver. Use the small server for test, we can do more special test and make it under control. For example, we can test report-async when inferior is running or control by GDB in this test. > > When we modify the GDB internal logic on RSP, the test cases of this > kind may be affected. This increases the burden of maintenance. We still not support report async in GDB-server. Current test is tested the GDB part. When gdbserver support report-async, I will post testsuite for GDBserver. > > On the other hand, I skim over the patch, and have some comments below, > > On 04/28/2012 03:14 PM, Hui Zhu wrote: >> 2012-04-28 Hui Zhu >> >> * Makefile.in (ALL_SUBDIRS): Add gdb.remote. >> * configure (ac_config_files): Add gdb.remote/Makefile. >> * configure.ac (AC_OUTPUT): Ditto. > > configure is "Regenerated" from changed configure.ac. This entry is > incorrect. > >> --- /dev/null >> +++ b/testsuite/gdb.remote/Makefile.in > > Copyright header is missing. > >> @@ -0,0 +1,17 @@ >> +VPATH = @srcdir@ >> +srcdir = @srcdir@ >> + >> +EXECUTABLES = reportasync-test >> + >> +MISCELLANEOUS = >> + >> +all info install-info dvi install uninstall installcheck check: >> + @echo "Nothing to be done for $@..." >> + >> +clean mostlyclean: >> + rm -f *~ *.o *.x *.ci *.sl a.out core >> + rm -f $(EXECUTABLES) $(MISCELLANEOUS) >> + >> +distclean maintainer-clean realclean: clean >> + rm -f Makefile config.status config.log site.* gdb.log gdb.sum >> + >> --- /dev/null >> +++ b/testsuite/gdb.remote/reportasync-test.c >> @@ -0,0 +1,371 @@ >> +/* This testcase is part of GDB, the GNU debugger. >> + >> + Copyright 2012 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 . */ >> + >> +#include >> +#include >> +#include >> +#include > > This program is not portable. I got errors when build it on mingw32. > >> --- /dev/null >> +++ b/testsuite/gdb.remote/reportasync-test.exp >> @@ -0,0 +1,80 @@ >> +# This testcase is part of GDB, the GNU debugger. >> + >> +# Copyright 2012 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 . >> + >> +load_lib gdbserver-support.exp >> + >> +set testfile "reportasync-test" >> +set srcfile ${testfile}.c >> +set binfile ${objdir}/${subdir}/${testfile} >> + >> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { >> + untested reportasync-test.exp >> + return -1 >> +} >> + >> +gdb_exit >> +gdb_start >> +gdb_reinitialize_dir $srcdir/$subdir >> + >> +# Make sure we're disconnected, in case we're testing with an >> +# extended-remote board, therefore already connected. >> +gdb_test "disconnect" ".*" >> + >> +send_gdb "show remote report-async\n" > > Remote feature should be tested in remote target or board file. > You are testing a remote feature GDB unconditionally, even in native > gdb. It looks incorrect to me. > This just a check for the this small gdbserver is OK. I don't think this test will work with board or something. >> +gdb_expect 10 { >> + -re "Undefined show remote command" { >> + fail "ReportAsync support" >> + return >> + } >> + -re "Support for the `ReportAsync' packet is auto-detected" { >> + pass "ReportAsync support" >> + } >> +} > > Why not using gdb_test_multiple? > >> + >> +#Let GDB connect to test server >> +set portnum 2345 >> +while 1 { >> + set server_spawn_id [remote_spawn target "$binfile $portnum"] >> + expect { >> + -i $server_spawn_id >> + -notransfer >> + -re "Listening on" {} >> + -re "Can't bind port" { >> + incr portnum >> + continue >> + } >> + } >> + break >> +} >> +gdb_target_cmd "remote" ":$portnum" >> + >> +gdb_test "continue" ".*0x00000000 in ?? ().*" "Continue with AsyncReport first time" >> + >> +exec sleep 2 >> + >> +gdb_test "set debug remote 1" ".*" > > gdb_test_no_output > >> + >> +gdb_test "x 0" ".*Ignore a ReportAsync shake hands package because waiting a ack.*" "Fake shake hands package first time" >> + >> +gdb_test "set debug remote 0" ".*" > > gdb_test_no_output > >> + >> +gdb_test "continue" ".*0x00000000 in ?? ().*" "Continue with AsyncReport second time" >> + >> +gdb_test "set debug remote 1" ".*" > > gdb_test_no_output. > >> + >> +gdb_test "tstatus" ".*Ignore a ReportAsync shake hands package because waiting a response.*" "Fake shake hands package second time" >> + > > This line is too long. > Not sure it is a good idea to rely on the debugging message for testing > purpose. > > Messages in test result are not unique, > ]$ cat testsuite/gdb.sum | grep "PASS" | sort | uniq -c | sort -n > 1 PASS: gdb.remote/reportasync-test.exp: Continue with AsyncReport > first time > 1 PASS: gdb.remote/reportasync-test.exp: Continue with AsyncReport > second time > 1 PASS: gdb.remote/reportasync-test.exp: disconnect > 1 PASS: gdb.remote/reportasync-test.exp: Fake shake hands package > first time > 1 PASS: gdb.remote/reportasync-test.exp: Fake shake hands package > second time > 1 PASS: gdb.remote/reportasync-test.exp: ReportAsync support > 1 PASS: gdb.remote/reportasync-test.exp: set debug remote 0 > 2 PASS: gdb.remote/reportasync-test.exp: set debug remote 1 > Because I try to split all these patches. So I will post the new version patch later. Best, Hui