Re: Patch for pascal-dynamic arrays

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Wed, 28 Oct 2009 18:34:46 +0100, Joost van der Sluis wrote:
> I've tested for regressions, this time with ada and fortran enabled and
> didn't have any regressions.

I get:
	$ cd gdb/testsuite; make site.exp; runtest gdb.ada/variant_record_packed_array.exp

	Running ./gdb.ada/variant_record_packed_array.exp ...
	FAIL: gdb.ada/variant_record_packed_array.exp: print empty (GDB internal error)

	(gdb) print my_buffer
	$1 = (size => 8, buffer => (ada-valprint.c:93: internal-error: print_optional_low_bound: Assertion `0' failed.
	A problem internal to GDB has been detected,
	further debugging may prove unreliable.
	Quit this debugging session? (y or n) FAIL: gdb.ada/variant_record_packed_array.exp: print empty (GDB internal error)

	gcc-gnat-4.4.2-4.fc12.x86_64

This is due to my part of the patch as commented below:
	> 2009 Jan Kratochvil <jan.kratochvil@redhat.com>>
	> * ada-valprint.c (print_optional_low_bound): no idea

I do not understand how could this patch pass your regression testing.

(Fortran testcases now PASS for me so this is just a minor point, thanks.)

The patch otherwise looks as a nice extension for archer-jankratochvil-vla now
but please fix up this ADA part.


> * tekhex.c (move_section_contents): fixed usage of offset parameter

> --- a/bfd/tekhex.c
> +++ b/bfd/tekhex.c
> @@ -583,8 +583,7 @@ move_section_contents (bfd *abfd,
>    bfd_vma prev_number = 1;	/* Nothing can have this as a high bit.  */
>    struct data_struct *d = NULL;
>  
> -  BFD_ASSERT (offset == 0);
> -  for (addr = section->vma; count != 0; count--, addr++)
> +  for (addr = section->vma + offset; count != 0; count--, addr++)
>      {
>        /* Get high bits of address.  */
>        bfd_vma chunk_number = addr & ~(bfd_vma) CHUNK_MASK;

(a) You use tekhex binary format for some builds wrt Pascal?
(b) OFFSET is already asserted as 0, this change must be a nop.


> 2009 Jan Kratochvil <jan.kratochvil@redhat.com>>
> * ada-valprint.c (print_optional_low_bound): no idea

> diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
> index 565172c..af5def1 100644
> --- a/gdb/ada-valprint.c
> +++ b/gdb/ada-valprint.c
> @@ -90,7 +90,8 @@ print_optional_low_bound (struct ui_file *stream, struct type *type,
>    if (options->print_array_indexes)
>      return 0;
>  
> -  if (!get_array_bounds (type, &low_bound, &high_bound))
> +gdb_assert (0);        /* type vs. val */
> +  if (!get_array_bounds (NULL, &low_bound, &high_bound))
>      return 0;
>  
>    /* If this is an empty array, then don't print the lower bound.

This patch I wrote to be able to build GDB first the first variant of the
patch as you built it without -Wall -Werror and thus the code was invalid.

As you changed the get_array_bounds prototype to require now `struct value *'
and not `struct type *' as befoer the ADA code was no longer compatible.

I did not find simple enough to fix it, there is no `struct value *' available
at that specific point of ADA code.  Either (a) one should pass
`struct value *' there somehow or (b) one should still provide
get_array_bounds variant accepting just `struct type *' for such cases.
Whether (b) is a feasible way or whether one should go the (a) way I found to
be more decided by the author of the patch - you.

One needs to care of all the targets and supported languages in the GNU
projects as even the generic code benefits from the code contributed by the
people around such specific targets/languages as ADA.


> * cp-valprint.c (cp_print_value_fields): when the address is 0, do not pass
> the 0 value increased with some offset to val_print, but pass 0 instead

> --- a/gdb/cp-valprint.c
> +++ b/gdb/cp-valprint.c
> -		  val_print (TYPE_FIELD_TYPE (type, i),
> -			     valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
> -			     address + TYPE_FIELD_BITPOS (type, i) / 8,
> -			     stream, recurse + 1, &opts,
> -			     current_language);
> +                  if (address != 0)
> +		    val_print (TYPE_FIELD_TYPE (type, i),
> +		               valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
> +			       address + TYPE_FIELD_BITPOS (type, i) / 8,
> +			       stream, recurse + 1, &opts,
> +			       current_language);
> +                  else
> +		    val_print (TYPE_FIELD_TYPE (type, i),
> +		               valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
> +			       0,
> +			       stream, recurse + 1, &opts,
> +			       current_language);

ADDRESS zero is a valid address of a variable on some targets, it must not be
treated as a specific "undefined" case.  GDB for example uses for this purpose
value ~0 (INVALID_ENTRY_POINT) which is also not right but less probable to be
hit in real world cases.

(Still this part is OK for archer-jankratochvil-vla but not for FSF GDB.)


> +  type=check_typedef_target (type);
        ^^^ -> type = check_typedef
		http://www.gnu.org/prep/standards/standards.html#Formatting


> @@ -240,7 +243,6 @@ static struct value_history_chunk *value_history_chain;
>  
>  static int value_history_count;	/* Abs number of last entry stored */
>  
> -\f
>  /* List of all value objects currently allocated
>     (except for those released by calls to release_value)
>     This is so they can be freed after each command.  */

Please drop such excessive patch changes.


> @@ -554,9 +556,23 @@ value_raw_address (struct value *value)
>  void
>  set_value_address (struct value *value, CORE_ADDR addr)
>  {
> +  CORE_ADDR data_addr = addr;
>    gdb_assert (value->lval != lval_internalvar
>  	      && value->lval != lval_internalvar_component);
>    value->location.address = addr;
> +  object_address_get_data (value_type (value), &data_addr);
> +  value->data_address = data_addr;
> +}
> +
> +CORE_ADDR
> +data_address (struct value *value)
> +{
> +  return value->data_address;
> +}
> +void
> +set_data_address (struct value *value, CORE_ADDR addr)
> +{
> +  value->data_address = addr;
>  }
>  
>  struct internalvar **

"data_address" is very general name when it is `struct value *' specific,
I would prefer some value_data_address / set_value_data_address, don't you?



> @@ -577,6 +593,53 @@ deprecated_value_regnum_hack (struct value *value)
>    return &value->regnum;
>  }
>  
> +long
> +get_bound (struct type *type, int i)
> +{
> +  struct type *index = TYPE_INDEX_TYPE (type);
> +  if ((!(index == NULL)) && (TYPE_CODE (index) == TYPE_CODE_RANGE))
> +    {
> +      int nfields;
> +      nfields = TYPE_NFIELDS (index);
> +
> +      if (nfields>(i-1))
> +        {
> +          switch (TYPE_FIELD_LOC_KIND (index, i))
> +            {
> +              case FIELD_LOC_KIND_BITPOS:
> +                return TYPE_FIELD_BITPOS (index, i);
> +              case FIELD_LOC_KIND_DWARF_BLOCK:
> +                if (TYPE_NOT_ALLOCATED (index)
> +                  || TYPE_NOT_ASSOCIATED (index))
> +                  return 0;
> +                else
> +                  {
> +                    return dwarf_locexpr_baton_eval (TYPE_FIELD_DWARF_BLOCK (index, i));
> +                  }
> +                break;
> +              default:
> +                internal_error (__FILE__, __LINE__,
> +                                _("Unexpected type field location kind: %d"),
> +                                  TYPE_FIELD_LOC_KIND (index, i));
> +            }
> +        }
> +    }
> +  /* NOTREACHED */
> +  return -1;
> +}
> +
> +long
> +value_lower_bound (struct value *value)
> +{
> +  return get_bound (value_type (value), 0);
> +}
> +
> +long 
> +value_upper_bound (struct value *value)
> +{
> +  return get_bound (value_type (value), 1);
> +}
> +
>  int
>  deprecated_value_modifiable (struct value *value)
>  {

The `long' data type is neither the minimal static bound as returned for
FIELD_LOC_KIND_BITPOS (which is currently `int') nor large enough to catch
CORE_ADDR of dwarf_locexpr_baton_eval (on some 32bit hosts with 64bit target
requiring `long long').

I would find `int' here more appropriate to just copy the
FIELD_LOC_KIND_BITPOS (main_type.fields->loc.bitpos) type.



Thanks,
Jan