Re: [PATCH v3][PR gdb/19893] Fix handling of synthetic C++ references

[Pedro Alves <palves@redhat.com>]

On 05/30/2016 08:40 PM, Martin Galvan wrote:

>  
>        if (!value_bytes_available (val, embedded_offset, TYPE_LENGTH (type)))
> @@ -484,6 +498,42 @@ generic_val_print_memberptr (struct type *type, const gdb_byte *valaddr,
>  			      original_value, options, 0, stream);
>  }
>  
> +/* Print '@' followed by the address contained in ADDRESS_BUFFER.  */
> +
> +static void
> +print_ref_address(struct type *type, const gdb_byte *address_buffer,
> +		  int embedded_offset, struct ui_file *stream)

Space before parenthesis.

> +{
> +  struct gdbarch *gdbarch = get_type_arch (type);
> +
> +  if (address_buffer != NULL)
> +    {
> +      CORE_ADDR address
> +	= extract_typed_address (address_buffer + embedded_offset, type);
> +
> +      fprintf_filtered (stream, "@");
> +      fputs_filtered (paddress (gdbarch, address), stream);
> +    }
> +  /* Else: we have a non-addressable value, such as a DW_AT_const_value.  */
> +}
> +
> +/* Return the address of the value represented by DEREF_VAL, or NULL if it's
> +   non-addressable.  */
> +
> +static const gdb_byte *
> +get_address_from_deref_val (struct value *deref_val)

This is returning the value contents of the pointer value, not the
value's address.  I think it's a confusing interface.  How about:

/* If VAL is addressable, return the value contents buffer of
   a value that represents a pointer to VAL.  Otherwise
   return NULL.  */

static const gdb_byte *
get_value_addr_contents (struct value *val)

>  static void
> @@ -492,41 +542,58 @@ generic_val_print_ref (struct type *type, const gdb_byte *valaddr,
>  		       const struct value *original_value,
>  		       const struct value_print_options *options)
>  {
> -  struct gdbarch *gdbarch = get_type_arch (type);
>    struct type *elttype = check_typedef (TYPE_TARGET_TYPE (type));
> +  struct value *deref_val = NULL;
> +  const gdb_byte *address;
> +  const int value_is_synthetic
> +    = value_bits_synthetic_pointer (original_value,
> +				    TARGET_CHAR_BIT * embedded_offset,
> +				    TARGET_CHAR_BIT * TYPE_LENGTH (type));
> +  const int must_coerce_ref = (options->addressprint && value_is_synthetic)
> +			      || options->deref_ref;

If the expression is split between lines, you need to wrap it in
an extra set of parens, and reindent:

  const int must_coerce_ref = ((options->addressprint && value_is_synthetic)
			       || options->deref_ref);


> +  const int type_is_defined = TYPE_CODE (elttype) != TYPE_CODE_UNDEF;
> +
> +  if (must_coerce_ref && type_is_defined)
> +    {
> +      deref_val = coerce_ref_if_computed (original_value);
> +
> +      if (deref_val != NULL)
> +	{
> +	  /* More complicated computed references are not supported.  */
> +	  gdb_assert (embedded_offset == 0);
> +	}
> +      else
> +	deref_val = value_at (TYPE_TARGET_TYPE (type),
> +			      unpack_pointer (type, valaddr + embedded_offset));
> +    }
> +  /* Else, original_value isn't a synthetic reference or we don't have to print
> +     the reference's contents.
> +
> +     Notice that for references to TYPE_CODE_STRUCT, 'set print object on' will
> +     cause original_value to be a not_lval instead of an lval_computed,
> +     which will make value_bits_synthetic_pointer return false.
> +     This happens because if options->objectprint is true, c_value_print will
> +     overwrite original_value's contents with the result of coercing
> +     the reference through value_addr, and then set its type back to
> +     TYPE_CODE_REF.  In that case we don't have to coerce the reference again;
> +     we can simply treat it as non-synthetic and move on.  */
>  
>    if (options->addressprint)
>      {
> -      CORE_ADDR addr
> -	= extract_typed_address (valaddr + embedded_offset, type);
> +      address = value_is_synthetic && type_is_defined ?
> +		get_address_from_deref_val (deref_val) : valaddr;

Operators go on next line, not at end of line, and wrap in parens:

      address = (value_is_synthetic && type_is_defined 
                 ? get_address_from_deref_val (deref_val) 
	         : valaddr);

Otherwise OK.  Please push.

Thanks,
Pedro Alves