From: Simon Marchi <simark@simark.ca>
To: John Baldwin <jhb@FreeBSD.org>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2 06/11] Add a more general version of lookup_struct_elt_type.
Date: Thu, 07 Mar 2019 15:53:00 -0000 [thread overview]
Message-ID: <d211b2e7-441e-3874-62a5-1a1291c35fd7@simark.ca> (raw)
In-Reply-To: <9a5a86e3591c8fe6c0fc8efb6151547902a63d3c.1549672588.git.jhb@FreeBSD.org>
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
next prev parent reply other threads:[~2019-03-07 15:53 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-09 0:42 [PATCH v2 00/11] Support for thread-local variables on FreeBSD John Baldwin
2019-02-09 0:42 ` [PATCH v2 07/11] Add a helper function to resolve TLS variable addresses for FreeBSD John Baldwin
2019-03-07 16:18 ` Simon Marchi
2019-02-09 0:42 ` [PATCH v2 08/11] Support TLS variables on FreeBSD/amd64 John Baldwin
2019-02-09 0:42 ` [PATCH v2 01/11] Support the fs_base and gs_base registers on i386 John Baldwin
2019-02-09 0:42 ` [PATCH v2 06/11] Add a more general version of lookup_struct_elt_type John Baldwin
2019-02-09 1:08 ` John Baldwin
2019-02-11 10:27 ` Philipp Rudo
2019-02-11 17:44 ` John Baldwin
2019-03-07 15:53 ` Simon Marchi [this message]
2019-03-08 0:04 ` John Baldwin
2019-03-08 0:32 ` Pedro Alves
2019-03-08 18:39 ` John Baldwin
2019-02-09 0:42 ` [PATCH v2 02/11] Support fs_base and gs_base on FreeBSD/i386 John Baldwin
2019-02-09 0:42 ` [PATCH v2 09/11] Support TLS variables " John Baldwin
2019-02-09 0:42 ` [PATCH v2 03/11] Handle an edge case for minisym TLS variable lookups John Baldwin
2019-02-09 0:42 ` [PATCH v2 11/11] Support TLS variables on FreeBSD/powerpc John Baldwin
2019-03-07 16:26 ` Simon Marchi
2019-02-09 0:50 ` [PATCH v2 05/11] Remove code disabled since at least 1999 from lookup_struct_elt_type John Baldwin
2019-03-07 16:25 ` Simon Marchi
2019-02-09 0:50 ` [PATCH v2 10/11] Support TLS variables on FreeBSD/riscv John Baldwin
2019-02-09 0:50 ` [PATCH v2 04/11] Add a new gdbarch method to resolve the address of TLS variables John Baldwin
2019-03-07 16:08 ` Simon Marchi
2019-03-07 23:50 ` John Baldwin
2019-03-08 2:55 ` Simon Marchi
2019-03-08 18:39 ` John Baldwin
2019-02-22 17:22 ` [PING][PATCH v2 00/11] Support for thread-local variables on FreeBSD John Baldwin
2019-03-12 20:21 ` Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d211b2e7-441e-3874-62a5-1a1291c35fd7@simark.ca \
--to=simark@simark.ca \
--cc=gdb-patches@sourceware.org \
--cc=jhb@FreeBSD.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox