This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Martin,

This looks generally good.  I only have minor comments below.
Once those are resolved, this should be good to go.  Please
fix them and resubmit.

Many thanks again for following through with the tests, which is
often not-that-fun to write, but is where I think of a lot of
the value is.

On 05/18/2016 03:47 PM, Martin Galvan wrote:

> 	* gdb.dwarf2/implref.exp: Rename to...
> 	* gdb.dwarf2/implref-const.exp: ...this.

Were there changes other than renaming?  Please tweak
your git config settings to detect renames / do "git diff -M".
E.g., I have:

[diff]
        renames = true

> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index bfe1173..ecd7cc4 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -2061,6 +2061,66 @@ get_frame_address_in_block_wrapper (void *baton)
>    return get_frame_address_in_block ((struct frame_info *) baton);
>  }
>  
> +static struct value *
> +fetch_const_value_from_synthetic_pointer (sect_offset die, LONGEST byte_offset,
> +					  struct dwarf2_per_cu_data *per_cu,
> +					  struct type *type)

Missing intro comment, which should leave no doubt on why is the function
named "const value", but then returns a non-const value.

> +{
> +  struct value *result = NULL;
> +  struct obstack temp_obstack;
> +  struct cleanup *cleanup;
> +  const gdb_byte *bytes;
> +  LONGEST len;

Empty line after decls.

> +  obstack_init (&temp_obstack);
> +  cleanup = make_cleanup_obstack_free (&temp_obstack);
> +  bytes = dwarf2_fetch_constant_bytes (die, per_cu, &temp_obstack, &len);
> +
> +  if (bytes != NULL)
> +    {
> +      if (byte_offset >= 0
> +	  && byte_offset + TYPE_LENGTH (TYPE_TARGET_TYPE (type)) <= len)
> +	{
> +	  bytes += byte_offset;
> +	  result = value_from_contents (TYPE_TARGET_TYPE (type), bytes);
> +	}
> +      else
> +	invalid_synthetic_pointer ();
> +    }
> +  else
> +    result = allocate_optimized_out_value (TYPE_TARGET_TYPE (type));
> +
> +  do_cleanups (cleanup);
> +
> +  return result;
> +}
> +
> +/* Fetch the value pointed at by a synthetic pointer.  */
> +static struct value *

Empty line between comment and function.

> +indirect_synthetic_pointer (sect_offset die, LONGEST byte_offset,
> +			    struct dwarf2_per_cu_data *per_cu,
> +			    struct frame_info *frame, struct type *type)
> +{
> +  struct value *result = NULL;

No need to initialize if you're supposed to always set
it below.  Let the compiler warn if something changes and
we miss initializing in some case.

> +
> +  /* Fetch the location expression of the DIE we're pointing to.  */
> +  struct dwarf2_locexpr_baton baton
> +    = dwarf2_fetch_die_loc_sect_off (die, per_cu,
> +				     get_frame_address_in_block_wrapper, frame);
> +
> +  /* If pointed-to DIE has a DW_AT_location, evaluate it and return the
> +     resulting value.  Otherwise, it may have a DW_AT_const_value instead,
> +     or it may've been optimized out.  */
> +  if (baton.data != NULL)
> +    result = dwarf2_evaluate_loc_desc_full (TYPE_TARGET_TYPE (type), frame,
> +					    baton.data, baton.size,
> +					    baton.per_cu, byte_offset);
> +  else
> +    result = fetch_const_value_from_synthetic_pointer (die, byte_offset, per_cu,
> +						       type);
> +
> +    return result;

This "return" seems incorrectly indented.

But you can actually remove the "result" local and call "return" directly
in both branches.

> +}
> +
>  /* An implementation of an lval_funcs method to indirect through a
>     pointer.  This handles the synthetic pointer case when needed.  */
>  
> @@ -2115,6 +2175,7 @@ indirect_pieced_value (struct value *value)
>        break;
>      }
>  
> +  gdb_assert (piece);

Please make this:

  gdb_assert (piece != NULL);

while at it.


