Re: [PATCH] libdw: Add new functions dwarf_getlocation_attr and dwarf_getlocation_die.

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Fri, 30 Aug 2013 23:55:12 +0200, Mark Wielaard wrote:
> DW_OP_implicit_value and DW_OP_GNU_implicit_pointer already had
> their own special accessors (dwarf_getlocation_implicit_value
> and dwarf_getlocation_implicit_pointer), but it seemed consistent
> to include them in the new more generic accessors too.

So dwarf_getlocation_implicit_value and dwarf_getlocation_implicit_pointer can
be deprecated - that is removed from the .h interfaces?  I haven't tried it
but with .symver also one could have only @version symbol without any
@@version symbol:
	dwarf_getlocation_implicit_value(a)ELFUTILS_0.143
	dwarf_getlocation_implicit_pointer(a)ELFUTILS_0.149


> --- a/libdw/dwarf_getlocation.c
> +++ b/libdw/dwarf_getlocation.c
> @@ -95,13 +95,15 @@ loc_compare (const void *p1, const void *p2)
>  /* For each DW_OP_implicit_value, we store a special entry in the cache.
>     This points us directly to the block data for later fetching.  */
>  static void
> -store_implicit_value (Dwarf *dbg, void **cache, Dwarf_Op *op,
> -		      unsigned char *data)
> +store_implicit_value (Dwarf *dbg, void **cache, Dwarf_Op *op)
>  {
>    struct loc_block_s *block = libdw_alloc (dbg, struct loc_block_s,
>  					   sizeof (struct loc_block_s), 1);
> +  const unsigned char *data = (const unsigned char *) op->number2;
> +  Dwarf_Word blength; // Ignored, equal to op->number.
> +  get_uleb128 (blength, data);
>    block->addr = op;
> -  block->data = data + op->number2;
> +  block->data = (unsigned char *) data;
>    block->length = op->number;
>    (void) tsearch (block, cache, loc_compare);
>  }
> @@ -412,11 +414,11 @@ __libdw_intern_expression (Dwarf *dbg, bool other_byte_order,
>  	  if (unlikely (dbg == NULL))
>  	    goto invalid;
>  
> +	  newloc->number2 = (Dwarf_Word) data; /* start of block inc. len.  */
>  	  /* XXX Check size.  */
>  	  get_uleb128 (newloc->number, data); /* Block length.  */
>  	  if (unlikely ((Dwarf_Word) (end_data - data) < newloc->number))
>  	    goto invalid;
> -	  newloc->number2 = data - block->data; /* Relative block offset.  */

I think it breaks ABI.  Dwarf_Op is a public structure.  Layout of Dwarf_Op
for each of the operations is not explicitly documented but there are various
elfutils API details which are not completely documented.

I do not think it is such a concern but I do not want to be a joker.


>  	  data += newloc->number;		/* Skip the block.  */
>  	  break;
>  
> @@ -437,17 +439,20 @@ __libdw_intern_expression (Dwarf *dbg, bool other_byte_order,
>  	  break;
>  
>  	case DW_OP_GNU_const_type:
> -	  /* XXX Check size.  */
> -	  get_uleb128 (newloc->number, data);
> -	  if (unlikely (data >= end_data))
> -	    goto invalid;
> -	  newloc->number2 = *data++; /* Block length.  */
> -	  if (unlikely ((Dwarf_Word) (end_data - data) < newloc->number2))
> -	    goto invalid;
> -	  /* The third operand is relative block offset:
> -		newloc->number3 = data - block->data;
> -	     We don't support this at this point.  */
> -	  data += newloc->number2;		/* Skip the block.  */
> +	  {
> +	    size_t size;
> +
> +	    /* XXX Check size.  */
> +	    get_uleb128 (newloc->number, data);
> +	    if (unlikely (data >= end_data))
> +	      goto invalid;
> +
> +	    newloc->number2 = (Dwarf_Word) data; /* start of block inc. len.  */
> +	    size = *data++;
> +	    if (unlikely ((Dwarf_Word) (end_data - data) < size))
> +	      goto invalid;
> +	    data += size;		/* Skip the block.  */

Likewise.


> +	  }
>  	  break;
>  
>  	default:


Thanks,
Jan