Trivial fix in value_sub

Trivial fix in value_sub

[Jim Ingham]

In value_sub, if you are doing pointer - integer and pointer points to 
an incomplete type,
then the computation:

ptr - sizeof (*ptr) * integer

turns into just:

ptr

which is probably not what the user expected.  In value_add, the return 
from TYPE_LENGTH is checked for zero, and set to 1 if it is.  We should 
probably do the same thing for value_sub, a la:

2002-03-29  James Ingham  <jingham@apple.com>

         * valarith.c (value_sub): If you are doing pointer - integer,
         and TYPE_LENGTH of the pointer's target comes back 0, set it
         to 1.  This is what value_add does, and we should be consistent.

Index: valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.14
diff -c -w -r1.14 valarith.c
*** valarith.c  2002/03/27 21:35:35     1.14
--- valarith.c  2002/03/29 23:22:27
***************
*** 105,110 ****
--- 105,112 ----
         {
           /* pointer - integer.  */
           LONGEST sz = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE 
(type1)));
+         if (sz == 0)
+           sz = 1;
           return value_from_pointer (type1,
                                      (value_as_address (arg1)
                                       - (sz * value_as_long (arg2))));

Jim
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer

Re: Trivial fix in value_sub

[Jim Blandy]

(Thanks for making value_add and value_sub consistent!)

If I use an incomplete type in my program --- say, by making a
definition like this:

        struct foo *x;

where there is no definition for `struct foo' in scope --- does GDB
set TYPE_LENGTH (TYPE_TARGET_TYPE (p)) to zero, where `p' is the type
of x?

See, that code in value_add (and now in value_sub) is supposed to
handle void *; as an extension, GCC allows arithmetic on void *
values, treating sizeof (void) as one.  This makes sense for void *
values, since they're often used as pointers to raw memory.

However, for things like incomplete struct types, treating the size as
one is completely bogus.  That's surely not the behavior the user
would expect; they may not even realize that the type is incomplete.

If GDB really does set the length of an incomplete struct type to
zero, then that code should really read something like:

  if (sz == 0 && TYPE_CODE (TYPE_TARGET_TYPE (type1)) == TYPE_CODE_VOID)
    sz = 1;

Can you tell me more about the context in which you noticed this
problem?

Re: Trivial fix in value_sub

[Michael Snyder]

Jim Blandy wrote:
> 
> (Thanks for making value_add and value_sub consistent!)
> 
> If I use an incomplete type in my program --- say, by making a
> definition like this:
> 
>         struct foo *x;
> 
> where there is no definition for `struct foo' in scope --- does GDB
> set TYPE_LENGTH (TYPE_TARGET_TYPE (p)) to zero, where `p' is the type
> of x?
> 
> See, that code in value_add (and now in value_sub) is supposed to
> handle void *; as an extension, GCC allows arithmetic on void *
> values, treating sizeof (void) as one.  This makes sense for void *
> values, since they're often used as pointers to raw memory.
> 
> However, for things like incomplete struct types, treating the size as
> one is completely bogus.  That's surely not the behavior the user
> would expect; they may not even realize that the type is incomplete.

I agree -- I think taking the sizeof an incomplete type should
result in an error, just as it would in c/c++.


> If GDB really does set the length of an incomplete struct type to
> zero, then that code should really read something like:
> 
>   if (sz == 0 && TYPE_CODE (TYPE_TARGET_TYPE (type1)) == TYPE_CODE_VOID)
>     sz = 1;
> 
> Can you tell me more about the context in which you noticed this
> problem?

Heh -- I bet I know.   ;-)

Re: Trivial fix in value_sub

[Jim Ingham]

Jim,

For incomplete types, TYPE_LENGTH does indeed return 0.  If it is an 
incomplete type, I don't think you should treat the size as 0, since 
that is CERTAINLY not what the user expected.  If you are going to 
handle it specially, then you should return an error in this case, 
saying something like "Can't do pointer arithmetic on incomplete types, 
try casting it as (void *)."

