Scope Checking Patch

Scope Checking Patch

[Rob Quill]

Hi all,

This is the first patch I have ever submitted to an open source
project, so I'm a little bit unsure of the process. The patch adds the
ability to check if a variable is in scope, as descibed here:

http://sourceware.org/ml/gdb/2006-11/msg00149.html

I have attached the diffs for the two files I've changed. However, I
am seeing some regressions against the current cvs, which I can't
understand, so I was wondering if a) the regressions happen for anyone
esle? (which presumably it does), b) if anyone could offer any
suggestions as to the cause, and c) give thier opinions on the patch.

The patch allows the scope of constants, variables and variables in
classes/structures, by using $in_scope(variable_name) as an
expression, with value 1 if variable_name is in scope and 0 if it is
not.

Any help and thoughts you can offer is much appreciated.

Thanks,

Rob

Re: Scope Checking Patch

[Rob Quill]

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

On 10/06/07, Rob Quill <rob.quill@gmail.com> wrote:
> Hi all,
>
> This is the first patch I have ever submitted to an open source
> project, so I'm a little bit unsure of the process. The patch adds the
> ability to check if a variable is in scope, as descibed here:
>
> http://sourceware.org/ml/gdb/2006-11/msg00149.html
>
> I have attached the diffs for the two files I've changed. However, I
> am seeing some regressions against the current cvs, which I can't
> understand, so I was wondering if a) the regressions happen for anyone
> esle? (which presumably it does), b) if anyone could offer any
> suggestions as to the cause, and c) give thier opinions on the patch.
>
> The patch allows the scope of constants, variables and variables in
> classes/structures, by using $in_scope(variable_name) as an
> expression, with value 1 if variable_name is in scope and 0 if it is
> not.
>
> Any help and thoughts you can offer is much appreciated.

...Diffs attached, sorry.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: c-exp.patch --]
[-- Type: text/x-patch; name="c-exp.patch", Size: 2625 bytes --]

103a104,106
> /* Global variable denoting whether we are only interested in scope, not value */
> int check_scope = 0;
> 
203a207,208
> /* $in_scope opperator */
> %left IN_SCOPE
246a252,256
> exp	:	IN_SCOPE
> 			{ check_scope = 1; }
> 		'(' exp ')'
> 	;
> 
586c596,602
< 			    error ("No symbol \"%s\" in specified context.",
---
> 			  {
> 			    /* Case for scope checking. If scope is being checked and
> 				   the symbol is not in scope, return an expresison of
> 				   value 0. */
> 			    if(check_scope == 0)
> 			    {
> 			       error ("No symbol \"%s\" in specified context.",
587a604,614
> 			    }
> 			    else
> 			    {
> 				   YYSTYPE val;
> 			 	   parse_number ("0", 1, 0, &val);
> 			  	   write_exp_elt_opcode (OP_LONG);
> 			  	   write_exp_elt_type (val.typed_val_int.type);
> 			  	   write_exp_elt_longcst ((LONGEST)val.typed_val_int.val);
> 			  	   write_exp_elt_opcode (OP_LONG);
> 			    }
> 			  }
666,667c693,708
< 			      error ("No symbol \"%s\" in current context.", name);
< 			}
---
> 			    {
> 			      if(check_scope == 0)
> 			      {
> 			    	error ("No symbol \"%s\" in current context.", name);
> 			      }
> 			      else
> 			      {
>                      YYSTYPE val;
>                      parse_number ("0", 1, 0, &val);
>                      write_exp_elt_opcode (OP_LONG);
>                      write_exp_elt_type (val.typed_val_int.type);
>                      write_exp_elt_longcst ((LONGEST)val.typed_val_int.val);
>                      write_exp_elt_opcode (OP_LONG);
> 			      }
> 			    }
> }
721,722c762,774
< 				error ("No symbol \"%s\" in current context.",
< 				       copy_name ($1.stoken));
---
> 			      {
> 			        if(check_scope == 0)
> 				       error ("No symbol \"%s\" in current context.", copy_name ($1.stoken));
> 			        else
> 			        {
> 				       YYSTYPE val;
> 			 	       parse_number ("0", 1, 0, &val);
> 			  	       write_exp_elt_opcode (OP_LONG);
> 			  	       write_exp_elt_type (val.typed_val_int.type);
> 			  	       write_exp_elt_longcst ((LONGEST)val.typed_val_int.val);
> 			  	       write_exp_elt_opcode (OP_LONG);
> 				    }
> 			      }
1260a1313,1317
> static const struct token tokentab9[] =
>   {
>     {"$in_scope", IN_SCOPE, BINOP_END}
>   };
> 
1322a1380,1388
>   /* Code for recognising the $in_scope token. */
>   /* See if it is a special token of length 9.  */
>   for (i = 0; i < sizeof tokentab9 / sizeof tokentab9[0]; i++)
>     if (strncmp (tokstart, tokentab9[i].operator, 9) == 0)
>       {
> 	lexptr += 9;
> 	yylval.opcode = tokentab9[i].opcode;
> 	return tokentab9[i].token;
>       }

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: eval.patch --]
[-- Type: text/x-patch; name="eval.patch", Size: 1294 bytes --]

