Re: [PATCH v7 1/1] fortran: Fix arrays of variable length strings for FORTRAN

[Tom Tromey <tom@tromey.com>]

>>>>> "Abdul" == Abdul Basit Ijaz <abdul.b.ijaz@intel.com> writes:

Abdul> Before this change resolve_dynamic_array_or_string was called for
Abdul> all TYPE_CODE_ARRAY and TYPE_CODE_STRING types, but, in the end,
Abdul> this function always called create_array_type_with_stride, which
Abdul> creates a TYPE_CODE_ARRAY type.

This part makes sense to me but I have some other questions & comments.

Abdul> Lastly, in gdb.opt/fortran-string.exp and gdb.fortran/string-types.exp
Abdul> tests it expects type of character array in 'character (3)' format but now
Abdul> after this change we get 'character*3', so tests are updated accordingly.

I don't know Fortran but I gather this is the result of the
array->string change.

Abdul> --- a/gdb/c-valprint.c
Abdul> +++ b/gdb/c-valprint.c
Abdul> @@ -427,6 +427,7 @@ c_value_print_inner (struct value *val, struct ui_file *stream, int recurse,
Abdul>    switch (type->code ())
Abdul>      {
Abdul>      case TYPE_CODE_ARRAY:
Abdul> +    case TYPE_CODE_STRING:
Abdul>        c_value_print_array (val, stream, recurse, options);

I think it would be better to put this in generic_value_print instead.

The normal rule (not extremely well enforced maybe) is that a language's
value-printer should defer to this for any types it doesn't understand.

Abdul> +	 The problem here is that, when we create values from the dynamic
Abdul> +	 array type, we resolve the data location, and use that as the
Abdul> +	 value address, this completely discards the original value
Abdul> +	 address

I don't think I understand why this happens.
Maybe some DWARF snippet is missing from either this comment or from the
explanatory text in the commit.

Abdul> +  if (prop != nullptr && type->code () == TYPE_CODE_STRING)
Abdul> +    {
Abdul> +       prop = nullptr;
Abdul> +       warning (_("byte stride property ignored on string type"));
Abdul> +    }

Seems weird to warn in this low-level code.

IMO this could be silently ignored and/or enforced by the debuginfo
readers.

Given this new code, is f77_get_dynamic_length_of_aggregate still
necessary?  If so, why?  Since it seems like now there are two different
ways of doing the same thing.  (Note this isn't necessarily bad -- Ada
has to do this since there are different debuginfo representations that
wind up as different internal representations in gdb.)

Tom