The way the problem came up is that the Toolbox folks here use lots of 
intentionally incomplete structures to mark tokens that are hiding real 
structures behind the curtains.  But they know the secret handshakes, so 
they know where things are around the pointers to fake structs, and use 
this kind of pointer arithmetic to poke around.  So treating the 
addition as void * is what they expect, and an error would probably 
mildly tick them off.

OTOH, you can always get around this by casting the pointer, either to 
void * or to what it is...  I am pretty sure their use is just shorthand.

So... I don't think you should keep the size at 0.  This seems like gdb 
is just silently ignoring the " -  x" part of what they typed, and you 
should always be explicit about what you have done.  But if you think an 
error is more appropriate, I am fine with that...

Jim

On Wednesday, April 3, 2002, at 02:52  PM, Jim Blandy wrote:

>
> (Thanks for making value_add and value_sub consistent!)
>
> If I use an incomplete type in my program --- say, by making a
> definition like this:
>
>         struct foo *x;
>
> where there is no definition for `struct foo' in scope --- does GDB
> set TYPE_LENGTH (TYPE_TARGET_TYPE (p)) to zero, where `p' is the type
> of x?
>
> See, that code in value_add (and now in value_sub) is supposed to
> handle void *; as an extension, GCC allows arithmetic on void *
> values, treating sizeof (void) as one.  This makes sense for void *
> values, since they're often used as pointers to raw memory.
>
> However, for things like incomplete struct types, treating the size as
> one is completely bogus.  That's surely not the behavior the user
> would expect; they may not even realize that the type is incomplete.
>
> If GDB really does set the length of an incomplete struct type to
> zero, then that code should really read something like:
>
>   if (sz == 0 && TYPE_CODE (TYPE_TARGET_TYPE (type1)) == TYPE_CODE_VOID)
>     sz = 1;
>
> Can you tell me more about the context in which you noticed this
> problem?
>
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer

Re: Trivial fix in value_sub

[Jim Blandy]

Jim Ingham <jingham@apple.com> writes:
> So... I don't think you should keep the size at 0.  This seems like
> gdb is just silently ignoring the " -  x" part of what they typed, and
> you should always be explicit about what you have done.  But if you
> think an error is more appropriate, I am fine with that...

Oh, no, I didn't mean to suggest that zero was the right size to use;
I agree with you completely that that would be pretty confusing.

Your story is pretty amazing --- I would never have guessed that
people actually *use* the sizeof (struct incomplete) == 1 behavior!  I
think it is much more common for people to be unaware that the type is
incomplete; if this hunch is correct, then the behavior your toolbox
folks love will be very confusing.  I think an error for arithmetic on
any incomplete type other than (void *) is the right thing.

Re: Trivial fix in value_sub

[Daniel Jacobowitz]

On Wed, Apr 03, 2002 at 10:54:57PM -0500, Jim Blandy wrote:
> 
> Jim Ingham <jingham@apple.com> writes:
> > So... I don't think you should keep the size at 0.  This seems like
> > gdb is just silently ignoring the " -  x" part of what they typed, and
> > you should always be explicit about what you have done.  But if you
> > think an error is more appropriate, I am fine with that...
> 
> Oh, no, I didn't mean to suggest that zero was the right size to use;
> I agree with you completely that that would be pretty confusing.
> 
> Your story is pretty amazing --- I would never have guessed that
> people actually *use* the sizeof (struct incomplete) == 1 behavior!  I
> think it is much more common for people to be unaware that the type is
> incomplete; if this hunch is correct, then the behavior your toolbox
> folks love will be very confusing.  I think an error for arithmetic on
> any incomplete type other than (void *) is the right thing.

FWIW, I agree.  If we don't know what the size is, we should say so;
having the behavior change based on whether a version of the
implementation (which might have debug symbols) is loaded would be
baffling.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: Trivial fix in value_sub

