Re: [patch] patch for review of PR gdb/10462

Re: [patch] patch for review of PR gdb/10462

[mbilal]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On 04/17/2013 Pedro Alves wrote:

 >BTW, is there a rationale for putting this in default.exp, which
 >is about default actions of commands?  I think setshow.exp might make
 >more sense

fixed.

 >The uniqueness issue is still here:

fixed.

Thanks Pedro for review.

Find attached patch.
Ok?

Thanks,
-Bilal

[-- Attachment #2: PR10462.diff --]
[-- Type: text/x-patch, Size: 2586 bytes --]

Index: cli/cli-decode.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-decode.c,v
retrieving revision 1.118
diff -u -p -r1.118 cli-decode.c
--- cli/cli-decode.c	10 Apr 2013 15:11:12 -0000	1.118
+++ cli/cli-decode.c	17 Apr 2013 12:16:37 -0000
@@ -1556,6 +1556,9 @@ lookup_cmd (const char **line, struct cm
     }
   else
     {
+      if (c->type == set_cmd && **line != '\0' && !isspace (**line))
+        error (_("Argument must be preceded by space."));
+
       /* We've got something.  It may still not be what the caller
          wants (if this command *needs* a subcommand).  */
       while (**line == ' ' || **line == '\t')
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15428
diff -u -p -r1.15428 ChangeLog
--- ChangeLog	17 Apr 2013 01:02:01 -0000	1.15428
+++ ChangeLog	17 Apr 2013 08:13:56 -0000
@@ -1,3 +1,9 @@
+2013-04-17  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* cli/cli-decode.c (lookup_command): Show an error if there is no space 
+	before argument.
+
 2013-04-17  Yao Qi  <yao@codesourcery.com>
 
 	* top.c (print_gdb_configuration): Print configure-time
Index: gdb.base/setshow.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/setshow.exp,v
retrieving revision 1.29
diff -u -p -r1.29 setshow.exp
--- gdb.base/setshow.exp	10 Apr 2013 15:11:12 -0000	1.29
+++ gdb.base/setshow.exp	17 Apr 2013 14:13:32 -0000
@@ -259,3 +259,10 @@ gdb_test "show verbose" "Verbose printin
 gdb_test_no_output "set verbose off" "set verbose off" 
 #test show verbose off
 gdb_test "show verbose" "Verbosity is off..*" "show verbose (off)" 
+#test argument must be preceded by space
+foreach x {"history file" "solib-search-path" "data-directory"} {
+    foreach y {"/home/" "." "~/home" "=home"} {
+        gdb_test "set $x$y" "Argument must be preceded by space." \
+            "$x is not set to $y"
+    }
+}
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.3628
diff -u -p -r1.3628 ChangeLog
--- ChangeLog	15 Apr 2013 20:37:36 -0000	1.3628
+++ ChangeLog	17 Apr 2013 14:14:53 -0000
@@ -1,3 +1,8 @@
+2013-04-17  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* gdb.base/setshow.exp: Add test case.
+
 2013-04-15  Siva Chandra Reddy  <sivachandra@google.com>
 
 	Add option to link testcases with Pthreads library when

Re: [patch] patch for review of PR gdb/10462

[Pedro Alves]

On 04/17/2013 03:52 PM, mbilal wrote:

> Ok?

This version looks good to me.

-- 
Pedro Alves

Re: [patch] patch for review of PR gdb/10462

[mbilal]

Checked in http://sourceware.org/ml/gdb-cvs/2013-04/msg00227.html

Thanks,
-Bilal

Re: [patch] patch for review of PR gdb/10462

[Pedro Alves]

On 04/17/2013 01:28 PM, mbilal wrote:
> --- gdb.base/default.exp	15 Apr 2013 18:09:02 -0000	1.64
> +++ gdb.base/default.exp	17 Apr 2013 12:18:38 -0000

BTW, is there a rationale for putting this in default.exp, which
is about default actions of commands?  I think setshow.exp might make
more sense.

> +#test argument must be preceded by space
> +foreach x {"history file" "solib-search-path" "data-directory"} {
> +    foreach y {"/home/" "." "~/home" "=home"} {
> +        gdb_test "set $x$y" "Argument must be preceded by space." \
> +            "check space before argument"
> +    }

The uniqueness issue is still here:

 http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Is there a reason

       gdb_test_no_output "set $x $y" "set $x $y"

was removed in this version?

-- 
Pedro Alves

Re: [patch] patch for review of PR gdb/10462

[mbilal]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Wednesday, April 17, 2013 2:40 PM Abid, Hafiz wrote:

 >I think you can break this line so that it does not go over 80 columns.

fixed.

On Wednesday, April 17, 2013 4:02 PM Pedro Alves wrote:


 >How about putting the 'line' tests together?

fixed.

 >I think it'd be good to test the reported case of using = instead of 
space too.

fixed.


find attached updated patch.


Thanks,
-Bilal

[-- Attachment #2: PR10462.diff --]
[-- Type: text/x-patch, Size: 2568 bytes --]

Index: cli/cli-decode.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-decode.c,v
retrieving revision 1.118
diff -u -p -r1.118 cli-decode.c
--- cli/cli-decode.c	10 Apr 2013 15:11:12 -0000	1.118
+++ cli/cli-decode.c	17 Apr 2013 12:16:37 -0000
@@ -1556,6 +1556,9 @@ lookup_cmd (const char **line, struct cm
     }
   else
     {
+      if (c->type == set_cmd && **line != '\0' && !isspace (**line))
+        error (_("Argument must be preceded by space."));
+
       /* We've got something.  It may still not be what the caller
          wants (if this command *needs* a subcommand).  */
       while (**line == ' ' || **line == '\t')
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15428
diff -u -p -r1.15428 ChangeLog
--- ChangeLog	17 Apr 2013 01:02:01 -0000	1.15428
+++ ChangeLog	17 Apr 2013 08:13:56 -0000
@@ -1,3 +1,9 @@
+2013-04-17  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* cli/cli-decode.c (lookup_command): Show an error if there is no space 
+	before argument.
+
 2013-04-17  Yao Qi  <yao@codesourcery.com>
 
 	* top.c (print_gdb_configuration): Print configure-time
Index: gdb.base/default.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/default.exp,v
retrieving revision 1.64
diff -u -p -r1.64 default.exp
--- gdb.base/default.exp	15 Apr 2013 18:09:02 -0000	1.64
+++ gdb.base/default.exp	17 Apr 2013 12:18:38 -0000
@@ -847,6 +847,13 @@ gdb_test "whatis" "The history is empty.
 gdb_test "where" "No stack." "where"
 #test x
 gdb_test "x" "Argument required .starting display address.*" "x"
+#test argument must be preceded by space
+foreach x {"history file" "solib-search-path" "data-directory"} {
+    foreach y {"/home/" "." "~/home" "=home"} {
+        gdb_test "set $x$y" "Argument must be preceded by space." \
+            "check space before argument"
+    }
+}
 
 gdb_exit
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.3628
diff -u -p -r1.3628 ChangeLog
--- ChangeLog	15 Apr 2013 20:37:36 -0000	1.3628
+++ ChangeLog	17 Apr 2013 08:23:11 -0000
@@ -1,3 +1,8 @@
+2013-04-17  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* gdb.base/default.exp: Add test case.
+
 2013-04-15  Siva Chandra Reddy  <sivachandra@google.com>
 
 	Add option to link testcases with Pthreads library when

Re: [patch] patch for review of PR gdb/10462

[Pedro Alves]

On 04/17/2013 10:14 AM, mbilal wrote:
> +      if (**line != '\0' && c->type == set_cmd && !isspace (**line))
> +        error (_("Argument must be preceded by space."));

How about putting the 'line' tests together?

      if (c->type == set_cmd && **line != '\0' && !isspace (**line))
        error (_("Argument must be preceded by space."));

As is looks a bit odd to me.

> +#test argument must be preceded by space
> +foreach x {"history file" "solib-search-path" "data-directory"} {
> +    foreach y {"/home/" "." "~/home"} {

I think it'd be good to test the reported case of using = instead of space too.

> +        gdb_test "set $x$y" "Argument must be preceded by space." "check space before argument"

http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

> +        gdb_test_no_output "set $x $y" "set $x $y"
> +    }
> +}

-- 
Pedro Alves

Re: [patch] patch for review of PR gdb/10462

[Abid, Hafiz]

> +        gdb_test "set $x$y" "Argument must be preceded by space."  
> "check space before argument"
I think you can break this line so that it does not go over 80 columns.

On 17/04/13 10:14:38, mbilal wrote:
> Hi,
> 
> Please find attached updated patch.
> 
> And
> Thanks for Hafiz Abid, Tom Tromey  and Andreas Schwab for review.
> 
> 
> Is that Ok?
> 
> Thanks,
> -Bilal

Re: [patch] patch for review of PR gdb/10462

[mbilal]

[-- Attachment #1: Type: text/plain, Size: 142 bytes --]

Hi,

Please find attached updated patch.

And
Thanks for Hafiz Abid, Tom Tromey  and Andreas Schwab for review.


Is that Ok?

Thanks,
-Bilal

[-- Attachment #2: PR10462.diff --]
[-- Type: text/x-patch, Size: 2597 bytes --]

Index: cli/cli-decode.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-decode.c,v
retrieving revision 1.118
diff -u -p -r1.118 cli-decode.c
--- cli/cli-decode.c	10 Apr 2013 15:11:12 -0000	1.118
+++ cli/cli-decode.c	17 Apr 2013 08:56:13 -0000
@@ -1556,6 +1556,9 @@ lookup_cmd (const char **line, struct cm
     }
   else
     {
+      if (**line != '\0' && c->type == set_cmd && !isspace (**line))
+        error (_("Argument must be preceded by space."));
+
       /* We've got something.  It may still not be what the caller
          wants (if this command *needs* a subcommand).  */
       while (**line == ' ' || **line == '\t')
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15428
diff -u -p -r1.15428 ChangeLog
--- ChangeLog	17 Apr 2013 01:02:01 -0000	1.15428
+++ ChangeLog	17 Apr 2013 08:13:56 -0000
@@ -1,3 +1,9 @@
+2013-04-17  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* cli/cli-decode.c (lookup_command): Show an error if there is no space 
+	before argument.
+
 2013-04-17  Yao Qi  <yao@codesourcery.com>
 
 	* top.c (print_gdb_configuration): Print configure-time
Index: gdb.base/default.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/default.exp,v
retrieving revision 1.64
diff -u -p -r1.64 default.exp
--- gdb.base/default.exp	15 Apr 2013 18:09:02 -0000	1.64
+++ gdb.base/default.exp	17 Apr 2013 09:04:13 -0000
@@ -847,6 +847,13 @@ gdb_test "whatis" "The history is empty.
 gdb_test "where" "No stack." "where"
 #test x
 gdb_test "x" "Argument required .starting display address.*" "x"
+#test argument must be preceded by space
+foreach x {"history file" "solib-search-path" "data-directory"} {
+    foreach y {"/home/" "." "~/home"} {
+        gdb_test "set $x$y" "Argument must be preceded by space." "check space before argument"
+        gdb_test_no_output "set $x $y" "set $x $y"
+    }
+}
 
 gdb_exit
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.3628
diff -u -p -r1.3628 ChangeLog
--- ChangeLog	15 Apr 2013 20:37:36 -0000	1.3628
+++ ChangeLog	17 Apr 2013 08:23:11 -0000
@@ -1,3 +1,8 @@
+2013-04-17  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* gdb.base/default.exp: Add test case.
+
 2013-04-15  Siva Chandra Reddy  <sivachandra@google.com>
 
 	Add option to link testcases with Pthreads library when

Re: [patch] patch for review of PR gdb/10462

[Andreas Schwab]

mbilal <mbilal@codesourcery.com> writes:

> +      if (strlen(*line) != 0 && c->type == set_cmd && **line != ' ' )
             **line != '\0'

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] patch for review of PR gdb/10462

[Tom Tromey]

>>>>> ">" == mbilal  <mbilal@codesourcery.com> writes:

>> please find attached,  proposed patch for  PR gdb/10462
>> that is, argument must be  preceded by  space.

>> +      if (strlen(*line) != 0 && c->type == set_cmd && **line != ' ' )

Space after 'strlen' and no space before the ')'.
Also, it seems like isspace would be better here.

Finally, this could use a test case.
You can just add one to some relevant .exp file.

Tom

Re: [patch] patch for review of PR gdb/10462

[Abid, Hafiz]

Hi Bilal,
Few small nits.

> +	PR gdb/10462
> +	* cli/cli-decode.c (lookup_command): Checked a space before  
> argument.
s/Checked/Check/
Also I think this can be a bit more descriptive like "Show an error if  
there is no space before argument."

> +      if (strlen(*line) != 0 && c->type == set_cmd && **line != ' ' )
A space here      ^.


Regards,
Abid
From gdb-patches-return-100552-listarch-gdb-patches=sources.redhat.com@sourceware.org Tue Apr 16 15:22:29 2013
Return-Path: <gdb-patches-return-100552-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Delivered-To: listarch-gdb-patches@sources.redhat.com
Received: (qmail 18416 invoked by alias); 16 Apr 2013 15:22:28 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.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 18407 invoked by uid 89); 16 Apr 2013 15:22:28 -0000
X-Spam-SWARE-Status: No, score=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1
Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28)    by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 16 Apr 2013 15:22:28 +0000
Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])	by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3GFMQC6005768	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits%6 verify=OK)	for <gdb-patches@sourceware.org>; Tue, 16 Apr 2013 11:22:26 -0400
Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11])	by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r3GFMPT3027438;	Tue, 16 Apr 2013 11:22:26 -0400
Message-ID: <516D6CB1.8070201@redhat.com>
Date: Tue, 16 Apr 2013 15:45:00 -0000
From: Pedro Alves <palves@redhat.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4
MIME-Version: 1.0
To: Tom Tromey <tromey@redhat.com>
CC: gdb-patches@sourceware.org
Subject: Re: RFC: fix PR gdb/15338
References: <87obdfbn8n.fsf@fleche.redhat.com> <516C5988.8070507@redhat.com> <87haj7bmgd.fsf@fleche.redhat.com> <8738uqbjcs.fsf@fleche.redhat.com>
In-Reply-To: <8738uqbjcs.fsf@fleche.redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
X-SW-Source: 2013-04/txt/msg00478.txt.bz2
Content-length: 263

