[v2,06/11] Add a more general version of lookup_struct_elt_type.

  lookup_struct_elt is a new function which returns a tuple of
information about a component of a structure or union.  The returned
tuple contains a pointer to the struct field object for the component
as well as a bit offset of that field within the structure.  If the
field names a field in an anonymous substructure, the offset is the
"global" offset relative to the original structure type.  If noerr is
set, then the returned tuple will set the field pointer to NULL to
indicate a missing component rather than throwing an error.

lookup_struct_elt_type is now reimplemented in terms of this new
function.  It simply returns the type of the returned field.

gdb/ChangeLog:

	* gdbtypes.c (lookup_struct_elt): New function.
	(lookup_struct_elt_type): Reimplement via lookup_struct_elt.
	* gdbtypes.h (struct struct_elt): New type.
	(lookup_struct_elt): New prototype.
---
 gdb/ChangeLog  |  7 ++++++
 gdb/gdbtypes.c | 60 ++++++++++++++++++++++++++++++++------------------
 gdb/gdbtypes.h | 19 ++++++++++++++++
 3 files changed, 65 insertions(+), 21 deletions(-)
  

On 2/8/19 4:40 PM, John Baldwin wrote:
> lookup_struct_elt is a new function which returns a tuple of
> information about a component of a structure or union.  The returned
> tuple contains a pointer to the struct field object for the component
> as well as a bit offset of that field within the structure.  If the
> field names a field in an anonymous substructure, the offset is the
> "global" offset relative to the original structure type.  If noerr is
> set, then the returned tuple will set the field pointer to NULL to
> indicate a missing component rather than throwing an error.
> 
> lookup_struct_elt_type is now reimplemented in terms of this new
> function.  It simply returns the type of the returned field.

Hopefully this is close enough to lk_find_field that you can reuse it.
One difference is that it defines its own dedicated type and the second is
that it returns the raw bitpos so that it is hopefully easier to reuse in
other places.  I think you can probably call it and just pass the members
the returned structure (with an added divide for the offset to convert to
bytes) to construct an lk_symbol.
  

Hey Jon

On Fri, 8 Feb 2019 17:07:22 -0800
John Baldwin <jhb@FreeBSD.org> wrote:

> On 2/8/19 4:40 PM, John Baldwin wrote:
> > lookup_struct_elt is a new function which returns a tuple of
> > information about a component of a structure or union.  The returned
> > tuple contains a pointer to the struct field object for the component
> > as well as a bit offset of that field within the structure.  If the
> > field names a field in an anonymous substructure, the offset is the
> > "global" offset relative to the original structure type.  If noerr is
> > set, then the returned tuple will set the field pointer to NULL to
> > indicate a missing component rather than throwing an error.
> > 
> > lookup_struct_elt_type is now reimplemented in terms of this new
> > function.  It simply returns the type of the returned field.  
> 
> Hopefully this is close enough to lk_find_field that you can reuse it.
> One difference is that it defines its own dedicated type and the second is
> that it returns the raw bitpos so that it is hopefully easier to reuse in
> other places.  I think you can probably call it and just pass the members
> the returned structure (with an added divide for the offset to convert to
> bytes) to construct an lk_symbol.

sorry, I totally missed your v1.

The patch looks sane to me. It should be possible to use it in lk_find_field.
I'm not fully sure what the 'check on baseclasses' does for C structs, but I
guess it doesn't harm. Otherwise there would have already been an outcry :)

Thanks
Philipp
  

On 2/11/19 2:27 AM, Philipp Rudo wrote:
> Hey Jon
> 
> On Fri, 8 Feb 2019 17:07:22 -0800
> John Baldwin <jhb@FreeBSD.org> wrote:
> 
>> On 2/8/19 4:40 PM, John Baldwin wrote:
>>> lookup_struct_elt is a new function which returns a tuple of
>>> information about a component of a structure or union.  The returned
>>> tuple contains a pointer to the struct field object for the component
>>> as well as a bit offset of that field within the structure.  If the
>>> field names a field in an anonymous substructure, the offset is the
>>> "global" offset relative to the original structure type.  If noerr is
>>> set, then the returned tuple will set the field pointer to NULL to
>>> indicate a missing component rather than throwing an error.
>>>
>>> lookup_struct_elt_type is now reimplemented in terms of this new
>>> function.  It simply returns the type of the returned field.  
>>
>> Hopefully this is close enough to lk_find_field that you can reuse it.
>> One difference is that it defines its own dedicated type and the second is
>> that it returns the raw bitpos so that it is hopefully easier to reuse in
>> other places.  I think you can probably call it and just pass the members
>> the returned structure (with an added divide for the offset to convert to
>> bytes) to construct an lk_symbol.
> 
> sorry, I totally missed your v1.

Oh, this patch wasn't in the v1 is why.  I was using parse_and_eval_long
with hand-coded offsetof equivalents previously.

