[PATCH] Make static fields be lazy

[PATCH] Make static fields be lazy

[Stan Shebs]

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

This is a small but interesting fix; C++ static fields were being gotten 
using value_at instead of value_at_lazy.  So if you had a static field 
that was a 5000-element array, then "print sfield[5]" would actually 
fetch all 5000 elements from the target, then just print the one.

For the live target, you would never notice unless you did debug remote 
or debug target, or maybe noticed poor responsiveness.  But, for 
tracepoints, this becomes a bug; you do "collect sfield[5]", the target 
agent duly collects just the one element it was instructed to save, then 
when you choose a trace frame and say "print sfield[5]", the target 
kicks it back with a memory read failure because the frame doesn't have 
all 5000 elements.

Anyway, the fix is easy, and shorter than the explanation. :-)  
Committed to trunk.

Stan

2010-03-22  Stan Shebs  <stan@codesourcery.com>

    * value.c (value_static_field): Be lazy about the field's value.




[-- Attachment #2: lazystatic-patch-1 --]
[-- Type: text/plain, Size: 1229 bytes --]

Index: value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.100
diff -p -r1.100 value.c
*** value.c	9 Mar 2010 18:09:08 -0000	1.100
--- value.c	22 Mar 2010 18:46:14 -0000
*************** value_static_field (struct type *type, i
*** 1813,1820 ****
  
    if (TYPE_FIELD_LOC_KIND (type, fieldno) == FIELD_LOC_KIND_PHYSADDR)
      {
!       retval = value_at (TYPE_FIELD_TYPE (type, fieldno),
! 			 TYPE_FIELD_STATIC_PHYSADDR (type, fieldno));
      }
    else
      {
--- 1813,1820 ----
  
    if (TYPE_FIELD_LOC_KIND (type, fieldno) == FIELD_LOC_KIND_PHYSADDR)
      {
!       retval = value_at_lazy (TYPE_FIELD_TYPE (type, fieldno),
! 			      TYPE_FIELD_STATIC_PHYSADDR (type, fieldno));
      }
    else
      {
*************** value_static_field (struct type *type, i
*** 1831,1838 ****
  	    return NULL;
  	  else
  	    {
! 	      retval = value_at (TYPE_FIELD_TYPE (type, fieldno),
! 				 SYMBOL_VALUE_ADDRESS (msym));
  	    }
  	}
        else
--- 1831,1838 ----
  	    return NULL;
  	  else
  	    {
! 	      retval = value_at_lazy (TYPE_FIELD_TYPE (type, fieldno),
! 				      SYMBOL_VALUE_ADDRESS (msym));
  	    }
  	}
        else

Re: [PATCH] Make static fields be lazy

[Doug Evans]

On Mon, Mar 22, 2010 at 12:00 PM, Stan Shebs <stan@codesourcery.com> wrote:
> This is a small but interesting fix; C++ static fields were being gotten
> using value_at instead of value_at_lazy.  So if you had a static field that
> was a 5000-element array, then "print sfield[5]" would actually fetch all
> 5000 elements from the target, then just print the one.
>
> For the live target, you would never notice unless you did debug remote or
> debug target, or maybe noticed poor responsiveness.  But, for tracepoints,
> this becomes a bug; you do "collect sfield[5]", the target agent duly
> collects just the one element it was instructed to save, then when you
> choose a trace frame and say "print sfield[5]", the target kicks it back
> with a memory read failure because the frame doesn't have all 5000 elements.
>
> Anyway, the fix is easy, and shorter than the explanation. :-)  Committed to
> trunk.
>
> Stan
>
> 2010-03-22  Stan Shebs  <stan@codesourcery.com>
>
>   * value.c (value_static_field): Be lazy about the field's value.
>
>
>
>
> Index: value.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/value.c,v
> retrieving revision 1.100
> diff -p -r1.100 value.c
> *** value.c     9 Mar 2010 18:09:08 -0000       1.100
> --- value.c     22 Mar 2010 18:46:14 -0000
> *************** value_static_field (struct type *type, i
> *** 1813,1820 ****
>
>    if (TYPE_FIELD_LOC_KIND (type, fieldno) == FIELD_LOC_KIND_PHYSADDR)
>      {
> !       retval = value_at (TYPE_FIELD_TYPE (type, fieldno),
> !                        TYPE_FIELD_STATIC_PHYSADDR (type, fieldno));
>      }
>    else
>      {
> --- 1813,1820 ----
>
>    if (TYPE_FIELD_LOC_KIND (type, fieldno) == FIELD_LOC_KIND_PHYSADDR)
>      {
> !       retval = value_at_lazy (TYPE_FIELD_TYPE (type, fieldno),
> !                             TYPE_FIELD_STATIC_PHYSADDR (type, fieldno));
>      }
>    else
>      {
> *************** value_static_field (struct type *type, i
> *** 1831,1838 ****
>            return NULL;
>          else
>            {
> !             retval = value_at (TYPE_FIELD_TYPE (type, fieldno),
> !                                SYMBOL_VALUE_ADDRESS (msym));
>            }
>        }
>        else
> --- 1831,1838 ----
>            return NULL;
>          else
>            {
> !             retval = value_at_lazy (TYPE_FIELD_TYPE (type, fieldno),
> !                                     SYMBOL_VALUE_ADDRESS (msym));
>            }
>        }
>        else

fwiw, I think a comment is required in the code saying that
value_at_lazy is being used on purpose for the reasons you stated.
I think one shouldn't have to rely on the mailing list for
explanations of why code is the way it is (when it's easy to add such
explanations to the code).

There is a comment at the definition of value_at that says "Call
value_at only if the data needs to be fetched immediately; ...".
One might think that the existing comment is sufficient, but I dunno -
the word "lazy" implies an optimization issue not a correctness issue,
and the existing comment to me doesn't sufficiently address the point
that there is a correctness issue here.  An alternative might be to at
least elaborate on the comment for value_at.

Re: [PATCH] Make static fields be lazy

[Stan Shebs]

Doug Evans wrote:
> On Mon, Mar 22, 2010 at 12:00 PM, Stan Shebs <stan@codesourcery.com> wrote:
>   
>> This is a small but interesting fix; C++ static fields were being gotten
>> using value_at instead of value_at_lazy.  So if you had a static field that
>> was a 5000-element array, then "print sfield[5]" would actually fetch all
>> 5000 elements from the target, then just print the one.
>>
>> [...]
>>     
> fwiw, I think a comment is required in the code saying that
> value_at_lazy is being used on purpose for the reasons you stated.
> I think one shouldn't have to rely on the mailing list for
> explanations of why code is the way it is (when it's easy to add such
> explanations to the code).
>   
That's a good point.  By now just about all value_at's are lazy, this 
static field one was more of an unnoticed holdout, but it is not obvious 
that laziness is required by tracepoints, not just an optimization.  
I'll work up some commentary.

Stan