[Jim Ingham]

Okay, so to be consistent, I throw errors in both value_add & 
value_sub.  How about this:

Index: valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.14
diff -c -w -p -r1.14 valarith.c
*** valarith.c  2002/03/27 21:35:35     1.14
--- valarith.c  2002/04/04 21:39:58
*************** value_add (struct value *arg1, struct va
*** 48,55 ****
   {
     struct value *valint;
     struct value *valptr;
!   register int len;
     struct type *type1, *type2, *valptrtype;

     COERCE_NUMBER (arg1);
     COERCE_NUMBER (arg2);
--- 48,56 ----
   {
     struct value *valint;
     struct value *valptr;
!   register int sz;
     struct type *type1, *type2, *valptrtype;
+   struct type *valtargettype;

     COERCE_NUMBER (arg1);
     COERCE_NUMBER (arg2);
*************** value_add (struct value *arg1, struct va
*** 77,88 ****
           valint = arg1;
           valptrtype = type2;
         }
!       len = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE 
(valptrtype)));
!       if (len == 0)
!       len = 1;                /* For (void *) */
         retval = value_from_pointer (valptrtype,
                                    value_as_address (valptr)
!                                  + (len * value_as_long (valint)));
         VALUE_BFD_SECTION (retval) = VALUE_BFD_SECTION (valptr);
         return retval;
       }
--- 78,109 ----
           valint = arg1;
           valptrtype = type2;
         }
!
!       valtargettype = check_typedef (TYPE_TARGET_TYPE (valptrtype));
!       sz = TYPE_LENGTH (valtargettype);
!       if (sz == 0)
!       {
!         if (TYPE_CODE (type1) == TYPE_CODE_VOID)
!           sz = 1;
!         else
!           {
!             char *name;
!
!             name = TYPE_NAME (valtargettype);
!             if (name == NULL)
!               name = TYPE_TAG_NAME (valtargettype);
!             if (name == NULL)
!               error ("Cannot perform pointer addition on incomplete 
types, "
!                      "try casting to a known type, or void *.");
!             else
!               error ("Cannot perform pointer addition on incomplete 
type \"%s\", "
!                      "try casting to a known type, or void *.", name);
!           }
!       }
!
         retval = value_from_pointer (valptrtype,
                                    value_as_address (valptr)
!                                  + (sz * value_as_long (valint)));
         VALUE_BFD_SECTION (retval) = VALUE_BFD_SECTION (valptr);
         return retval;
       }
*************** value_sub (struct value *arg1, struct va
*** 104,110 ****
         if (TYPE_CODE (type2) == TYPE_CODE_INT)
         {
           /* pointer - integer.  */
!         LONGEST sz = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE 
(type1)));
           return value_from_pointer (type1,
                                      (value_as_address (arg1)
                                       - (sz * value_as_long (arg2))));
