Re: Changing the "enclosing_type" of a value structure

Re: Changing the "enclosing_type" of a value structure

[Jim Ingham]

On Monday, August 14, 2000, at 11:32 PM, Eli Zaretskii wrote:

> > Date: Mon, 14 Aug 2000 11:32:07 -0700 
> > From: Jim Ingham <jingham@apple.com> 
> >  
> > The sketch I sent in the last note was not right.  You have to change the 
> > lazy flag 
>  
> Sorry for jumping in--I wasn't following this thread--but changing the 
> way the lazy flag is set might affect the hardware watchpoints, at 
> least on x86 targets.  The current code which checks whether a given 
> value can be watched by hardware watchpoint(s) and the code which 
> inserts the watchpoints assume that those parts of the value chain 
> which have their lazy flag set don't need to be watched.  This allows 
> to watch members of large structs and array elements without watching 
> the whole struct/array. 
>  
> If the issue you were discussing has no relation to that problem, I 
> apologize. 
>  

Humm...  I have to think about this some more.  The comment before the definition of the lazy field in value.h says:

    /* If zero, contents of this value are in the contents field.
       If nonzero, contents are in inferior memory at address
       in the location.address field plus the offset field
       (and the lval field should be lval_memory).  */
    char lazy;
 
This doesn't sound at first reading like the meaning that it is being given by the watchpoint code, which also doesn't quite jibe with the description of the VALUE_CONTENTS macro.  In valops.c & friends, the lazy flag is definitely used to indicate whether the data HAS been read in from inferior memory or not.

I don't yet understand how this all works enough to know whether this seeming overloading is actually a problem or not, though I agree that given how it is used for watchpoints, it is not as simple as I first thought.

Still digging...

Jim
Jim Ingham                                   jingham@apple.com
Apple Computer

Re: Changing the "enclosing_type" of a value structure

[Eli Zaretskii]

> Date: Tue, 15 Aug 2000 16:08:36 -0700
> From: Jim Ingham <jingham@apple.com>
>
>     /* If zero, contents of this value are in the contents field.
>        If nonzero, contents are in inferior memory at address
>        in the location.address field plus the offset field
>        (and the lval field should be lval_memory).  */
>     char lazy;
>  
> This doesn't sound at first reading like the meaning that it is
> being given by the watchpoint code

You are right.  When use of the lazy flag was suggested for fixing the
problem with watchpoints (GDB was unable to watch struct members and
array elements, it wanted to watch the entire struct/array), I
expressed my concerns about overloading the meaning of that flag, and
about the possibility that if someone changes the way the lazy flag
behaves, it could break watchpoints.

I think we need a prominent comment near the declaration of the flag
which alerts people to this use.  I will add it.

> In valops.c & friends, the lazy flag is definitely used to indicate
> whether the data HAS been read in from inferior memory or not.

That is indeed its meaning.  The reason it works with watchpoints is
because parts of the value chain which are irrelevant for watching the
value are never read from memory, and so remain lazy in most cases.

> I don't yet understand how this all works enough to know whether
> this seeming overloading is actually a problem or not, though I
> agree that given how it is used for watchpoints, it is not as simple
> as I first thought.

If your change turns on the lazy flag for something that should never
be watched, or if that flag will be turned off (once the value is
re-read) before GDB gets to inserting watchpoints, then it's quite
possible that there is no problems with your change.

> Still digging...

Let me know if I can help you figure this out.  I broke a few teeth at
the time digging into this, even though I stood on the Shoulders of
Giants such as Jim Blandy and Michael Snyder... ;-)

Re: Changing the "enclosing_type" of a value structure

[Jim Ingham]

on 8/15/00 11:20 PM, Eli Zaretskii at eliz@delorie.com wrote:

>> Date: Tue, 15 Aug 2000 16:08:36 -0700
>> From: Jim Ingham <jingham@apple.com>
>> 
>> /* If zero, contents of this value are in the contents field.
>> If nonzero, contents are in inferior memory at address
>> in the location.address field plus the offset field
>> (and the lval field should be lval_memory).  */
>> char lazy;
>> 
>> This doesn't sound at first reading like the meaning that it is
>> being given by the watchpoint code
> 
> You are right.  When use of the lazy flag was suggested for fixing the
> problem with watchpoints (GDB was unable to watch struct members and
> array elements, it wanted to watch the entire struct/array), I
> expressed my concerns about overloading the meaning of that flag, and
> about the possibility that if someone changes the way the lazy flag
> behaves, it could break watchpoints.
> 
> I think we need a prominent comment near the declaration of the flag
> which alerts people to this use.  I will add it.
> 