> The patch looks sane to me. It should be possible to use it in lk_find_field.
> I'm not fully sure what the 'check on baseclasses' does for C structs, but I
> guess it doesn't harm. Otherwise there would have already been an outcry :)

Ok.
  

On 2019-02-08 7:40 p.m., John Baldwin wrote:
> lookup_struct_elt is a new function which returns a tuple of
> information about a component of a structure or union.  The returned
> tuple contains a pointer to the struct field object for the component
> as well as a bit offset of that field within the structure.  If the
> field names a field in an anonymous substructure, the offset is the
> "global" offset relative to the original structure type.

Can you add this bit of detail (about anonymous struct) to the function doc?

> If noerr is
> set, then the returned tuple will set the field pointer to NULL to
> indicate a missing component rather than throwing an error.
> 
> lookup_struct_elt_type is now reimplemented in terms of this new
> function.  It simply returns the type of the returned field.
> 
> gdb/ChangeLog:
> 
> 	* gdbtypes.c (lookup_struct_elt): New function.
> 	(lookup_struct_elt_type): Reimplement via lookup_struct_elt.
> 	* gdbtypes.h (struct struct_elt): New type.
> 	(lookup_struct_elt): New prototype.
> ---
>   gdb/ChangeLog  |  7 ++++++
>   gdb/gdbtypes.c | 60 ++++++++++++++++++++++++++++++++------------------
>   gdb/gdbtypes.h | 19 ++++++++++++++++
>   3 files changed, 65 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index cebd63bcb5..c7fee7eb11 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2019-02-08  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* gdbtypes.c (lookup_struct_elt): New function.
> +	(lookup_struct_elt_type): Reimplement via lookup_struct_elt.
> +	* gdbtypes.h (struct struct_elt): New type.
> +	(lookup_struct_elt): New prototype.
> +
>   2019-02-08  John Baldwin  <jhb@FreeBSD.org>
>   
>   	* gdbtypes.c (lookup_struct_elt_type): Update comment and
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index e4acb0e985..0f3a450f9f 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1644,7 +1644,8 @@ lookup_template_type (char *name, struct type *type,
>     return (SYMBOL_TYPE (sym));
>   }
>   
> -/* Given a type TYPE, lookup the type of the component named NAME.
> +/* Given a type TYPE, lookup the field and offset of the component named
> +   NAME.
>   
>      TYPE can be either a struct or union, or a pointer or reference to
>      a struct or union.  If it is a pointer or reference, its target
> @@ -1652,11 +1653,11 @@ lookup_template_type (char *name, struct type *type,
>      as specified for the definitions of the expression element types
>      STRUCTOP_STRUCT and STRUCTOP_PTR.
>   
> -   If NOERR is nonzero, return NULL if there is no component named
> -   NAME.  */
> +   If NOERR is nonzero, the returned structure will have field set to
> +   NULL if there is no component named NAME.  */

For both functions, please move the comment to the .h, and add /* See 
gdbtypes.h.  */ here.

>   
> -struct type *
> -lookup_struct_elt_type (struct type *type, const char *name, int noerr)
> +struct_elt
> +lookup_struct_elt (struct type *type, const char *name, int noerr) >   {
>     int i;
>   
> @@ -1683,39 +1684,56 @@ lookup_struct_elt_type (struct type *type, const char *name, int noerr)
>   
>         if (t_field_name && (strcmp_iw (t_field_name, name) == 0))
>   	{
> -	  return TYPE_FIELD_TYPE (type, i);
> +	  return struct_elt (&TYPE_FIELD(type, i), TYPE_FIELD_BITPOS (type, i));
>   	}
>        else if (!t_field_name || *t_field_name == '\0')
>   	{
> -	  struct type *subtype
> -	    = lookup_struct_elt_type (TYPE_FIELD_TYPE (type, i), name, 1);
> -
> -	  if (subtype != NULL)
> -	    return subtype;
> +	  struct_elt elt
> +	    = lookup_struct_elt (TYPE_FIELD_TYPE (type, i), name, 1);
> +	  if (elt.field != NULL)
> +	    {
> +	      elt.offset += TYPE_FIELD_BITPOS (type, i);
> +	      return elt;
> +	    }
>   	}
>       }
>   
>     /* OK, it's not in this class.  Recursively check the baseclasses.  */
>     for (i = TYPE_N_BASECLASSES (type) - 1; i >= 0; i--)
>       {
> -      struct type *t;
> -
> -      t = lookup_struct_elt_type (TYPE_BASECLASS (type, i), name, 1);
> -      if (t != NULL)
> -	{
> -	  return t;
> -	}
> +      struct_elt elt = lookup_struct_elt (TYPE_BASECLASS (type, i), name, 1);
> +      if (elt.field != NULL)
> +	return elt;
>       }
>   
>     if (noerr)
> -    {
> -      return NULL;
> -    }
> +      return struct_elt ();
>   
>     std::string type_name = type_to_string (type);
>     error (_("Type %s has no component named %s."), type_name.c_str (), name);
>   }
>   
> +/* Given a type TYPE, lookup the type of the component named NAME.
> +
> +   TYPE can be either a struct or union, or a pointer or reference to
> +   a struct or union.  If it is a pointer or reference, its target
> +   type is automatically used.  Thus '.' and '->' are interchangable,
> +   as specified for the definitions of the expression element types
> +   STRUCTOP_STRUCT and STRUCTOP_PTR.
> +
> +   If NOERR is nonzero, return NULL if there is no component named
> +   NAME.  */
> +
> +struct type *
> +lookup_struct_elt_type (struct type *type, const char *name, int noerr)
> +{
> +  struct_elt elt = lookup_struct_elt (type, name, noerr);
> +  if (elt.field != NULL)
> +    return FIELD_TYPE (*elt.field);
> +  else
> +    return NULL;
> +}
> +
>   /* Store in *MAX the largest number representable by unsigned integer type
>      TYPE.  */
>   
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index a6d4f64e9b..894c7b2fc6 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1873,6 +1873,25 @@ extern struct type *allocate_stub_method (struct type *);
>   
>   extern const char *type_name_or_error (struct type *type);
>   
> +struct struct_elt
> +{
> +  /* The field of the element, or NULL if no element was found.  */
> +  struct field *field;
> +
> +  /* The bit offset of the element in the parent structure.  */
> +  LONGEST offset;
> +
> +  struct_elt ()
> +    : field (nullptr), offset (0)
> +  {}
> +
> +  struct_elt (struct field *field, LONGEST offset)
> +    : field (field), offset (offset)
> +  {}
> +};

