This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Variable objects laziness


Daniel Jacobowitz wrote:

> On Tue, Nov 14, 2006 at 04:43:24PM +0300, Vladimir Prus wrote:
>> At the moment, MI's variable objects are too eager. For example, if you
>> create a variable object corresponding to a structure, gdb will read the
>> entire structure from the memory, even though it never does anything with
>> that value directly. Structure values are not compared, and you can't
>> access them other than by creating children variable objects.
>> 
>> For example, if you have a huge structure containing two structure
>> fields, gdb will read all the outer structure even though you never open
>> one of the children structures.
>> 
>> Worse, the code for fetching the values is scattered over a number of
>> places.
> 
> I think this is a good patch.  I'm going to wait a few days before
> approving this, however, in case any of the other MI users on the list
> have comments.
> 
> Neither before nor after this patch do I really understand what
> "type_changeable" means so I'll trust you've got it right :-)

Do you want to me add a comment to that function?

> How does reading the children instead of the parent affect bitfields?

Good question, but I have a good answer -- the patch does not change this.
The only effect that instead of reading the structure right when you create
it, the structure will be read when you list the children.

> Will we do eight reads from the target for this?
> 
>   struct x {
>     unsigned char a:1, b:1, c:1, d:1, e:1, f:1, g:1, h:1;
>   };

No. Look at value_primitive_field. For bitfeilds, it calls
unpack_field_as_long, passing value_contents (args1) to it. The
value_contents function will read entire structure from memory on the first
time. For the second bitfield, the already-read data will be used.

> How about unions?

Here's what will happen for a union:

1. The value_primitive_field will be called to create values for all
elements of a union.

2. When value_fetch_lazy is called on each value, it will directly read
the content from the memory. Once for each value.

If the union's value is not lazy (like it is today), value_primitive_field
will immediately memcpy the value, and target memory will be read just once.

I think I can address this issue by making install_new_value always fetch
values of unions.

Or I can do nothing, because in CLI, if your program is:

    struct S
    {
        union {
           int i;
           int j;
        };
    };

    int main()
    {
        S s;
        .....

And you do:

    print s.i
    print s.j

it will generate two reads of memory too.

The only way to consistently fix this problem everywhere is to add 'parent'
field to struct value, so that we can lazily fetch parent, and do it just
once.


>> +    value = evaluate_type (var->root->exp);
>> +
>> +      var->type = value_type (value);
> 
> Space/tabs mismatch.

What do you mean? The second line is indented 6 spaces, so it can use tab
for indentation.

>> +/** Assign new value to a variable object.  If INITIAL is non-zero,
> 
> Javadoc?  Not here :-)  

Why not? ;-) It would be great to get API docs fro gdb. But year, it's
probably easier to teach doxygen to handle ordinary comments.

>> +  /* If the type is changeable, compare the old and the new values.
>> +     If the type of varobj changed, no point in comparing values.  */
> 
> "the type of the varobj".

In fact, the second sentence is stale, I've reworded it like this:

   If this is the initial assignment, we don't have any old value
   to compare with.

>> +  if (!initial && changeable)
>> +    {
>> +      /* If the value of the varobj was set by -var-set-value, then the
>> +     value in the varobj and in the target is the same.  However, that
>> value
>> +     is different from the value that the varobj had after the previous
>> +     -var-update. So need to the varobj as changed.  */
>> +      if (var->updated)
>> +    changed = 1;
> 
> "need to mark the".  Why is this true?  I would have thought we only
> needed to mark the varobj as changed if the value assigned to it was
> different from the previous one.  

True, the 'updated' flag is set only in that case. I've modified the comment
to read:

    /* If the value of the varobj was changed by -var-set-value, then the

>> +  /* We must always keep new values, since children depend on it. */
> 
> "values" is plural, "it" is singular, so this sounds strange; probably
> "must always keep the new value".

This comment was present in the old code. Fixed anyway.

Thanks,
Volodya





Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]