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: [PATCH] Make static fields be lazy


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.


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