Not really a big deal, but I find it a bit overkill to define a type 
just for this, I would have probably just returned the the offset as an 
output parameter.  But maybe this way is better, in that if we want to 
add something to the return type, we don't have to update the callers.

So the patch LGTM with the nits noted above.

Simon
  

On 3/7/19 7:53 AM, Simon Marchi wrote:
> Not really a big deal, but I find it a bit overkill to define a type 
> just for this, I would have probably just returned the the offset as an 
> output parameter.  But maybe this way is better, in that if we want to 
> add something to the return type, we don't have to update the callers.

Honestly, I just modeled this after the similar code in the Linux
kernel thread patches.  Using a reference parameter for the offset
would be fine and I don't mind making that change.  I don't think that
we are going to add more things to that structure in the future.  The
field generally has everything else you want to know other than the
offset.
  

On 03/08/2019 12:04 AM, John Baldwin wrote:
> On 3/7/19 7:53 AM, Simon Marchi wrote:
>> Not really a big deal, but I find it a bit overkill to define a type 
>> just for this, I would have probably just returned the the offset as an 
>> output parameter.  But maybe this way is better, in that if we want to 
>> add something to the return type, we don't have to update the callers.
> 
> Honestly, I just modeled this after the similar code in the Linux
> kernel thread patches.  Using a reference parameter for the offset
> would be fine and I don't mind making that change.  I don't think that
> we are going to add more things to that structure in the future.  The
> field generally has everything else you want to know other than the
> offset.
> 

Note that you can keep the struct with just the data fields
and no ctor boilerplate if you use brace initialization.

I.e., with just:

struct struct_elt
{
  /* The field of the element, or NULL if no element was found.  */
  struct field *field;

  /* The bit offset of the element in the parent structure.  */
  LONGEST offset;
};

You write:

 return {nullptr, 0};
 return {&TYPE_FIELD (type, i), TYPE_FIELD_BITPOS (type, i)};

Instead of:

 return struct_elt ();
 return struct_elt (&TYPE_FIELD (type, i), TYPE_FIELD_BITPOS (type, i));




BTW, I noticed the missing space before parens below:

 > -	  return TYPE_FIELD_TYPE (type, i);
 > +	  return struct_elt (&TYPE_FIELD(type, i), TYPE_FIELD_BITPOS (type, i));

Thanks,
Pedro Alves
  

On 3/7/19 4:32 PM, Pedro Alves wrote:
> On 03/08/2019 12:04 AM, John Baldwin wrote:
>> On 3/7/19 7:53 AM, Simon Marchi wrote:
>>> Not really a big deal, but I find it a bit overkill to define a type 
>>> just for this, I would have probably just returned the the offset as an 
>>> output parameter.  But maybe this way is better, in that if we want to 
>>> add something to the return type, we don't have to update the callers.
>>
>> Honestly, I just modeled this after the similar code in the Linux
>> kernel thread patches.  Using a reference parameter for the offset
>> would be fine and I don't mind making that change.  I don't think that
>> we are going to add more things to that structure in the future.  The
>> field generally has everything else you want to know other than the
>> offset.
>>
> 
> Note that you can keep the struct with just the data fields
> and no ctor boilerplate if you use brace initialization.

Thanks, I've made this change and also fixed the missing space.