On 04/16/2013 04:12 PM, Tom Tromey wrote:
> Tom> There isn't one, but I can ask the reporter for it.
> Tom> I didn't ask earlier since I thought the problem was obvious from the
> Tom> assertion failure.
>
> There's one in the PR now.

Thanks.

--
Pedro Alves

[patch] patch for review of PR gdb/10462

[mbilal]

[-- Attachment #1: Type: text/plain, Size: 126 bytes --]

Hi,

please find attached,  proposed patch for  PR gdb/10462
that is, argument must be  preceded by  space.



Thanks,
-Bilal

[-- Attachment #2: Pr10462.diff --]
[-- Type: text/x-patch, Size: 1237 bytes --]

Index: cli/cli-decode.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-decode.c,v
retrieving revision 1.118
diff -u -p -r1.118 cli-decode.c
--- cli/cli-decode.c	10 Apr 2013 15:11:12 -0000	1.118
+++ cli/cli-decode.c	16 Apr 2013 11:49:40 -0000
@@ -1556,6 +1556,9 @@ lookup_cmd (const char **line, struct cm
     }
   else
     {
+      if (strlen(*line) != 0 && c->type == set_cmd && **line != ' ' )
+     error (_("Argument must be preceded by space."));
+
       /* We've got something.  It may still not be what the caller
          wants (if this command *needs* a subcommand).  */
       while (**line == ' ' || **line == '\t')

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.15421
diff -u -p -r1.15421 ChangeLog
--- ChangeLog	15 Apr 2013 23:53:37 -0000	1.15421
+++ ChangeLog	16 Apr 2013 11:45:38 -0000
@@ -1,3 +1,8 @@
+2013-04-16  Muhammad Bilal  <mbilal@codesourcery.com>
+
+	PR gdb/10462
+	* cli/cli-decode.c (lookup_command): Checked a space before argument.
+	
 2013-04-15  Doug Evans  <dje@google.com>
 
 	* dwarf2read.c (struct dwo_file): Replace member "cus" with "cu".