--- 125,156 ----
         if (TYPE_CODE (type2) == TYPE_CODE_INT)
         {
           /* pointer - integer.  */
!         struct type *type1target = check_typedef (TYPE_TARGET_TYPE 
(type1));
!         LONGEST sz = TYPE_LENGTH (type1target);
!         /* This is the way value_add does it.  Is this right, because
!            it means you have a different result depending on whether 
you
!            have an incomplete type or not... */
!
!         if (sz == 0)
!           {
!             if (TYPE_CODE (type1) == TYPE_CODE_VOID)
!               sz = 1;
!             else
!               {
!                 char *name;
!
!                 name = TYPE_NAME (type1target);
!                 if (name == NULL)
!                   name = TYPE_TAG_NAME (type1target);
!                 if (name == NULL)
!                   error ("Cannot perform pointer subtraction on 
incomplete types, "
!                          "try casting to a known type, or void *.");
!               else
!                 error ("Cannot perform pointer subtraction on 
incomplete type \"%s\", "
!                        "try casting to a known type, or void *.", 
name);
!               }
!           }
!
           return value_from_pointer (type1,
                                      (value_as_address (arg1)
                                       - (sz * value_as_long (arg2))));


Jim

P.S. I have long ago given up being amazed at all the egregiously 
heinous uses to which people will put debuggers...  To quote my revered 
father: "If I were to shake my head and say "Oh, my god" every time I 
see something like this, I would spend so much time with my head & mouth 
in motion people would think I had acquired some form of ecclesiastical 
palsy..."

Jim

On Wednesday, April 3, 2002, at 08:11  PM, Daniel Jacobowitz wrote:

> On Wed, Apr 03, 2002 at 10:54:57PM -0500, Jim Blandy wrote:
>>
>> Jim Ingham <jingham@apple.com> writes:
>>> So... I don't think you should keep the size at 0.  This seems like
>>> gdb is just silently ignoring the " -  x" part of what they typed, and
>>> you should always be explicit about what you have done.  But if you
>>> think an error is more appropriate, I am fine with that...
>>
>> Oh, no, I didn't mean to suggest that zero was the right size to use;
>> I agree with you completely that that would be pretty confusing.
>>
>> Your story is pretty amazing --- I would never have guessed that
>> people actually *use* the sizeof (struct incomplete) == 1 behavior!  I
>> think it is much more common for people to be unaware that the type is
>> incomplete; if this hunch is correct, then the behavior your toolbox
>> folks love will be very confusing.  I think an error for arithmetic on
>> any incomplete type other than (void *) is the right thing.
>
> FWIW, I agree.  If we don't know what the size is, we should say so;
> having the behavior change based on whether a version of the
> implementation (which might have debug symbols) is loaded would be
> baffling.
>
> --
> Daniel Jacobowitz                           Carnegie Mellon University
> MontaVista Software                         Debian GNU/Linux Developer
>
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer

Re: Trivial fix in value_sub

[Jim Blandy]

Jim Ingham <jingham@apple.com> writes:
> Okay, so to be consistent, I throw errors in both value_add &
> value_sub.  How about this:

Superb!  Could you move the "fix up my size or report an error" code
into a function instead of repeating it?

Re: Trivial fix in value_sub

[Jim Ingham]

Okay, how 'bout:

2002-04-04  Jim Ingham  <jingham@apple.com>

         * valarith.c (find_size_for_pointer_math): New function, either 
returns
	   the size for a pointer's target, returns 1 for void *, or errors for
         incomplete types.
         (value_add, value_sub): use find_size_for_pointer_math.

Index: valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.14
diff -c -w -p -r1.14 valarith.c
*** valarith.c  2002/03/27 21:35:35     1.14
--- valarith.c  2002/04/04 23:05:12
*************** static struct value *value_subscripted_r
*** 43,54 ****
   void _initialize_valarith (void);



   struct value *
   value_add (struct value *arg1, struct value *arg2)
   {
     struct value *valint;
     struct value *valptr;
!   register int len;
     struct type *type1, *type2, *valptrtype;

     COERCE_NUMBER (arg1);
--- 43,92 ----
   void _initialize_valarith (void);



+ /* Given a pointer, return the size of its target.
+    If the pointer type is void *, then return 1.
+    If the target type is incomplete, then error out.
+    This isn't a general purpose function, but just a
+    helper for value_sub & value_add.
+ */
+
+ static LONGEST
+ find_size_for_pointer_math (struct type *ptr_type)
+ {
+   LONGEST sz = -1;
+   struct type *ptr_target;
+
+   ptr_target = check_typedef (TYPE_TARGET_TYPE (ptr_type));
+
+   sz = TYPE_LENGTH (ptr_target);
+   if (sz == 0)
+     {
+       if (TYPE_CODE (ptr_type) == TYPE_CODE_VOID)
+       sz = 1;
+       else
+       {
+         char *name;
+
+         name = TYPE_NAME (ptr_target);
+         if (name == NULL)
+           name = TYPE_TAG_NAME (ptr_target);
+         if (name == NULL)
+           error ("Cannot perform pointer math on incomplete types, "
+                  "try casting to a known type, or void *.");
+         else
+           error ("Cannot perform pointer math on incomplete type 
\"%s\", "
+                  "try casting to a known type, or void *.", name);
+       }
+     }
+   return sz;
+ }
+
   struct value *
   value_add (struct value *arg1, struct value *arg2)
   {
     struct value *valint;
     struct value *valptr;
!   LONGEST sz;
     struct type *type1, *type2, *valptrtype;

     COERCE_NUMBER (arg1);
*************** value_add (struct value *arg1, struct va
*** 77,88 ****
           valint = arg1;
           valptrtype = type2;
         }
!       len = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE 
(valptrtype)));
!       if (len == 0)
!       len = 1;                /* For (void *) */
         retval = value_from_pointer (valptrtype,
                                    value_as_address (valptr)
!                                  + (len * value_as_long (valint)));
         VALUE_BFD_SECTION (retval) = VALUE_BFD_SECTION (valptr);
         return retval;
       }
