ping: [PATCH] Pass address around within ada-valprint.c

["Andrew Burgess" <aburgess at broadcom dot com>]

Any feedback on the following?


On 30/07/2013 11:40 AM, Andrew Burgess wrote:
> I was trying to better understand ada-lang.c:coerce_unspec_val_to_type,
> and after looking at the code for a while I tried experimentally
> applying this patch to make all values lazy:
>
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index dc5f2b6..58c4ba1 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -568,15 +568,8 @@ coerce_unspec_val_to_type (struct value *val,
> struct type *
>           trying to allocate some memory for it.  */
>        check_size (type);
>  
> -      if (value_lazy (val)
> -          || TYPE_LENGTH (type) > TYPE_LENGTH (value_type (val)))
> -       result = allocate_value_lazy (type);
> -      else
> -       {
> -         result = allocate_value (type);
> -         memcpy (value_contents_raw (result), value_contents (val),
> -                 TYPE_LENGTH (type));
> -       }
> +      result = allocate_value_lazy (type);
> +
>        set_value_component_location (result, val);
>        set_value_bitsize (result, value_bitsize (val));
>        set_value_bitpos (result, value_bitpos (val));
>
>
> I know we don't want to actually apply this patch as it would
> cause gdb to fetch the value contents from the inferior in cases
> where we currently reuse the contents we already have to hand,
> BUT, as an experiment, I don't see why it should not be fine.
>
> Unfortunately I ran into a few regressions.... it turns out that
> in some cases the address parameter on VAL is not set-up correctly [1].
>
> This worries me, given the "|| TYPE_LENGTH (type) > TYPE_LENGTH
> (value_type (val))" check, would it not be possible to create an
> example where the address of VAL is unset, but the sizes of the types
> involved force the creation of a lazy value?  If we could make such
> an example then it surely would fail.
>
> The patch below fixes all the regressions that the above experiment
> revealed, it's really just passing the address around inside
> ada-valprint.c a little more, seems like a good thing to me, but let
> me know what you think.
>
> OK to apply?
>
>
> Thanks,
> Andrew
>
> [1] Maybe there's a reason for this that I'm not seeing / understanding.
>
>
> 2013-07-30  Andrew Burgess  <aburgess@broadcom.com>
>
>     * ada-valprint.c (print_record): Add address parameter, and pass
>     the address on to sub-functions.
>     (print_field_values): Add address parameter, and pass the address
>     on to sub-functions.
>     (ada_val_print_1): Pass address parameter on to sub-functions.
>     (print_variant_part): Pass address parameter on to sub-functions.
>
>
> diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
> index 4a04d28..6ec7764 100644
> --- a/gdb/ada-valprint.c
> +++ b/gdb/ada-valprint.c
> @@ -34,14 +34,15 @@
>  #include "exceptions.h"
>  #include "objfiles.h"
>  
> -static void print_record (struct type *, const gdb_byte *, int,
> +static void print_record (struct type *, const gdb_byte *,
> +              int, CORE_ADDR,
>                struct ui_file *,
>                int,
>                const struct value *,
>                const struct value_print_options *);
>  
>  static int print_field_values (struct type *, const gdb_byte *,
> -                   int,
> +                   int, CORE_ADDR,
>                     struct ui_file *, int,
>                     const struct value *,
>                     const struct value_print_options *,
> @@ -658,7 +659,7 @@ ada_val_print_1 (struct type *type, const gdb_byte
> *valaddr,
>        struct value *mark = value_mark ();
>        struct value *val;
>  
> -      val = value_from_contents_and_address (type, valaddr + offset,
> address);
> +      val = value_from_contents_and_address (type, valaddr + offset,
> address + offset);
>        /* If this is a reference, coerce it now.  This helps taking care
>       of the case where ADDRESS is meaningless because original_value
>       was not an lval.  */
> @@ -732,7 +733,7 @@ ada_val_print_1 (struct type *type, const gdb_byte
> *valaddr,
>               nonsense value.  Actually, we could use the same
>               code regardless of lengths; I'm just avoiding a cast.  */
>            struct value *v1
> -        = value_from_contents_and_address (type, valaddr + offset, 0);
> +        = value_from_contents_and_address (type, valaddr + offset,
> address + offset);
>            struct value *v = value_cast (target_type, v1);
>  
>            ada_val_print_1 (target_type,
> @@ -850,7 +851,7 @@ ada_val_print_1 (struct type *type, const gdb_byte
> *valaddr,
>      }
>        else
>      {
> -      print_record (type, valaddr, offset_aligned,
> +      print_record (type, valaddr, offset_aligned, address,
>              stream, recurse, original_value, options);
>        return;
>      }
> @@ -916,6 +917,7 @@ ada_val_print_1 (struct type *type, const gdb_byte
> *valaddr,
>  static int
>  print_variant_part (struct type *type, int field_num,
>              const gdb_byte *valaddr, int offset,
> +            CORE_ADDR address,
>              struct ui_file *stream, int recurse,
>              const struct value *val,
>              const struct value_print_options *options,
> @@ -934,7 +936,7 @@ print_variant_part (struct type *type, int field_num,
>         valaddr,
>         offset + TYPE_FIELD_BITPOS (type, field_num) / HOST_CHAR_BIT
>         + TYPE_FIELD_BITPOS (var_type, which) / HOST_CHAR_BIT,
> -       stream, recurse, val, options,
> +       address, stream, recurse, val, options,
>         comma_needed, outer_type, outer_offset);
>  }
>  
> @@ -990,7 +992,7 @@ ada_value_print (struct value *val0, struct
> ui_file *stream,
>  
>  static void
>  print_record (struct type *type, const gdb_byte *valaddr,
> -          int offset,
> +          int offset, CORE_ADDR address,
>            struct ui_file *stream, int recurse,
>            const struct value *val,
>            const struct value_print_options *options)
> @@ -999,7 +1001,7 @@ print_record (struct type *type, const gdb_byte
> *valaddr,
>  
>    fprintf_filtered (stream, "(");
>  
> -  if (print_field_values (type, valaddr, offset,
> +  if (print_field_values (type, valaddr, offset, address,
>                stream, recurse, val, options,
>                0, type, offset) != 0 && options->prettyformat)
>      {
> @@ -1027,7 +1029,8 @@ print_record (struct type *type, const gdb_byte
> *valaddr,
>  
>  static int
>  print_field_values (struct type *type, const gdb_byte *valaddr,
> -            int offset, struct ui_file *stream, int recurse,
> +            int offset, CORE_ADDR address,
> +            struct ui_file *stream, int recurse,
>              const struct value *val,
>              const struct value_print_options *options,
>              int comma_needed,
> @@ -1049,7 +1052,7 @@ print_field_values (struct type *type, const
> gdb_byte *valaddr,
>                  valaddr,
>                  (offset
>                   + TYPE_FIELD_BITPOS (type, i) / HOST_CHAR_BIT),
> -                stream, recurse, val, options,
> +                address, stream, recurse, val, options,
>                  comma_needed, type, offset);
>        continue;
>      }
> @@ -1057,7 +1060,7 @@ print_field_values (struct type *type, const
> gdb_byte *valaddr,
>      {
>        comma_needed =
>          print_variant_part (type, i, valaddr,
> -                offset, stream, recurse, val,
> +                offset, address, stream, recurse, val,
>                  options, comma_needed,
>                  outer_type, outer_offset);
>        continue;
> @@ -1111,7 +1114,7 @@ print_field_values (struct type *type, const
> gdb_byte *valaddr,
>            opts.deref_ref = 0;
>            val_print (TYPE_FIELD_TYPE (type, i),
>               value_contents_for_printing (v),
> -             value_embedded_offset (v), 0,
> +             value_embedded_offset (v), address,
>               stream, recurse + 1, v,
>               &opts, current_language);
>          }
> @@ -1125,7 +1128,7 @@ print_field_values (struct type *type, const
> gdb_byte *valaddr,
>               valaddr,
>               (offset
>                + TYPE_FIELD_BITPOS (type, i) / HOST_CHAR_BIT),
> -             0, stream, recurse + 1, val, &opts);
> +             address, stream, recurse + 1, val, &opts);
>      }
>        annotate_field_end ();
>      }
>
>
>
>
>