48a49,51
> /* Variable denoting is scope is being checked */
> extern int check_scope;
> 
167c170,179
<   return evaluate_subexp (NULL_TYPE, exp, &pc, EVAL_NORMAL);
---
> 
>   /* Modifications so that if we are checking scope we can 
>      reset the value of the variable to 0, so we don't check 
>      scope when we don't want to */
>   struct value *val = evaluate_subexp (NULL_TYPE, exp, &pc, EVAL_NORMAL);
>   
>   if(check_scope == 1)
>     check_scope = 0;
> 
>   return val;
474a487,494
> 	/* Special case, if we are checking scope and a variable is 
>        in scope, return a expression of value 1. */
> 	if(check_scope == 0)
> 	  return value_of_variable (exp->elts[pc + 2].symbol,
> 				    exp->elts[pc + 1].block);
> 	else
> 	  return value_from_longest (builtin_type_int, (LONGEST) 1);
> 
1336c1356,1368
<       arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
---
> 
>       /* Temporalily disable scope checking while we evaluate
>          the subexpression, so that the correct type of value
>          is returned */
>       if(check_scope == 1)
>        {
> 	     check_scope = 0;
>          arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
>          check_scope = 1;
>        }
>       else
>          arg1 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
> 

Re: Scope Checking Patch

[Jim Blandy]

"Rob Quill" <rob.quill@gmail.com> writes:
> ...Diffs attached, sorry.

OMG, you implemented it!  I'd better review it!  :)

Once you've got -up or -cp patches, I'll take a look.

Re: Scope Checking Patch

[Rob Quill]

On 11/06/07, Jim Blandy <jimb@codesourcery.com> wrote:
>
> "Rob Quill" <rob.quill@gmail.com> writes:
> > ...Diffs attached, sorry.
>
> OMG, you implemented it!  I'd better review it!  :)

Thanks :) It has been done for a good while against 6.5, but I am just
trying to get it working properly against head. I'll post the patches
either later today or tomorrow, depending if I find any problems.

Rob

> Once you've got -up or -cp patches, I'll take a look.
>

Re: Scope Checking Patch

[Markus Deuling]

Hi Rob,

I haven't gone through your patch, but I have a suggestion. If I do a patch for GDB I do it like that:

a) Checkout current head
b) Create a copy of that directory e.g. gdb.new
c) Do my changes in gdb.new
d) Create patch by: diff -urN gdb/ gdb.new/ > diff-file

Then you the two diff files in one file and its better readable. Also its easier to apply just one file instead of two.

Regards,
Markus

Rob Quill wrote:
> Hi all,
> 
> This is the first patch I have ever submitted to an open source
> project, so I'm a little bit unsure of the process. The patch adds the
> ability to check if a variable is in scope, as descibed here:
> 
> http://sourceware.org/ml/gdb/2006-11/msg00149.html
> 
> I have attached the diffs for the two files I've changed. However, I
> am seeing some regressions against the current cvs, which I can't
> understand, so I was wondering if a) the regressions happen for anyone
> esle? (which presumably it does), b) if anyone could offer any
> suggestions as to the cause, and c) give thier opinions on the patch.
> 
> The patch allows the scope of constants, variables and variables in
> classes/structures, by using $in_scope(variable_name) as an
> expression, with value 1 if variable_name is in scope and 0 if it is
> not.
> 
> Any help and thoughts you can offer is much appreciated.
> 
> Thanks,
> 
> Rob
> 


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: Scope Checking Patch

[Rob Quill]

On 11/06/07, Markus Deuling <deuling@de.ibm.com> wrote:
> Hi Rob,
>
> I haven't gone through your patch, but I have a suggestion. If I do a patch for GDB I do it like that:
>
> a) Checkout current head
> b) Create a copy of that directory e.g. gdb.new
> c) Do my changes in gdb.new
> d) Create patch by: diff -urN gdb/ gdb.new/ > diff-file

Thanks. I tried this, but I ended up with a 214MB diff file. I'm not
sure why this is, as I did a make distclean on both copies, I think it
may be due to the testsuite results not having been cleaned or
something. Does anyone have any ideas what to do?