--- 115,126 ----
           valint = arg1;
           valptrtype = type2;
         }
!
!       sz = find_size_for_pointer_math (valptrtype);
!
         retval = value_from_pointer (valptrtype,
                                    value_as_address (valptr)
!                                  + (sz * value_as_long (valint)));
         VALUE_BFD_SECTION (retval) = VALUE_BFD_SECTION (valptr);
         return retval;
       }
*************** value_sub (struct value *arg1, struct va
*** 104,110 ****
         if (TYPE_CODE (type2) == TYPE_CODE_INT)
         {
           /* pointer - integer.  */
!         LONGEST sz = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE 
(type1)));
           return value_from_pointer (type1,
                                      (value_as_address (arg1)
                                       - (sz * value_as_long (arg2))));
--- 142,149 ----
         if (TYPE_CODE (type2) == TYPE_CODE_INT)
         {
           /* pointer - integer.  */
!         LONGEST sz = find_size_for_pointer_math (type1);
!
           return value_from_pointer (type1,
                                      (value_as_address (arg1)
                                       - (sz * value_as_long (arg2))));

Jim

>
> Jim Ingham <jingham@apple.com> writes:
>> Okay, so to be consistent, I throw errors in both value_add &
>> value_sub.  How about this:
>
> Superb!  Could you move the "fix up my size or report an error" code
> into a function instead of repeating it?
>
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer

Re: Trivial fix in value_sub

[Jim Blandy]

Great.  Please commit that.

Jim Ingham <jingham@apple.com> writes:

> Okay, how 'bout:
> 
> 2002-04-04  Jim Ingham  <jingham@apple.com>
> 
>          * valarith.c (find_size_for_pointer_math): New function,
> either returns
> 	   the size for a pointer's target, returns 1 for void *, or errors for
>          incomplete types.
>          (value_add, value_sub): use find_size_for_pointer_math.
> 
> Index: valarith.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/valarith.c,v
> retrieving revision 1.14
> diff -c -w -p -r1.14 valarith.c
> *** valarith.c  2002/03/27 21:35:35     1.14
> --- valarith.c  2002/04/04 23:05:12
> *************** static struct value *value_subscripted_r
> *** 43,54 ****
>    void _initialize_valarith (void);
> 
> 
> 
>    struct value *
>    value_add (struct value *arg1, struct value *arg2)
>    {
>      struct value *valint;
>      struct value *valptr;
> !   register int len;
>      struct type *type1, *type2, *valptrtype;
> 
>      COERCE_NUMBER (arg1);
> --- 43,92 ----
>    void _initialize_valarith (void);
> 
> 
> 
> + /* Given a pointer, return the size of its target.
> +    If the pointer type is void *, then return 1.
> +    If the target type is incomplete, then error out.
> +    This isn't a general purpose function, but just a
> +    helper for value_sub & value_add.
> + */
> +
> + static LONGEST
> + find_size_for_pointer_math (struct type *ptr_type)
> + {
> +   LONGEST sz = -1;
> +   struct type *ptr_target;
> +
> +   ptr_target = check_typedef (TYPE_TARGET_TYPE (ptr_type));
> +
> +   sz = TYPE_LENGTH (ptr_target);
> +   if (sz == 0)
> +     {
> +       if (TYPE_CODE (ptr_type) == TYPE_CODE_VOID)
> +       sz = 1;
> +       else
> +       {
> +         char *name;
> +
> +         name = TYPE_NAME (ptr_target);
> +         if (name == NULL)
> +           name = TYPE_TAG_NAME (ptr_target);
> +         if (name == NULL)
> +           error ("Cannot perform pointer math on incomplete types, "
> +                  "try casting to a known type, or void *.");
> +         else
> +           error ("Cannot perform pointer math on incomplete type
> \"%s\", "
> +                  "try casting to a known type, or void *.", name);
> +       }
> +     }
> +   return sz;
> + }
> +
>    struct value *
>    value_add (struct value *arg1, struct value *arg2)
>    {
>      struct value *valint;
>      struct value *valptr;
> !   LONGEST sz;
>      struct type *type1, *type2, *valptrtype;
> 
>      COERCE_NUMBER (arg1);
> *************** value_add (struct value *arg1, struct va
> *** 77,88 ****
>            valint = arg1;
>            valptrtype = type2;
>          }
> !       len = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE
> (valptrtype)));
> !       if (len == 0)
> !       len = 1;                /* For (void *) */
>          retval = value_from_pointer (valptrtype,
>                                     value_as_address (valptr)
> !                                  + (len * value_as_long (valint)));
>          VALUE_BFD_SECTION (retval) = VALUE_BFD_SECTION (valptr);
>          return retval;
>        }
> --- 115,126 ----
>            valint = arg1;
>            valptrtype = type2;
>          }
> !
> !       sz = find_size_for_pointer_math (valptrtype);
> !
>          retval = value_from_pointer (valptrtype,
>                                     value_as_address (valptr)
> !                                  + (sz * value_as_long (valint)));
>          VALUE_BFD_SECTION (retval) = VALUE_BFD_SECTION (valptr);
>          return retval;
>        }
> *************** value_sub (struct value *arg1, struct va
> *** 104,110 ****
>          if (TYPE_CODE (type2) == TYPE_CODE_INT)
>          {
>            /* pointer - integer.  */
> !         LONGEST sz = TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE
> (type1)));
>            return value_from_pointer (type1,
>                                       (value_as_address (arg1)
>                                        - (sz * value_as_long (arg2))));
> --- 142,149 ----
>          if (TYPE_CODE (type2) == TYPE_CODE_INT)
>          {
>            /* pointer - integer.  */
> !         LONGEST sz = find_size_for_pointer_math (type1);
> !
>            return value_from_pointer (type1,
>                                       (value_as_address (arg1)
>                                        - (sz * value_as_long (arg2))));
> 
> Jim
> 
> >
> > Jim Ingham <jingham@apple.com> writes:
> >> Okay, so to be consistent, I throw errors in both value_add &
> >> value_sub.  How about this:
> >
> > Superb!  Could you move the "fix up my size or report an error" code
> > into a function instead of repeating it?
> >
> --
> Jim Ingham                                   jingham@apple.com
> Developer Tools - gdb
> Apple Computer

Re: Trivial fix in value_sub

[Jim Ingham]

Done.

On Thursday, April 4, 2002, at 03:15  PM, Jim Blandy wrote:

>
> Great.  Please commit that.
>
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer