Refactor tdep-i386.c to fix all -Wshadow warnings

Refactor tdep-i386.c to fix all -Wshadow warnings

[Goncalo Gomes]

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

The attached patch fixes all warnings in tdep-i386.c resulting from
enabling shadow warnings in gcc. As this is my first contribution to
GDB, I decided to keep it short to a single file to obtain feedback.

2012-08-05  Goncalo Gomes <goncalo@promisc.org>

        * Refactor tdep-i386.c to fix all -Wshadow warnings

Goncalo

[-- Attachment #2: enable-gcc-shadow-warnings-0001.patch --]
[-- Type: application/octet-stream, Size: 5004 bytes --]

diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 0c59a25..9c47a7b 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -145,9 +145,9 @@ int exceptions_state_mc_action_iter_1 (void);
 
 #define TRY_CATCH(EXCEPTION,MASK) \
      { \
-       EXCEPTIONS_SIGJMP_BUF *buf = \
+       EXCEPTIONS_SIGJMP_BUF *_trycatch_sjbuf = \
 	 exceptions_state_mc_init (&(EXCEPTION), (MASK)); \
-       EXCEPTIONS_SIGSETJMP (*buf); \
+       EXCEPTIONS_SIGSETJMP (*_trycatch_sjbuf); \
      } \
      while (exceptions_state_mc_action_iter ()) \
        while (exceptions_state_mc_action_iter_1 ())
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 84e9794..6fbc51f 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2336,7 +2336,7 @@ i386_16_byte_align_p (struct type *type)
 
 static CORE_ADDR
 i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
-		      struct value **args, int nargs, struct type *value_type,
+		      struct value **args, int nargs, struct type *valtype,
 		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
 		      struct regcache *regcache)
 {
@@ -2514,7 +2514,7 @@ i386_store_return_value (struct gdbarch *gdbarch, struct type *type,
 
   if (TYPE_CODE (type) == TYPE_CODE_FLT)
     {
-      ULONGEST fstat;
+      ULONGEST fpstat;
       gdb_byte buf[I386_MAX_REGISTER_SIZE];
 
       if (tdep->st0_regnum < 0)
@@ -2538,9 +2538,9 @@ i386_store_return_value (struct gdbarch *gdbarch, struct type *type,
          actual value doesn't really matter, but 7 is what a normal
          function return would end up with if the program started out
          with a freshly initialized FPU.  */
-      regcache_raw_read_unsigned (regcache, I387_FSTAT_REGNUM (tdep), &fstat);
-      fstat |= (7 << 11);
-      regcache_raw_write_unsigned (regcache, I387_FSTAT_REGNUM (tdep), fstat);
+      regcache_raw_read_unsigned (regcache, I387_FSTAT_REGNUM (tdep), &fpstat);
+      fpstat |= (7 << 11);
+      regcache_raw_write_unsigned (regcache, I387_FSTAT_REGNUM (tdep), fpstat);
 
       /* Mark %st(1) through %st(7) as empty.  Since we set the top of
          the floating-point register stack to 7, the appropriate value
@@ -2825,12 +2825,12 @@ i386_mmx_regnum_to_fp_regnum (struct regcache *regcache, int regnum)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (get_regcache_arch (regcache));
   int mmxreg, fpreg;
-  ULONGEST fstat;
+  ULONGEST fpstat;
   int tos;
 
   mmxreg = regnum - tdep->mm0_regnum;
-  regcache_raw_read_unsigned (regcache, I387_FSTAT_REGNUM (tdep), &fstat);
-  tos = (fstat >> 11) & 0x7;
+  regcache_raw_read_unsigned (regcache, I387_FSTAT_REGNUM (tdep), &fpstat);
+  tos = (fpstat >> 11) & 0x7;
   fpreg = (mmxreg + tos) % 8;
 
   return (I387_ST0_REGNUM (tdep) + fpreg);
@@ -3561,7 +3561,7 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		  const char *start;
 		  char *base;
 		  int len_base;
-		  char *index;
+		  char *idx;
 		  int len_index;
 		  struct stoken base_token, index_token;
 
@@ -3609,15 +3609,15 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		    ++s;
 
 		  len_index = s - start;
-		  index = alloca (len_index + 1);
-		  strncpy (index, start, len_index);
-		  index[len_index] = '\0';
+		  idx = alloca (len_index + 1);
+		  strncpy (idx, start, len_index);
+		  idx[len_index] = '\0';
 
 		  if (user_reg_map_name_to_regnum (gdbarch,
-						   index, len_index) == -1)
+						   idx, len_index) == -1)
 		    error (_("Invalid register name `%s' "
 			     "on expression `%s'."),
-			   index, p->saved_arg);
+			   idx, p->saved_arg);
 
 		  if (*s != ',' && *s != ')')
 		    break;
@@ -3662,7 +3662,7 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
 		    write_exp_elt_opcode (BINOP_ADD);
 
 		  write_exp_elt_opcode (OP_REGISTER);
-		  index_token.ptr = index;
+		  index_token.ptr = idx;
 		  index_token.length = len_index;
 		  write_exp_string (index_token);
 		  write_exp_elt_opcode (OP_REGISTER);
@@ -3970,7 +3970,7 @@ i386_record_lea_modrm_addr (struct i386_record_s *irp, uint64_t *addr)
       int havesib = 0;
       uint8_t scale = 0;
       uint8_t byte;
-      uint8_t index = 0;
+      uint8_t idx = 0;
       uint8_t base = irp->rm;
 
       if (base == 4)
@@ -3980,7 +3980,7 @@ i386_record_lea_modrm_addr (struct i386_record_s *irp, uint64_t *addr)
 	    return -1;
 	  irp->addr++;
 	  scale = (byte >> 6) & 3;
-	  index = ((byte >> 3) & 7) | irp->rex_x;
+	  idx = ((byte >> 3) & 7) | irp->rex_x;
 	  base = (byte & 7);
 	}
       base |= irp->rex_b;
@@ -4028,9 +4028,9 @@ i386_record_lea_modrm_addr (struct i386_record_s *irp, uint64_t *addr)
       else
         *addr = (uint32_t) (offset64 + *addr);
 
-      if (havesib && (index != 4 || scale != 0))
+      if (havesib && (idx != 4 || scale != 0))
 	{
-	  regcache_raw_read_unsigned (irp->regcache, irp->regmap[index],
+	  regcache_raw_read_unsigned (irp->regcache, irp->regmap[idx],
                                       &offset64);
 	  if (irp->aflag == 2)
 	    *addr += offset64 << scale;

[-- Attachment #3: enable-gcc-shadow-warnings-0001.changelog --]
[-- Type: application/octet-stream, Size: 110 bytes --]

2012-08-05  Goncalo Gomes <goncalo@promisc.org>

        * Refactor tdep-i386.c to fix all -Wshadow warnings

Re: Refactor tdep-i386.c to fix all -Wshadow warnings

[Sergio Durigan Junior]

On Sunday, August 05 2012, Goncalo Gomes wrote:

> The attached patch fixes all warnings in tdep-i386.c resulting from
> enabling shadow warnings in gcc. As this is my first contribution to
> GDB, I decided to keep it short to a single file to obtain feedback.

Thanks for your patch.  It is a cleanup and not a big patch, thus I
guess you won't need a copyright assignment to get it in.  However, if
you would like to start the process, drop me an e-mail offlist and I can
send you the form.

I have a few comments regarding formatting nits.

> 2012-08-05  Goncalo Gomes <goncalo@promisc.org>

Two spaces between your name and your e-mail address.

>
>         * Refactor tdep-i386.c to fix all -Wshadow warnings

You have to explicit mention each file, and each function/location being
modified.  Take a look at gdb/ChangeLog to see how it is done.  You also
need to indent the entries using a tab, and not spaces.  Something like:

2012-08-06  Goncalo Gomes  <goncalo@promisc.org>

	* exceptions.h (TRY_CATCH): Rename `buf' to `trycatch_sjbuf'.
	* i386-tdep.c (....)...

> diff --git a/gdb/exceptions.h b/gdb/exceptions.h
> index 0c59a25..9c47a7b 100644
> --- a/gdb/exceptions.h
> +++ b/gdb/exceptions.h
> @@ -145,9 +145,9 @@ int exceptions_state_mc_action_iter_1 (void);
>  
>  #define TRY_CATCH(EXCEPTION,MASK) \
>       { \
> -       EXCEPTIONS_SIGJMP_BUF *buf = \
> +       EXCEPTIONS_SIGJMP_BUF *_trycatch_sjbuf = \
>  	 exceptions_state_mc_init (&(EXCEPTION), (MASK)); \
> -       EXCEPTIONS_SIGSETJMP (*buf); \
> +       EXCEPTIONS_SIGSETJMP (*_trycatch_sjbuf); \
>       } \
>       while (exceptions_state_mc_action_iter ()) \
>         while (exceptions_state_mc_action_iter_1 ())

Is it possible to remove the leading `_' from `_trycatch_sjbuf'?  It
should be used only for internal variables inside some library which I
forgot the name...

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 84e9794..6fbc51f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c

> @@ -3561,7 +3561,7 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>  		  const char *start;
>  		  char *base;
>  		  int len_base;
> -		  char *index;
> +		  char *idx;
>  		  int len_index;

Please change this variable to `len_idx', since it refers to `idx' now.

Aside of that, I guess the patch is pretty trivial.  I am not a
maintainer, so you will still need an approval from a maintainer before
being able to check it in.  In fact, I guess you won't be able to check
it in for yourself because you still don't have an account on
sourceware, so probably someone else will commit it for you.

Thanks,

-- 
Sergio

Re: Refactor tdep-i386.c to fix all -Wshadow warnings

[Goncalo Gomes]

On Mon, Aug 6, 2012 at 11:46 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Sunday, August 05 2012, Goncalo Gomes wrote:
>
>> The attached patch fixes all warnings in tdep-i386.c resulting from
>> enabling shadow warnings in gcc. As this is my first contribution to
>> GDB, I decided to keep it short to a single file to obtain feedback.
>
(...)
> I have a few comments regarding formatting nits.
>
>> 2012-08-05  Goncalo Gomes <goncalo@promisc.org>
>
> Two spaces between your name and your e-mail address.
>
>>
>>         * Refactor tdep-i386.c to fix all -Wshadow warnings
>
> You have to explicit mention each file, and each function/location being
> modified.  Take a look at gdb/ChangeLog to see how it is done.  You also
> need to indent the entries using a tab, and not spaces.  Something like:
>
> 2012-08-06  Goncalo Gomes  <goncalo@promisc.org>
>
>         * exceptions.h (TRY_CATCH): Rename `buf' to `trycatch_sjbuf'.
>         * i386-tdep.c (....)...
>
>> diff --git a/gdb/exceptions.h b/gdb/exceptions.h
>> index 0c59a25..9c47a7b 100644
>> --- a/gdb/exceptions.h
>> +++ b/gdb/exceptions.h
>> @@ -145,9 +145,9 @@ int exceptions_state_mc_action_iter_1 (void);
>>
>>  #define TRY_CATCH(EXCEPTION,MASK) \
>>       { \
>> -       EXCEPTIONS_SIGJMP_BUF *buf = \
>> +       EXCEPTIONS_SIGJMP_BUF *_trycatch_sjbuf = \
>>        exceptions_state_mc_init (&(EXCEPTION), (MASK)); \
>> -       EXCEPTIONS_SIGSETJMP (*buf); \
>> +       EXCEPTIONS_SIGSETJMP (*_trycatch_sjbuf); \
>>       } \
>>       while (exceptions_state_mc_action_iter ()) \
>>         while (exceptions_state_mc_action_iter_1 ())
>
> Is it possible to remove the leading `_' from `_trycatch_sjbuf'?  It
> should be used only for internal variables inside some library which I
> forgot the name...
>
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index 84e9794..6fbc51f 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>
>> @@ -3561,7 +3561,7 @@ i386_stap_parse_special_token (struct gdbarch *gdbarch,
>>                 const char *start;
>>                 char *base;
>>                 int len_base;
>> -               char *index;
>> +               char *idx;
>>                 int len_index;
>
> Please change this variable to `len_idx', since it refers to `idx' now.
>
> Aside of that, I guess the patch is pretty trivial.  I am not a
> maintainer, so you will still need an approval from a maintainer before
> being able to check it in.  In fact, I guess you won't be able to check
> it in for yourself because you still don't have an account on
> sourceware, so probably someone else will commit it for you.

Thanks for reviewing. I have (hopefully) applied all your suggestions.

I'm going to re-send the revised patch and changelog in a follow-up
email with a new subject line.

Goncalo

Re: Refactor tdep-i386.c to fix all -Wshadow warnings

[Tom Tromey]

>>>>> "Goncalo" == Goncalo Gomes <goncalo@promisc.org> writes:

Goncalo> The attached patch fixes all warnings in tdep-i386.c resulting from
Goncalo> enabling shadow warnings in gcc. As this is my first contribution to
Goncalo> GDB, I decided to keep it short to a single file to obtain feedback.

Just a friendly word of warning -- this is arguably the worst project to
pick of all the things on the project page (though unfortunately there
is more than one stinker on there).  It's been attempted a couple of
times, had always lead to a lot of contention, etc.

That doesn't mean it can't be done, just that you ought to expect it to
be a pain.

Last time this came up, I think the conclusion was that we'd prefer it
if we could get warnings only for some kinds of shadowing, but not all
kinds.  There was a sense that warnings for shadowing of 'index' was not
very useful.  I remember some discussion of checking for a GCC change in
gdb's configure, but I don't remember the details any more.  They're in
the archives.

Tom

Re: Refactor tdep-i386.c to fix all -Wshadow warnings

[Goncalo Gomes]

On Mon, Aug 6, 2012 at 3:23 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Goncalo" == Goncalo Gomes <goncalo@promisc.org> writes:
>
> Goncalo> The attached patch fixes all warnings in tdep-i386.c resulting from
> Goncalo> enabling shadow warnings in gcc. As this is my first contribution to
> Goncalo> GDB, I decided to keep it short to a single file to obtain feedback.
>
> Just a friendly word of warning -- this is arguably the worst project to
> pick of all the things on the project page (though unfortunately there
> is more than one stinker on there).  It's been attempted a couple of
> times, had always lead to a lot of contention, etc.
>
> That doesn't mean it can't be done, just that you ought to expect it to
> be a pain.
>
> Last time this came up, I think the conclusion was that we'd prefer it
> if we could get warnings only for some kinds of shadowing, but not all
> kinds.  There was a sense that warnings for shadowing of 'index' was not
> very useful.  I remember some discussion of checking for a GCC change in
> gdb's configure, but I don't remember the details any more.  They're in
> the archives.

Thanks for this piece of advice!

I'll look into some bugs or re-consider a larger feature set going
forward (though, I still need to work out the copyright assignment
paperwork.)

-- 
Goncalo <goncalo@promisc.org>
http://promisc.org/

Re: Refactor tdep-i386.c to fix all -Wshadow warnings

[Sergio Durigan Junior]

On Monday, August 06 2012, Tom Tromey wrote:

>>>>>> "Goncalo" == Goncalo Gomes <goncalo@promisc.org> writes:
>
> Goncalo> The attached patch fixes all warnings in tdep-i386.c resulting from
> Goncalo> enabling shadow warnings in gcc. As this is my first contribution to
> Goncalo> GDB, I decided to keep it short to a single file to obtain feedback.
>
> Just a friendly word of warning -- this is arguably the worst project to
> pick of all the things on the project page (though unfortunately there
> is more than one stinker on there).  It's been attempted a couple of
> times, had always lead to a lot of contention, etc.
>
> That doesn't mean it can't be done, just that you ought to expect it to
> be a pain.
>
> Last time this came up, I think the conclusion was that we'd prefer it
> if we could get warnings only for some kinds of shadowing, but not all
> kinds.  There was a sense that warnings for shadowing of 'index' was not
> very useful.  I remember some discussion of checking for a GCC change in
> gdb's configure, but I don't remember the details any more.  They're in
> the archives.

Thank you for this warning.  I have updated the wiki page ProjectIdeas
to reflect this.

-- 
Sergio

Re: Refactor tdep-i386.c to fix all -Wshadow warnings

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Mon, 06 Aug 2012 08:23:11 -0600
> 
> >>>>> "Goncalo" == Goncalo Gomes <goncalo@promisc.org> writes:
> 
> Goncalo> The attached patch fixes all warnings in tdep-i386.c resulting from
> Goncalo> enabling shadow warnings in gcc. As this is my first contribution to
> Goncalo> GDB, I decided to keep it short to a single file to obtain feedback.
> 
> Just a friendly word of warning -- this is arguably the worst project to
> pick of all the things on the project page (though unfortunately there
> is more than one stinker on there).  It's been attempted a couple of
> times, had always lead to a lot of contention, etc.
> 
> That doesn't mean it can't be done, just that you ought to expect it to
> be a pain.
> 
> Last time this came up, I think the conclusion was that we'd prefer it
> if we could get warnings only for some kinds of shadowing, but not all
> kinds.  There was a sense that warnings for shadowing of 'index' was not
> very useful.  I remember some discussion of checking for a GCC change in
> gdb's configure, but I don't remember the details any more.  They're in
> the archives.

Right.  In particular the case where a local variable "shadows" a
global function.  Until GCC stops warning about those cases I think we
should leave the -Wshadow issue alone.