>    frame = get_selected_frame (_("No frame selected."));
>  
>    /* This is an offset requested by GDB, such as value subscripts.
> @@ -2132,43 +2193,35 @@ indirect_pieced_value (struct value *value)
>  					TYPE_LENGTH (type), byte_order);
>    byte_offset += piece->v.ptr.offset;
>  
> -  gdb_assert (piece);
> -  baton
> -    = dwarf2_fetch_die_loc_sect_off (piece->v.ptr.die, c->per_cu,
> -				     get_frame_address_in_block_wrapper,
> -				     frame);
> +  return indirect_synthetic_pointer (piece->v.ptr.die, byte_offset, c->per_cu,
> +				     frame, type);
> +}
>  
> -  if (baton.data != NULL)
> -    return dwarf2_evaluate_loc_desc_full (TYPE_TARGET_TYPE (type), frame,
> -					  baton.data, baton.size, baton.per_cu,
> -					  byte_offset);
> -
> -  {
> -    struct obstack temp_obstack;
> -    struct cleanup *cleanup;
> -    const gdb_byte *bytes;
> -    LONGEST len;
> -    struct value *result;
> -
> -    obstack_init (&temp_obstack);
> -    cleanup = make_cleanup_obstack_free (&temp_obstack);
> -
> -    bytes = dwarf2_fetch_constant_bytes (piece->v.ptr.die, c->per_cu,
> -					 &temp_obstack, &len);
> -    if (bytes == NULL)
> -      result = allocate_optimized_out_value (TYPE_TARGET_TYPE (type));
> -    else
> -      {
> -	if (byte_offset < 0
> -	    || byte_offset + TYPE_LENGTH (TYPE_TARGET_TYPE (type)) > len)
> -	  invalid_synthetic_pointer ();
> -	bytes += byte_offset;
> -	result = value_from_contents (TYPE_TARGET_TYPE (type), bytes);
> -      }
> +static struct value *coerce_pieced_ref (const struct value *value)
> +{

Line break before "coerce".  Missing intro comment.

> +  struct value *result = NULL;
> +  struct type *type = check_typedef (value_type (value));
>  
> -    do_cleanups (cleanup);
> -    return result;
> -  }
> +  if (value_bits_synthetic_pointer (value, value_embedded_offset (value),
> +				    TARGET_CHAR_BIT * TYPE_LENGTH (type)))
> +    {
> +      const struct piece_closure *closure
> +	= (struct piece_closure *) value_computed_closure (value);
> +
> +      /* gdb represents synthetic pointers as pieced values with a single
> +       piece.  */
> +      gdb_assert (closure != NULL);
> +      gdb_assert (closure->n_pieces == 1);
> +
> +      struct frame_info *frame = get_selected_frame (_("No frame selected."));

Declarations in the middle of a scope break the C build.  Declare "frame"
next to "closure".

> +
> +      result = indirect_synthetic_pointer (closure->pieces->v.ptr.die,
> +					   closure->pieces->v.ptr.offset,
> +					   closure->per_cu, frame, type);
> +    }
> +  /* Else: not a synthetic reference; do nothing.  */
> +
> +  return result;

Likewise here, you can avoid the odd dangling else comment
by removing the "result" variable, and write

   if (value_bits_synthetic_pointer (value, value_embedded_offset (value),
				    TARGET_CHAR_BIT * TYPE_LENGTH (type)))
    {
      ...
      return indirect_synthetic_pointer (closure->pieces->v.ptr.die,
				         closure->pieces->v.ptr.offset,
					 closure->per_cu, frame, type);
    }
   else
     {
       /* Not a synthetic reference; do nothing.  */
       return NULL;
     }
>  
>  static void *



> +# We need to know the size of integer and address types in order
> +# to write some of the debugging info we'd like to generate.
> +#
> +# For that, we ask GDB by debugging our implref-array program.
> +# Any program would do, but since we already have implref-array
> +# specifically for this testcase, might as well use that.
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
> +    untested ${testfile}.exp
> +    return -1
> +}
> +
> +set array_length [get_valueof "/u" "sizeof(array) / sizeof(array\[0\])" 5]

Replace the hard coded fallback "5" with -1 or 0 or some such, so that
we're not actually relying on the fallback.  Happens in more places.

> +if [prepare_for_testing ${testfile}.exp ${executable} [list ${asm_file} ${srcfile}] {}] {
> +    return -1
> +}
> +
> +# DW_OP_GNU_implicit_pointer implementation requires a valid frame.
> +if ![runto_main] {
> +    return -1
> +}
> +
> +# This matches e.g. '(int (&)[5])'
> +set ref_type [format {\(int \(&\)\[%d\]\)} ${array_length}]
> +
> +# This matches e.g. '(int (*)[5])'
> +set ptr_type [format {\(int \(\*\)\[%d\]\)} ${array_length}]
> +
> +# Contents of the array.  Trim leading/trailing whitespace, '{' and '}'
> +# since they confuse TCL to no end.
> +set contents [get_valueof "" "array" "\{0, 1, 2, 3, 4\}"]

Ditto.

> +set contents [string trim ${contents}]
> +set contents [string trim ${contents} "{}"]
> +
> +# Doing 'print ref' should show us e.g. '(int (&)[5]) <synthetic pointer>: {0, 1, 2, 3, 4}'.
> +gdb_test "print ref" " = ${ref_type} <synthetic pointer>: \\{${contents}\\}" "print ref"
> +
> +# Doing 'print &ref' should show us e.g. '(int (*)[5]) 0xdeadbeef <array>'.
> +gdb_test "print &ref" " = ${ptr_type} \[\[:xdigit:\]x\]+ <array>" "print &ref"