Would it be possible to break out the uses of lazy into two flags, a
"needs_fetching" flag to tell whether gdb needs to read the flag from the
inferior, and a "already_fetched"  flag, used if "needs_fetching" is true,
to indicate whether the data has been read in or not?  I don't think that we
are ever going to get & keep around so may struct value's that one more int
is a really big deal, and this would make the intent of the code a lot
clearer.

Then we could either pull the VALUE_LAZY test into the value_fetch_lazy, or
redefine the VALUE_LAZY macro to be (val)->needs_fetching &&
(val)->already_fetched.

This just seems lots less mysterious, at the expense of being a little more
verbose.  If you think this sounds like a good idea, I will whack it
together.

>> In valops.c & friends, the lazy flag is definitely used to indicate
>> whether the data HAS been read in from inferior memory or not.
> 
> That is indeed its meaning.  The reason it works with watchpoints is
> because parts of the value chain which are irrelevant for watching the
> value are never read from memory, and so remain lazy in most cases.
> 
>> I don't yet understand how this all works enough to know whether
>> this seeming overloading is actually a problem or not, though I
>> agree that given how it is used for watchpoints, it is not as simple
>> as I first thought.
> 
> If your change turns on the lazy flag for something that should never
> be watched, or if that flag will be turned off (once the value is
> re-read) before GDB gets to inserting watchpoints, then it's quite
> possible that there is no problems with your change.

I agree with this, but the "quite possible" qualifier (which I also agree
with) makes me a bit nervous.

> 
>> Still digging...
> 
> Let me know if I can help you figure this out.  I broke a few teeth at
> the time digging into this, even though I stood on the Shoulders of
> Giants such as Jim Blandy and Michael Snyder... ;-)

Cool, thanks.  I need to do some more investigations (mostly to raise my
level of cultural awareness), and then I may pester you with stupid
questions.

Jim
--
Jim Ingham                                            jingham@apple.com
Apple Computer, Inc.

Re: Changing the "enclosing_type" of a value structure

[Eli Zaretskii]

> Date: Wed, 16 Aug 2000 11:23:01 -0700
> From: Jim Ingham <jingham@apple.com>
> 
> Would it be possible to break out the uses of lazy into two flags, a
> "needs_fetching" flag to tell whether gdb needs to read the flag from the
> inferior, and a "already_fetched"  flag, used if "needs_fetching" is true,
> to indicate whether the data has been read in or not?

I don't see why not.

However, what worried (and continues to worry) me is that the
causality between a value being not fetched and its relation to the
expression whose value is to be watched--that causality is not based
on anything but empirical evidence.  Dividing the flag into two
doesn't change that.

But if it will help to clear the mystery a bit, IMHO we should do it.

Re: Changing the "enclosing_type" of a value structure

[Eli Zaretskii]

> Date: Mon, 14 Aug 2000 11:32:07 -0700
> From: Jim Ingham <jingham@apple.com>
> 
> The sketch I sent in the last note was not right.  You have to change the
> lazy flag

Sorry for jumping in--I wasn't following this thread--but changing the
way the lazy flag is set might affect the hardware watchpoints, at
least on x86 targets.  The current code which checks whether a given
value can be watched by hardware watchpoint(s) and the code which
inserts the watchpoints assume that those parts of the value chain
which have their lazy flag set don't need to be watched.  This allows
to watch members of large structs and array elements without watching
the whole struct/array.

If the issue you were discussing has no relation to that problem, I
apologize.

Re: Changing the "enclosing_type" of a value structure

[Jim Ingham]

Also sprach Andrew Cagney:

> Jim Ingham wrote:
> 
>> /* If we have the full object, but for some reason the enclosing
>> type is wrong, set it *//* pai: FIXME -- sounds iffy */
> 
> I like the comment...

Yeah, this is the sort of thing that really warms your heart after you have
spent a couple of days chasing down memory corruption...

> 
>> return (value_ptr) xrealloc (val, sizeof (struct type)
>> + TYPE_LENGTH (new_type));
> 
> Should it simply refuse to expand a type?  That extra data becomes
> undefined in general?
> 


The sketch I sent in the last note was not right.  You have to change the
lazy flag - which will take care of your objection (though in all the code
paths I could see the data had not been read when this switch was done) -
and you have to stick the new value back into the value chain, or the old
value will get freed, which is BAD.  Here is another - better - version.
This one seems to work pretty well.

