Message ID | 5371910C.9020607@mentor.com |
---|---|
State | Committed |
Headers |
Return-Path: <x14314964@homiemail-mx21.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (peon2454.g.dreamhost.com [208.113.200.127]) by wilcox.dreamhost.com (Postfix) with ESMTP id 7971136007A for <siddhesh@wilcox.dreamhost.com>; Mon, 12 May 2014 20:27:20 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14314964) id 9B1E218CD4AE; Mon, 12 May 2014 20:27:19 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx21.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx21.g.dreamhost.com (Postfix) with ESMTPS id 66CCA18CD4A6 for <gdb@patchwork.siddhesh.in>; Mon, 12 May 2014 20:27:19 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:content-type:content-transfer-encoding; q=dns; s= default; b=GpyEm8A4Al1hXO34cnUpFI/gDSMReYy7Kp9irCOw5Le3hCvUfdCLM W9GzjVM+9iWnXQsuLnaAOJn68xtPzksxCct4vgUWx3NMtKV/v5w+eXiccFWx/HeQ hkIvvbeCLmPRTF/McgeD/PPMueXpi9hsN5bNb8OV6OT3/t4pOebnrE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:content-type:content-transfer-encoding; s=default; bh=R bfC+1qoxzkP8C9Qq32PjQrMOhg=; b=d6DihxBUC5hfALRraypcswaJStOkOHOK6 xzd4j8PMMocRrQKHFOf7cWy8VVSvVw8sxW21KpXGLvMul15yU+gb79R+5l46yhSK iOsN1UPBgzAG7m3fdnHuSj9mC7vAc2xIiSuJWIE9604NQTtruXLGblFd52AYfcVc OnO9CrfZpU= Received: (qmail 28092 invoked by alias); 13 May 2014 03:27:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-gdb=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 28080 invoked by uid 89); 13 May 2014 03:27:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.0 required=5.0 tests=none autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 13 May 2014 03:27:14 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1Wk3N0-0007b3-Ip from Hui_Zhu@mentor.com for gdb-patches@sourceware.org; Mon, 12 May 2014 20:27:10 -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); Mon, 12 May 2014 20:27:10 -0700 Received: from localhost.localdomain (147.34.91.1) by svr-orw-fem-03.mgc.mentorg.com (147.34.97.39) with Microsoft SMTP Server id 14.2.247.3; Mon, 12 May 2014 20:27:09 -0700 Message-ID: <5371910C.9020607@mentor.com> Date: Tue, 13 May 2014 11:27:08 +0800 From: Hui Zhu <hui_zhu@mentor.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: gdb-patches ml <gdb-patches@sourceware.org> CC: Nathan Sidwell <nathan_sidwell@mentor.com> Subject: [PATCH] Add system test before "set remote system-call-allowed 1" Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in |
Commit Message
Hui Zhu
May 13, 2014, 3:27 a.m. UTC
This patch is update version according to the discussion in https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. If test get the target doesn't support fileio system according to the remote log. It will set this test as "unsupported". Before I made this patch, I want add a check before all of tests in this file. But I found that the target maybe support one call but not others. For example: my target support Fwrite, Fopen and so on. But not Fgettimeofday. And it doesn't support Fsystem NULL but it support Fsystem not NULL. So I think if we want to check target support fileio, we need check them one by one. Thanks, Hui 2014-05-13 Nathan Sidwell <nathan@codesourcery.com> Hui Zhu <hui@codesourcery.com> * gdb.base/fileio.exp: Add test for shell not available as well as available. * gdb.base/fileio.c (test_system): Check for shell twice.
Comments
Ping. Thanks, Hui On Tue, May 13, 2014 at 11:27 AM, Hui Zhu <hui_zhu@mentor.com> wrote: > This patch is update version according to the discussion in > https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. > If test get the target doesn't support fileio system according to the > remote log. It will set this test as "unsupported". > > Before I made this patch, I want add a check before all of tests in this > file. But I found that the target maybe support one call but not others. > For example: my target support Fwrite, Fopen and so on. But not > Fgettimeofday. > And it doesn't support Fsystem NULL but it support Fsystem not NULL. > So I think if we want to check target support fileio, we need check them > one by one. > > Thanks, > Hui > > 2014-05-13 Nathan Sidwell <nathan@codesourcery.com> > Hui Zhu <hui@codesourcery.com> > > * gdb.base/fileio.exp: Add test for shell not available as well as > available. > * gdb.base/fileio.c (test_system): Check for shell twice. > > --- a/gdb/testsuite/gdb.base/fileio.c > +++ b/gdb/testsuite/gdb.base/fileio.c > @@ -55,7 +55,11 @@ time(time_t *t); > Not applicable. > > system (const char * string); > -1) Invalid string/command. - returns 127. */ > +1) See if shell available - returns 0 > +2) See if shell available - returns !0 > +3) Execute simple shell command - returns 0 > +4) Invalid string/command. - returns 127. */ > + > static const char *strerrno (int err); > > /* Note that OUTDIR is defined by the test suite. */ > @@ -375,21 +379,26 @@ test_system () > */ > int ret; > > - /* Test for shell */ > + /* Test for shell (testsuite should have it disabled). */ > ret = system (NULL); > - printf ("system 1: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); > + printf ("system 1: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); > + stop (); > + /* Test for shell again (the testsuite will have enabled it now). */ > + ret = system (NULL); > + printf ("system 2: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); > stop (); > /* This test prepares the directory for test_rename() */ > sprintf (sys, "mkdir -p %s/%s %s/%s", OUTDIR, TESTSUBDIR, OUTDIR, > TESTDIR2); > ret = system (sys); > if (ret == 127) > - printf ("system 2: ret = %d /bin/sh unavailable???\n", ret); > + printf ("system 3: ret = %d /bin/sh unavailable???\n", ret); > else > - printf ("system 2: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); > + printf ("system 3: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); > stop (); > /* Invalid command (just guessing ;-) ) */ > ret = system ("wrtzlpfrmpft"); > - printf ("system 3: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? "OK" : > ""); > + printf ("system 4: ret = %d %s\n", ret, > + WEXITSTATUS (ret) == 127 ? "OK" : ""); > stop (); > } > > --- a/gdb/testsuite/gdb.base/fileio.exp > +++ b/gdb/testsuite/gdb.base/fileio.exp > @@ -180,19 +180,34 @@ gdb_test continue \ > "Continuing\\..*isatty 5:.*OK$stop_msg" \ > "Isatty (open file)" > > -gdb_test continue \ > -"Continuing\\..*system 1:.*OK$stop_msg" \ > -"System says shell is available" > +gdb_test_no_output "set debug remote 1" > +set msg "System says shell is not available" > +gdb_test_multiple "continue" $msg { > + -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { > + pass $msg > + } > + -re ".*Fsystem.*$gdb_prompt $" { > + fail $msg > + } > + -re ".*$gdb_prompt $" { > + unsupported $msg > + } > +} > +gdb_test_no_output "set debug remote 0" > > gdb_test_no_output "set remote system-call-allowed 1" > > gdb_test continue \ > "Continuing\\..*system 2:.*OK$stop_msg" \ > +"System says shell is available" > + > +gdb_test continue \ > +"Continuing\\..*system 3:.*OK$stop_msg" \ > "System(3) call" > > # Is this ok? POSIX says system returns a waitpid status? > gdb_test continue \ > -"Continuing\\..*system 3:.*OK$stop_msg" \ > +"Continuing\\..*system 4:.*OK$stop_msg" \ > "System with invalid command returns 127" > > gdb_test continue \
On 05/13/2014 04:27 AM, Hui Zhu wrote: > This patch is update version according to the discussion in > https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. > If test get the target doesn't support fileio system according to the > remote log. It will set this test as "unsupported". > > Before I made this patch, I want add a check before all of tests in this > file. But I found that the target maybe support one call but not others. > For example: my target support Fwrite, Fopen and so on. But not > Fgettimeofday. > And it doesn't support Fsystem NULL but it support Fsystem not NULL. So IIUC, the test will still have system (NULL) FAIL on your target, right? > So I think if we want to check target support fileio, we need check them > one by one. Against native, we'll get a new UNSUPPORTED, right? > @@ -375,21 +379,26 @@ test_system () > */ > int ret; > > - /* Test for shell */ > + /* Test for shell (testsuite should have it disabled). */ This comment confused me a bit, as I didn't recall that this is disabled by default. So, I'd prefer: /* Test for shell ('set remote system-call-allowed' is disabled by default). */ > ret = system (NULL); > - printf ("system 1: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); > + printf ("system 1: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); > + stop (); > + /* Test for shell again (the testsuite will have enabled it now). */ > + ret = system (NULL); > + printf ("system 2: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); > stop (); > /* This test prepares the directory for test_rename() */ > sprintf (sys, "mkdir -p %s/%s %s/%s", OUTDIR, TESTSUBDIR, OUTDIR, > TESTDIR2); > ret = system (sys); > if (ret == 127) > - printf ("system 2: ret = %d /bin/sh unavailable???\n", ret); > + printf ("system 3: ret = %d /bin/sh unavailable???\n", ret); > else > - printf ("system 2: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); > + printf ("system 3: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); > stop (); > /* Invalid command (just guessing ;-) ) */ > ret = system ("wrtzlpfrmpft"); > - printf ("system 3: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? > "OK" : ""); > + printf ("system 4: ret = %d %s\n", ret, > + WEXITSTATUS (ret) == 127 ? "OK" : ""); > stop (); > } > > --- a/gdb/testsuite/gdb.base/fileio.exp > +++ b/gdb/testsuite/gdb.base/fileio.exp > @@ -180,19 +180,34 @@ gdb_test continue \ > "Continuing\\..*isatty 5:.*OK$stop_msg" \ > "Isatty (open file)" > > -gdb_test continue \ > -"Continuing\\..*system 1:.*OK$stop_msg" \ > -"System says shell is available" > +gdb_test_no_output "set debug remote 1" > +set msg "System says shell is not available" > +gdb_test_multiple "continue" $msg { > + -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { > + pass $msg > + } > + -re ".*Fsystem.*$gdb_prompt $" { > + fail $msg > + } > + -re ".*$gdb_prompt $" { > + unsupported $msg > + } No need for leading '.*' : -re "Fsystem.*$gdb_prompt $" { fail $msg } -re "$gdb_prompt $" { unsupported $msg } OK with those changes.
On 05/29/14 19:12, Pedro Alves wrote: > On 05/13/2014 04:27 AM, Hui Zhu wrote: >> This patch is update version according to the discussion in >> https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. >> If test get the target doesn't support fileio system according to the >> remote log. It will set this test as "unsupported". >> >> Before I made this patch, I want add a check before all of tests in this >> file. But I found that the target maybe support one call but not others. >> For example: my target support Fwrite, Fopen and so on. But not >> Fgettimeofday. >> And it doesn't support Fsystem NULL but it support Fsystem not NULL. > > So IIUC, the test will still have system (NULL) FAIL on your > target, right? It will use fileio if argument of Fsystem is not NULL. But will not use fileio if its argument is NULL. > >> So I think if we want to check target support fileio, we need check them >> one by one. > > Against native, we'll get a new UNSUPPORTED, right? With current patch, we will get a new UNSUPPORTED because it just check if target support fileio system(NULL) without "set remote system-call-allowed 1". > >> @@ -375,21 +379,26 @@ test_system () >> */ >> int ret; >> >> - /* Test for shell */ >> + /* Test for shell (testsuite should have it disabled). */ > > This comment confused me a bit, as I didn't recall that > this is disabled by default. So, I'd prefer: > > /* Test for shell ('set remote system-call-allowed' is disabled > by default). */ Updated. > >> ret = system (NULL); >> - printf ("system 1: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); >> + printf ("system 1: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); >> + stop (); >> + /* Test for shell again (the testsuite will have enabled it now). */ >> + ret = system (NULL); >> + printf ("system 2: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); >> stop (); >> /* This test prepares the directory for test_rename() */ >> sprintf (sys, "mkdir -p %s/%s %s/%s", OUTDIR, TESTSUBDIR, OUTDIR, >> TESTDIR2); >> ret = system (sys); >> if (ret == 127) >> - printf ("system 2: ret = %d /bin/sh unavailable???\n", ret); >> + printf ("system 3: ret = %d /bin/sh unavailable???\n", ret); >> else >> - printf ("system 2: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); >> + printf ("system 3: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); >> stop (); >> /* Invalid command (just guessing ;-) ) */ >> ret = system ("wrtzlpfrmpft"); >> - printf ("system 3: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? >> "OK" : ""); >> + printf ("system 4: ret = %d %s\n", ret, >> + WEXITSTATUS (ret) == 127 ? "OK" : ""); >> stop (); >> } >> >> --- a/gdb/testsuite/gdb.base/fileio.exp >> +++ b/gdb/testsuite/gdb.base/fileio.exp >> @@ -180,19 +180,34 @@ gdb_test continue \ >> "Continuing\\..*isatty 5:.*OK$stop_msg" \ >> "Isatty (open file)" >> >> -gdb_test continue \ >> -"Continuing\\..*system 1:.*OK$stop_msg" \ >> -"System says shell is available" >> +gdb_test_no_output "set debug remote 1" >> +set msg "System says shell is not available" >> +gdb_test_multiple "continue" $msg { >> + -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { >> + pass $msg >> + } >> + -re ".*Fsystem.*$gdb_prompt $" { >> + fail $msg >> + } >> + -re ".*$gdb_prompt $" { >> + unsupported $msg >> + } > > No need for leading '.*' : > > -re "Fsystem.*$gdb_prompt $" { > fail $msg > } > -re "$gdb_prompt $" { > unsupported $msg > } > > OK with those changes. > Committed as https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9f5a4cef68413e211bc765e969bf6778150231db Thanks, Hui
On 06/04/2014 07:44 AM, Hui Zhu wrote: > On 05/29/14 19:12, Pedro Alves wrote: >> On 05/13/2014 04:27 AM, Hui Zhu wrote: >>> This patch is update version according to the discussion in >>> https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. >>> If test get the target doesn't support fileio system according to the >>> remote log. It will set this test as "unsupported". >>> >>> Before I made this patch, I want add a check before all of tests in this >>> file. But I found that the target maybe support one call but not others. >>> For example: my target support Fwrite, Fopen and so on. But not >>> Fgettimeofday. >>> And it doesn't support Fsystem NULL but it support Fsystem not NULL. >> >> So IIUC, the test will still have system (NULL) FAIL on your >> target, right? > > It will use fileio if argument of Fsystem is not NULL. > But will not use fileio if its argument is NULL. I mean, you still get one FAIL when you run the test against your target, even with your patch applied. Otherwise, I think I'm missing something. Hmm, looking again at the patch, I'm pretty much confused. :-/ gdb_test continue \ -"Continuing\\..*system 1:.*OK$stop_msg" \ -"System says shell is available" +gdb_test_no_output "set debug remote 1" +set msg "System says shell is not available" +gdb_test_multiple "continue" $msg { + -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { ^^^^^^^ What's this "Fsystem" supposed to be matching here? At this point "set remote system-call-allowed 1" has NOT been issued yet, so how come we expect to see that? + pass $msg + } + -re ".*Fsystem.*$gdb_prompt $" { + fail $msg + } + -re "$gdb_prompt $" { + unsupported $msg + } +} +gdb_test_no_output "set debug remote 0"
On 06/05/14 00:29, Pedro Alves wrote: > On 06/04/2014 07:44 AM, Hui Zhu wrote: >> On 05/29/14 19:12, Pedro Alves wrote: >>> On 05/13/2014 04:27 AM, Hui Zhu wrote: >>>> This patch is update version according to the discussion in >>>> https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. >>>> If test get the target doesn't support fileio system according to the >>>> remote log. It will set this test as "unsupported". >>>> >>>> Before I made this patch, I want add a check before all of tests in this >>>> file. But I found that the target maybe support one call but not others. >>>> For example: my target support Fwrite, Fopen and so on. But not >>>> Fgettimeofday. >>>> And it doesn't support Fsystem NULL but it support Fsystem not NULL. >>> >>> So IIUC, the test will still have system (NULL) FAIL on your >>> target, right? >> >> It will use fileio if argument of Fsystem is not NULL. >> But will not use fileio if its argument is NULL. > > I mean, you still get one FAIL when you run the test > against your target, even with your patch applied. No. It will not because the fail just happen when the target use remote file io to handle this system call. > > Otherwise, I think I'm missing something. > > Hmm, looking again at the patch, I'm pretty much confused. :-/ > > gdb_test continue \ > -"Continuing\\..*system 1:.*OK$stop_msg" \ > -"System says shell is available" > +gdb_test_no_output "set debug remote 1" > +set msg "System says shell is not available" > +gdb_test_multiple "continue" $msg { > + -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { > ^^^^^^^ > > What's this "Fsystem" supposed to be matching here? At this point > "set remote system-call-allowed 1" has NOT been issued yet, so > how come we expect to see that? I am so sorry that I didn't explain this patch very clear. The test before "set remote system-call-allowed 1" is to test the function that introduced in https://sourceware.org/gdb/current/onlinedocs/gdb/system.html "Due to security concerns, the system call is by default refused by GDB. The user has to allow this call explicitly with the set remote system-call-allowed 1 command." So 3 part of this test is for: gdb_test_no_output "set debug remote 1" # Open the switch to check if this target use file io to handle this system or not. set msg "System says shell is not available" gdb_test_multiple "continue" $msg { -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { pass $msg } Target uses file io handle system and it gets OK. Then the test pass. -re ".*Fsystem.*$gdb_prompt $" { fail $msg } Target uses file io handle system and it doesn't OK. Then the test fail. -re "$gdb_prompt $" { unsupported $msg } Target doesn't use file io. The test is not support. } Thanks, Hui > > + pass $msg > + } > + -re ".*Fsystem.*$gdb_prompt $" { > + fail $msg > + } > + -re "$gdb_prompt $" { > + unsupported $msg > + } > +} > +gdb_test_no_output "set debug remote 0" >
On 06/08/2014 11:05 AM, Hui Zhu wrote: > On 06/05/14 00:29, Pedro Alves wrote: >> On 06/04/2014 07:44 AM, Hui Zhu wrote: >>> On 05/29/14 19:12, Pedro Alves wrote: >>>> On 05/13/2014 04:27 AM, Hui Zhu wrote: >>>>> This patch is update version according to the discussion in >>>>> https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. >>>>> If test get the target doesn't support fileio system according to the >>>>> remote log. It will set this test as "unsupported". >>>>> >>>>> Before I made this patch, I want add a check before all of tests in this >>>>> file. But I found that the target maybe support one call but not others. >>>>> For example: my target support Fwrite, Fopen and so on. But not >>>>> Fgettimeofday. >>>>> And it doesn't support Fsystem NULL but it support Fsystem not NULL. >>>> >>>> So IIUC, the test will still have system (NULL) FAIL on your >>>> target, right? >>> >>> It will use fileio if argument of Fsystem is not NULL. >>> But will not use fileio if its argument is NULL. >> >> I mean, you still get one FAIL when you run the test >> against your target, even with your patch applied. > > No. It will not because the fail just happen when the target use remote > file io to handle this system call. I think I understand now. So in this test: ~~~ gdb_test_no_output "set remote system-call-allowed 1" gdb_test continue \ "Continuing\\..*system 2:.*OK$stop_msg" \ "System says shell is available" ~~~ your target is not sending an Fsystem packet at all, and it happens that you target's "system" implementation returns != 0, indicating that there's a shell on the target. Correct? Odd target behavior... Why would this be desirable instead of a target bug? But I digress. >> Otherwise, I think I'm missing something. >> >> Hmm, looking again at the patch, I'm pretty much confused. :-/ ... >> What's this "Fsystem" supposed to be matching here? At this point >> "set remote system-call-allowed 1" has NOT been issued yet, so >> how come we expect to see that? ... > set msg "System says shell is not available" > gdb_test_multiple "continue" $msg { > -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { > pass $msg > } > > Target uses file io handle system and it gets OK. > Then the test pass. Ah, I see now. I managed to get confused and assume that Fsystem was GDB's reply, instead of the request the target sends... Thanks,
On 06/09/14 17:29, Pedro Alves wrote: > On 06/08/2014 11:05 AM, Hui Zhu wrote: >> On 06/05/14 00:29, Pedro Alves wrote: >>> On 06/04/2014 07:44 AM, Hui Zhu wrote: >>>> On 05/29/14 19:12, Pedro Alves wrote: >>>>> On 05/13/2014 04:27 AM, Hui Zhu wrote: >>>>>> This patch is update version according to the discussion in >>>>>> https://www.sourceware.org/ml/gdb-patches/2009-11/msg00090.html. >>>>>> If test get the target doesn't support fileio system according to the >>>>>> remote log. It will set this test as "unsupported". >>>>>> >>>>>> Before I made this patch, I want add a check before all of tests in this >>>>>> file. But I found that the target maybe support one call but not others. >>>>>> For example: my target support Fwrite, Fopen and so on. But not >>>>>> Fgettimeofday. >>>>>> And it doesn't support Fsystem NULL but it support Fsystem not NULL. >>>>> >>>>> So IIUC, the test will still have system (NULL) FAIL on your >>>>> target, right? >>>> >>>> It will use fileio if argument of Fsystem is not NULL. >>>> But will not use fileio if its argument is NULL. >>> >>> I mean, you still get one FAIL when you run the test >>> against your target, even with your patch applied. >> >> No. It will not because the fail just happen when the target use remote >> file io to handle this system call. > > I think I understand now. So in this test: > > ~~~ > gdb_test_no_output "set remote system-call-allowed 1" > > gdb_test continue \ > "Continuing\\..*system 2:.*OK$stop_msg" \ > "System says shell is available" > ~~~ > > your target is not sending an Fsystem packet at all, > and it happens that you target's "system" implementation > returns != 0, indicating that there's a shell on the target. > Correct? > > Odd target behavior... Why would this be desirable instead of > a target bug? But I digress. I checked all the gdbrsp packages with the target of this test. It will not use fileio if the argument of system is NULL. It will use fileio if the argument is not NULL. I think idea of target's designer is that system NULL can be handled in target very well. So it will not use fileio in this moment. I have two ideas on this test: 1. What about I add another test the will do a system(!NULL) before "set remote system-call-allowed 1"? 2. Other gdb_test of this test will meet the same issue that target doesn't support fileio with the system call but just get a pass. What about I change other gdb_test to the test like what I didn't in current patch? > >>> Otherwise, I think I'm missing something. >>> >>> Hmm, looking again at the patch, I'm pretty much confused. :-/ > > ... > >>> What's this "Fsystem" supposed to be matching here? At this point >>> "set remote system-call-allowed 1" has NOT been issued yet, so >>> how come we expect to see that? > > ... > >> set msg "System says shell is not available" >> gdb_test_multiple "continue" $msg { >> -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { >> pass $msg >> } >> >> Target uses file io handle system and it gets OK. >> Then the test pass. > > Ah, I see now. I managed to get confused and assume that > Fsystem was GDB's reply, instead of the request the target > sends... Yes, and GDB doesn't have a switch or something to control except "remote system-call-allowed". Then gdbserver can attack the file system of GDB if it want. > > Thanks, >
--- a/gdb/testsuite/gdb.base/fileio.c +++ b/gdb/testsuite/gdb.base/fileio.c @@ -55,7 +55,11 @@ time(time_t *t); Not applicable. system (const char * string); -1) Invalid string/command. - returns 127. */ +1) See if shell available - returns 0 +2) See if shell available - returns !0 +3) Execute simple shell command - returns 0 +4) Invalid string/command. - returns 127. */ + static const char *strerrno (int err); /* Note that OUTDIR is defined by the test suite. */ @@ -375,21 +379,26 @@ test_system () */ int ret; - /* Test for shell */ + /* Test for shell (testsuite should have it disabled). */ ret = system (NULL); - printf ("system 1: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); + printf ("system 1: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); + stop (); + /* Test for shell again (the testsuite will have enabled it now). */ + ret = system (NULL); + printf ("system 2: ret = %d %s\n", ret, ret != 0 ? "OK" : ""); stop (); /* This test prepares the directory for test_rename() */ sprintf (sys, "mkdir -p %s/%s %s/%s", OUTDIR, TESTSUBDIR, OUTDIR, TESTDIR2); ret = system (sys); if (ret == 127) - printf ("system 2: ret = %d /bin/sh unavailable???\n", ret); + printf ("system 3: ret = %d /bin/sh unavailable???\n", ret); else - printf ("system 2: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); + printf ("system 3: ret = %d %s\n", ret, ret == 0 ? "OK" : ""); stop (); /* Invalid command (just guessing ;-) ) */ ret = system ("wrtzlpfrmpft"); - printf ("system 3: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? "OK" : ""); + printf ("system 4: ret = %d %s\n", ret, + WEXITSTATUS (ret) == 127 ? "OK" : ""); stop (); } --- a/gdb/testsuite/gdb.base/fileio.exp +++ b/gdb/testsuite/gdb.base/fileio.exp @@ -180,19 +180,34 @@ gdb_test continue \ "Continuing\\..*isatty 5:.*OK$stop_msg" \ "Isatty (open file)" -gdb_test continue \ -"Continuing\\..*system 1:.*OK$stop_msg" \ -"System says shell is available" +gdb_test_no_output "set debug remote 1" +set msg "System says shell is not available" +gdb_test_multiple "continue" $msg { + -re "Continuing\\..*Fsystem.*system 1:.*OK$stop_msg\r\n$gdb_prompt $" { + pass $msg + } + -re ".*Fsystem.*$gdb_prompt $" { + fail $msg + } + -re ".*$gdb_prompt $" { + unsupported $msg + } +} +gdb_test_no_output "set debug remote 0" gdb_test_no_output "set remote system-call-allowed 1" gdb_test continue \ "Continuing\\..*system 2:.*OK$stop_msg" \ +"System says shell is available" + +gdb_test continue \ +"Continuing\\..*system 3:.*OK$stop_msg" \ "System(3) call" # Is this ok? POSIX says system returns a waitpid status? gdb_test continue \ -"Continuing\\..*system 3:.*OK$stop_msg" \ +"Continuing\\..*system 4:.*OK$stop_msg" \ "System with invalid command returns 127" gdb_test continue \