No need to spell out a test message (last parameter to gdb_test above) when it's exactly
the same as the command (first) parameter.

> +
> +# gdb assumes C++ references are implemented as pointers, and print &(&ref)
> +# shows us the underlying pointer's address.  Since in this case there's no
> +# physical pointer, gdb should tell us so.
> +gdb_test "print &(&ref)" "Attempt to take address of value not located in memory." "print &(&ref)"
> +

Ditto.  Happens in more places.

> +# Test assignment through the synthetic reference.
> +set first_value 10
> +gdb_test_no_output "set (ref\[0\] = ${first_value})" "set (ref\[0\] = ${first_value})"
> +
> +# This matches e.g. '{10, 1, 2, 3, 4}'
> +set new_contents [format {\{%d[\d,\s]+\}} ${first_value}]

Why not match exactly "{10, 1, 2, 3, 4}" ?

> +
> +# Doing 'print ref' should now show us e.g.
> +# '(int (&)[5]) <synthetic pointer>: {10, 1, 2, 3, 4}'.
> +gdb_test "print ref" " = ${ref_type} <synthetic pointer>: ${new_contents}" "print ref (after assignment)"
> +gdb_test "print array" " = ${new_contents}" "print array (after assignment)"

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

> +
> +# Test treating the array as a pointer.
> +set second_value 20
> +set new_contents [format {\{%d, %d[\d,\s]+\}} ${first_value} ${second_value}]
> +
> +gdb_test "print *ref" " = ${first_value}" "print *ref"
> +gdb_test_no_output "set (*(ref + 1) = ${second_value})" "set (*(ref + 1) = ${second_value})"
> +gdb_test "print ref\[1\]" " = ${second_value}" "print ref\[1\]"

Remove third param.

> +gdb_test "print array" " = ${new_contents}" "print array (after second assignment)"

See above.

The tests files are all similar, so the same comments apply to all of them.

> diff --git a/gdb/testsuite/gdb.dwarf2/implref-const.exp b/gdb/testsuite/gdb.dwarf2/implref-const.exp


> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/implref-struct.c
> @@ -0,0 +1,43 @@
> +/* Copyright (C) 2016 Free Software Foundation, Inc.


> +/* Test program for synthetic C++ references to structs.  */
> +
> +struct S {
> +  int a;
> +  int b;
> +  int c;
> +};
> +
> +struct S s1 = {
> +  .a = 0,
> +  .b = 1,
> +  .c = 2

Guess this file is compiling as a C program.  I think
designated initializers are not in C++.  Please remove
them for portability, since the test does not seem
to require them.

> +};
> +
> +struct S s2 = {
> +  .a = 10,
> +  .b = 11,
> +  .c = 12
> +};
> +

> @@ -497,13 +499,29 @@ generic_val_print_ref (struct type *type, const gdb_byte *valaddr,
>  
>    if (options->addressprint)
>      {
> -      CORE_ADDR addr
> -	= extract_typed_address (valaddr + embedded_offset, type);
> +      const int is_synthetic =

= goes on the next line.

> +	value_bits_synthetic_pointer (original_value,
> +				      TARGET_CHAR_BIT * embedded_offset,
> +				      TARGET_CHAR_BIT * TYPE_LENGTH (type));
> +      if (!is_synthetic)
> +	{
> +	  CORE_ADDR addr
> +	    = extract_typed_address (valaddr + embedded_offset, type);
>  
> -      fprintf_filtered (stream, "@");
> -      fputs_filtered (paddress (gdbarch, addr), stream);
> -      if (options->deref_ref)
> -	fputs_filtered (": ", stream);
> +	  fprintf_filtered (stream, "@");
> +	  fputs_filtered (paddress (gdbarch, addr), stream);
> +	}
> +      /* Else, we have a synthetic reference.  Don't print '@address'; we'll
> +	 show '<synthetic pointer>' instead through valprint_check_validity.
> +	 Notice however that 'set print object on' will still show '@address'.

Is that a bug, or desirable?

> +	 This happens because value_bits_synthetic_pointer returns false if
> +	 original value is not 'lval_computed'.  While that's usually the case,
> +	 if options->objectprint is true, c_value_print will call value_addr
> +	 on the reference, which coerces synthetic references and returns a
> +	 'not_lval'.  */
> +
> +	if (options->deref_ref)
> +	  fputs_filtered (": ", stream);
>      }
>    /* De-reference the reference.  */
>    if (options->deref_ref)
> 

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]