Thanks,

Rob

> Then you the two diff files in one file and its better readable. Also its easier to apply just one file instead of two.
>
> Regards,
> Markus
>
> Rob Quill wrote:
> > Hi all,
> >
> > This is the first patch I have ever submitted to an open source
> > project, so I'm a little bit unsure of the process. The patch adds the
> > ability to check if a variable is in scope, as descibed here:
> >
> > http://sourceware.org/ml/gdb/2006-11/msg00149.html
> >
> > I have attached the diffs for the two files I've changed. However, I
> > am seeing some regressions against the current cvs, which I can't
> > understand, so I was wondering if a) the regressions happen for anyone
> > esle? (which presumably it does), b) if anyone could offer any
> > suggestions as to the cause, and c) give thier opinions on the patch.
> >
> > The patch allows the scope of constants, variables and variables in
> > classes/structures, by using $in_scope(variable_name) as an
> > expression, with value 1 if variable_name is in scope and 0 if it is
> > not.
> >
> > Any help and thoughts you can offer is much appreciated.
> >
> > Thanks,
> >
> > Rob
> >
>
>
> --
> Markus Deuling
> GNU Toolchain for Linux on Cell BE
> deuling@de.ibm.com
>
>

Re: Scope Checking Patch

[Markus Deuling]

Rob Quill wrote:
> On 11/06/07, Markus Deuling <deuling@de.ibm.com> wrote:
>> Hi Rob,
>>
>> I haven't gone through your patch, but I have a suggestion. If I do a 
>> patch for GDB I do it like that:
>>
>> a) Checkout current head
>> b) Create a copy of that directory e.g. gdb.new
>> c) Do my changes in gdb.new
>> d) Create patch by: diff -urN gdb/ gdb.new/ > diff-file
> 
> Thanks. I tried this, but I ended up with a 214MB diff file. I'm not
> sure why this is, as I did a make distclean on both copies, I think it
> may be due to the testsuite results not having been cleaned or
> something. Does anyone have any ideas what to do?

Hm, I think I haven't pointed it out clearly. Lets assume following directory structure:

gdb-6.6/  <- extract gdb-6.6.tar.bz2
gdb.dev/  <- copy of gdb-6.6/
gdb.dev/build <- directory in which you configure & build gdb
gdb.dev/gdb <- source tree where you do your changes
gdb-6.6/gdb <- original gdb source tree

You do a patch now with following command:
diff -urN gdb-6.6/gdb/ gdb.dev/gdb > diff-file

What you should do is to compare ONLY the source tree in gdb/ directories.


To configure & build gdb:
cd gdb.dev/
mkdir build
cd build/
../configure --disable-werror
make

You should always build GDB in a directory separate from source directory.


-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com

Re: Scope Checking Patch

[Andrew STUBBS]

Rob Quill wrote:
> Thanks. I tried this, but I ended up with a 214MB diff file. I'm not
> sure why this is, as I did a make distclean on both copies, I think it
> may be due to the testsuite results not having been cleaned or
> something. Does anyone have any ideas what to do?

Don't build in your source tree. As you discovered it is really 
inconvenient.

Instead, put all the object files in a second directory alongside your 
src directory:

mkdir objdir
cd objdir
../src/configure
make

This way the sources are not polluted* and the diff will be much cleaner.

Also, like Markus said, always use context or unified diffs. Ordinary 
diff output isn't nearly so useful. I'd also suggest using the -p option 
- it adds a procedure name to each hunk - entirely cosmetic, but nice to
have.

Hope that helps,

Andrew

* There are some files, such as .info files, which get written back to 
the source tree.

Re: Scope Checking Patch

[Nick Roberts]

 > > Thanks. I tried this, but I ended up with a 214MB diff file. I'm not
 > > sure why this is, as I did a make distclean on both copies, I think it
 > > may be due to the testsuite results not having been cleaned or
 > > something. Does anyone have any ideas what to do?
 > 
 > Don't build in your source tree. As you discovered it is really 
 > inconvenient.

Or you can use "cvs diff gdb" which will just diff the files under CVS
control.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Scope Checking Patch

[Daniel Jacobowitz]

On Mon, Jun 11, 2007 at 11:59:16AM +0100, Andrew STUBBS wrote:
> Also, like Markus said, always use context or unified diffs. Ordinary diff 
> output isn't nearly so useful. I'd also suggest using the -p option - it adds a 
> procedure name to each hunk - entirely cosmetic, but nice to
> have.

Yes.  Rob, please use diff -up, and send patches to gdb-patches -
very little should be cross-posted to both lists.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery