Re: Calculating array length

[Joost van der Sluis <joost at cnoc dot nl>]

Op zondag 21-06-2009 om 00:34 uur [tijdzone +0200], schreef Jan
Kratochvil:
> On Sun, 14 Jun 2009 22:29:38 +0200, Joost van der Sluis wrote:

> Created an artifical DWARF testcase for the problematic case of Pascal array
> containing AnsiStrings (as simulated by `struct string_t' in the attached
> testcase gdb.arch/x86_64-pascal-string-array.c).  Therefore TYPE_LENGTH of the
> same `string_t' type will differ for each element of the array, depending on
> the current object_address (shifted by the element size=stride for each array
> element).
> Original C code:
>   struct string_t
>     {
>       const char *string;
>       unsigned long length;
>     }
>   array[3];
> Hand-modified DWARF originally produced from the C code above:
>  <2><84>: Abbrev Number: 5 (DW_TAG_string_type)
>     <85>   DW_AT_name        : (indirect string, offset: 0x0): string_t
>     <89>   DW_AT_byte_size   : 8
>     <8a>   DW_AT_string_length: 3 byte block: 97 23 8   (DW_OP_push_object_address; DW_OP_plus_uconst: 8)
>     <8e>   DW_AT_data_location: 2 byte block: 97 6      (DW_OP_push_object_address; DW_OP_deref)
>  <2><93>: Abbrev Number: 4 (DW_TAG_variable)
>     <94>   DW_AT_name        : (indirect string, offset: 0x11): array
>     <9a>   DW_AT_type        : <0xf9>
>     <9e>   DW_AT_location    : 3 byte block: 91 80 7f   (DW_OP_fbreg: -128)
>  <1><b6>: Abbrev Number: 10 (DW_TAG_base_type)
>     <b7>   DW_AT_byte_size   : 8
>     <b8>   DW_AT_encoding    : 7        (unsigned)
>  <1><f9>: Abbrev Number: 14 (DW_TAG_array_type)
>     <fa>   DW_AT_type        : <0x84>
>  <2><fe>: Abbrev Number: 15 (DW_TAG_subrange_type)
>     <ff>   DW_AT_type        : <0xb6>
>     <103>   DW_AT_upper_bound : 2
>     <104>   DW_AT_stride      : 16

This is not the same as the fpc-compiler does it. Fpc encodes it's
strings as 1-based arrays of char. That's mainly done because that way
you can also debug strings with older versions of gdb. So this test-case
is not completely the same as the pascal-array of strings case.

But the dynamic length is tested with this, yes.

> > To see if this is really neccessary to do, I test if
> > TYPE_DATA_LOCATION_DWARF_BLOCK is set. I doubt if that is ok, but it
> > works for my case.
> > 
> > Also pascal_cal_print is adapted so that it does not call check_typedef
> > on the type before it is passed to val_print_array_elements.
> 
> This part was workarounding an existing bug of the VLA patch as it creates
> a static type (=evaluate dynamic fields for the current variable/sub-part)
> recursively by check_typedef().

Yes it is a workoround for that, but I didn't knew you considered the
recursive calls a bug.

> A better way would be to make static only one step and the caller would have
> to call check_typedef() on TYPE_TARGET_TYPE etc. again.  This way was
> attempted by the attached patch.  It works with the new `string_t' testcase
> and I hope it would work even for your Pascal arrays (I do not have the
> compiler for them).

It only works partly. The length of the strings are ok. But they are not
displayed correctly. That's because at the object_address points at the
address where another address is stored, which points at the actual
string. With your patch this second address is printed as the output.

So not only the type has to be evaluated again for each element, also
the data-location.

> While trying to fix it I stopped as this path of global object_address_set was
> only temporary for maintainability as a 3rd party patch.  As the patch has
> been promised as generally acceptable it needs to be done the right way instead:
> 
> (1) object_address (for DW_OP_push_object_address) should be local for each
>     variable / type evaluation.  That means changing many `struct type *' to
>     carry also the object address, now assuming by using `struct value *'
>     having the `lazy' flag set and object address stored there in
>     value->location.address.

But this means that the object address of a type should be set. Where do
we do that? And is it true that a 'struct type *' is only used for one
variable at a time? How do we enforce that?

I was working on a patch that removes the dynamic stuff from the 'stuct
type *' into the 'struct value *'. That works, but the main problem with
that is that the *_val_print functions do not have a 'struct value *' as
parameter, so they can not access it. Or they should construct a new
'struct value' from the type and the address. 

So maybe we should adapt value_fetch_lazy so that it does not only sets
val_address, but also the  dynamic fields in the type. That including
point 2,3, and 4 below should do it. Do you think this could be an
acceprable way to fix this issue?


> (2) Function check_typedef should be dropped and its resolving of TYPE_DYNAMIC
>     fields should be done on-demand, when any code asks for it.  It is mostly
>     required by the new attached testcase where each array element's
>     TYPE_LENGTH differs.
> 
>     It will also make the types garbage collector unused by the VLA patch.
> 
>     The check_typedef removal should be easy to make incrementally.
> 
> (3) object_address_get_data() (converting now object address -> data address)
>     should be dropped - the object address (for DW_OP_push_object_address)
>     needs to be kept indefinitely for `struct value' as the data address
>     (possibly different by optional DW_AT_data_location) will be evaluated
>     dynamically derived from the object address.  This will remove the problem
>     of the single address being kept now which sometimes needs to be the
>     object address and sometimes the data address.
> 
> (4) value_raw_address() vs. value_address() (as is shifted by value->offset)
>     should be made more clear.  value_address() should return _data_address_
>     + offset.  value_raw_address() is only used once in java_value_print, it
>     may be dropped.
> 
> 
> Currently not intending to accept neither of the mine or yours patches for
> archer-jankratochvil-vla, is it a problem to keep the your patch for your GDB
> fork?  Going to start working on the right solution above as I already lost
> a lot of time trying to keep the current patch in its add-on form.

Offcourse. As long as we are trying to find a proper solution and until
it's stabilazed somewhat, I can setup my own repository. It's only not
my intention to maintain a pascal-fork of gdb forever. The goal is to
get it into the main gdb-repositories, offcourse.

Thanks,

Joost.