value_ptr
value_change_enclosing_type (value_ptr val, struct type *new_encl_type)
{

  /* If you follow the code, most of the time that this function
     is called, the types are the same...  So shortcut this case. */

  if (new_encl_type == VALUE_ENCLOSING_TYPE (val))
    {
      return val;
    }
  else if (TYPE_LENGTH (new_encl_type)
           > TYPE_LENGTH (VALUE_ENCLOSING_TYPE (val)))
    {
       value_ptr old_val = val;
       register value_ptr prev;

       val = (value_ptr) xrealloc (old_val, sizeof (struct value)
                                       + TYPE_LENGTH (new_encl_type));

       /* We have to make sure this ends up in the same place in the value
         chain as the original copy, so it's clean-up behavior is the same.
          If the value has been released, this is a waste of time, but there
          is no way to tell that in advance, so... */

       if (old_val != all_values)
         {
           for (prev = all_values; prev != NULL; prev = prev->next)
             {
               if (prev->next == old_val)
                 {
                   prev->next = val;
                   break;
                 }
             }
         }
    }

  VALUE_ENCLOSING_TYPE (val) = new_encl_type;

  /* If we had to change the enclosing type, the data in the value
     is no longer good.  Setting lazy back to 1 will force it to be
     reread. */

  VALUE_LAZY (val) = 1;

  return val;
}


Jim
-- 
Jim Ingham                                 jingham@apple.com
Apple Computer

Re: Changing the "enclosing_type" of a value structure

[Andrew Cagney]

Jim Ingham wrote:

>    /* If we have the full object, but for some reason the enclosing
>       type is wrong, set it *//* pai: FIXME -- sounds iffy */

I like the comment...

>        return (value_ptr) xrealloc (val, sizeof (struct type)
>                                     + TYPE_LENGTH (new_type));

Should it simply refuse to expand a type?  That extra data becomes
undefined in general?

	Andrew

Changing the "enclosing_type" of a value structure

[Jim Ingham]

Hi, all...

I found a number of places in the gdb sources where we are changing the
enclosing_type field of a value structure.  For instance, in
valops.c:3490ff,  the following is done:

   /* If we have the full object, but for some reason the enclosing
      type is wrong, set it *//* pai: FIXME -- sounds iffy */
   if (full)
     {
       VALUE_ENCLOSING_TYPE (argp) = real_type;
       return argp;
     }

NB: it is NOT a good idea to blythely change the enclosing type of a
value, since allocate_value uses the length of the enclosing type to
figure out the size of the data area to allocate at the end of the
"value structure".  So if the new enclosing_type is bigger than the one
you passed to allocate_value, and you ever go to read in the data, you
will stomp whatever happens to be after this value structure in memory,
causing all sorts of badness later on...  This bit of code was causing a
crash for just this reason when I was viewing member objects of a C++
class.

There are other cases like this (valops.c:791ff):

  /* Get target memory address */
  arg2 = value_from_pointer (lookup_pointer_type (VALUE_TYPE (arg1)),
                 (VALUE_ADDRESS (arg1)
                  + VALUE_OFFSET (arg1)
                  + VALUE_EMBEDDED_OFFSET (arg1)));

  /* This may be a pointer to a base subobject; so remember the
     full derived object's type ... */
  VALUE_ENCLOSING_TYPE (arg2) = lookup_pointer_type(VALUE_ENCLOSING_TYPE
(arg1));

Which happens to work because these are both pointers, and so have the same
size...

My software fascist side says you should NEVER be allowed to change the
enclosing_type of a value without at least putting in an assert making
sure that the new enclosing_type's length is <= that of the old
enclosing type.  After all, making an error here may cause memory
corruption in the application, which is generally pretty unpleasant to
track down.

It might even be good to introduce a function like:

value_ptr
value_change_enclosing_type (value_ptr val, type *new_type)
{
  if (TYPE_LENGTH (new_type) <= TYPE_LENGTH (VALUE_ENCLOSING_TYPE (val)))
    {
      VALUE_ENCLOSING_TYPE(val) = new_type;
      return val;
    }
  else
    {
       return (value_ptr) xrealloc (val, sizeof (struct type)
                                    + TYPE_LENGTH (new_type));
    }
}

and use that everywhere that we were just reassigning before.

If this makes sense to people (and/or I am not missing something
obvious), then I will whip up a patch and submit it.

Jim
-- 
Jim Ingham                                 jingham@apple.com
Apple Computer