From: Pedro Alves <palves@redhat.com>
To: John Baldwin <jhb@FreeBSD.org>,
gdb-patches@sourceware.org, binutils@sourceware.org
Subject: Re: [PATCH 5/6] Add a new gdbarch method to print a single AUXV entry.
Date: Mon, 20 Jun 2016 23:40:00 -0000 [thread overview]
Message-ID: <17933b55-b3e1-51e3-3494-fa35b8cd71fd@redhat.com> (raw)
In-Reply-To: <20160616060202.63470-6-jhb@FreeBSD.org>
On 06/16/2016 07:02 AM, John Baldwin wrote:
> Different platforms have different meanings for auxiliary vector
> entries. The 'print_auxv' gdbarch method allows an architecture
> to output a suitable description for platform-specific entries.
>
> A fprint_single_auxv function is split out of fprint_target_auxv.
> This function outputs the description of a single auxiliary vector
> entry to the specified file using caller-supplied formatting and
> strings to describe the vector type.
>
> The existing switch on auxiliary vector types is moved out of
> fprint_target_auxv into a new default_print_auxv function.
> default_print_auxv chooses an appropriate format and description
> and calls fprint_single_auxv to describe a single vector entry.
>
> fprint_target_auxv now invokes the gdbarch 'print_auxv' function
> on each vector entry. If the function is not present or returns
> zero, default_printf_auxv is called to output a description for
> the vector.
I like the idea. Though, I think we can simplify this. How about:
- make default_print_auxv be the default gdbarch_print_auxv
implementation, in gdbarch.sh.
- make fprint_target_auxv calls gdbarch_print_auxv unconditionally.
- remove the support for gdbarch_print_auxv returning 0. Instead,
implementations that want to defer to default_print_auxv simply
call it directly.
Also, I think it'd be a bit less confusing to rename things like this:
gdbarch_print_auxv -> gdbarch_print_auxv_entry
default_print_auxv -> default_print_auxv_entry
fprint_single_auxv -> fprint_auxv_entry
This way methods that print a single entry are consistently named,
and not so easily confused with methods that print the whole table,
like fprint_target_auxv.
>
>
> +/* Print a description of a single AUXV entry on the specified file. */
> +void
Empty line between comment and function.
> +fprint_single_auxv (struct ui_file *file, const char *name,
> + const char *description, enum auxv_format flavor,
> + CORE_ADDR type, CORE_ADDR val)
> +{
> + fprintf_filtered (file, "%-4s %-20s %-30s ",
> + plongest (type), name, description);
If not translatable, wrap string in () [not _()], in order to
avoid ARI complaints.
> + switch (flavor)
> + {
> + case dec:
> + fprintf_filtered (file, "%s\n", plongest (val));
> + break;
> + case hex:
> + fprintf_filtered (file, "%s\n", paddress (target_gdbarch (), val));
> + break;
> + case str:
> + {
> + struct value_print_options opts;
> +
> + get_user_print_options (&opts);
> + if (opts.addressprint)
> + fprintf_filtered (file, "%s ", paddress (target_gdbarch (), val));
> + val_print_string (builtin_type (target_gdbarch ())->builtin_char,
> + NULL, val, -1, file, &opts);
> + fprintf_filtered (file, "\n");
> + }
Likewise the other format strings.
> +
> +static void
> +default_print_auxv (struct ui_file *file, CORE_ADDR type, CORE_ADDR val)
> +{
Intro comment. Something like "default implementation of ...".
> +
> /* Print the contents of the target's AUXV on the specified file. */
> int
> fprint_target_auxv (struct ui_file *file, struct target_ops *ops)
> {
> + struct gdbarch *gdbarch = target_gdbarch();
Space before open parenthesis.
> diff --git a/gdb/auxv.h b/gdb/auxv.h
> index 9efe604..91d94f9 100644
> --- a/gdb/auxv.h
> +++ b/gdb/auxv.h
> @@ -46,6 +46,14 @@ extern int target_auxv_parse (struct target_ops *ops,
> extern int target_auxv_search (struct target_ops *ops,
> CORE_ADDR match, CORE_ADDR *valp);
>
> +/* Print a description of a single AUXV entry on the specified file. */
> +enum auxv_format { dec, hex, str };
These very generic names used to be OK, because they were
defined inside the function. But now that this is in a header,
the potential for conflict is much higher. hex could conflict
with std::hex, for instance. Please rename the values to
for example: AUXV_FORMAT_DEC / AUXV_FORMAT_HEX / AUXV_FORMAT_STR.
We should add a comment for what each value means, too, since
we lost the pass of obvious-because-of-locality.
> +
> +extern void fprint_single_auxv (struct ui_file *file, const char *name,
> + const char *description,
> + enum auxv_format flavor, CORE_ADDR type,
I'd find it more consistent to call parameter "format" instead of "flavor",
which then avoids people looking for enum auxv_flavor.
> +# Print the description of a single auxv entry described by TYPE and VAL
> +# to FILE. Return 0 and output no description if the TYPE is unknown.
> +# Return 0 if the TYPE of the auxv entry is unknown.
> +# Return 1 if a description was output.
> +M:int:print_auxv:struct ui_file *file, CORE_ADDR type, CORE_ADDR val:file, type, val
Thanks,
Pedro Alves
next prev parent reply other threads:[~2016-06-20 23:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-16 6:02 [PATCH 0/6] Add ELF auxiliary vector handling for FreeBSD John Baldwin
2016-06-16 6:02 ` [PATCH 3/6] Fetch the ELF auxiliary vector from live processes on FreeBSD John Baldwin
2016-06-20 23:01 ` Pedro Alves
2016-06-16 6:02 ` [PATCH 6/6] Add a gdbarch 'print_auxv' method for FreeBSD ABIs John Baldwin
2016-06-20 23:43 ` Pedro Alves
2016-06-16 6:02 ` [PATCH 2/6] Add elfcore_grok_freebsd_note to parse FreeBSD ELF core notes John Baldwin
2016-06-17 8:28 ` Nick Clifton
2016-06-16 6:02 ` [PATCH 1/6] Add constants for FreeBSD-specific auxiliary vector entry types John Baldwin
2016-06-17 8:27 ` Nick Clifton
2016-06-16 6:11 ` [PATCH 4/6] Create a psuedo section for the ELF AUXV core dump note on FreeBSD John Baldwin
2016-06-17 8:28 ` Nick Clifton
2016-06-20 23:45 ` Pedro Alves
2016-06-16 6:11 ` [PATCH 5/6] Add a new gdbarch method to print a single AUXV entry John Baldwin
2016-06-20 23:40 ` Pedro Alves [this message]
2016-06-23 20:19 ` John Baldwin
2016-06-20 17:10 ` [PATCH 0/6] Add ELF auxiliary vector handling for FreeBSD John Baldwin
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=17933b55-b3e1-51e3-3494-fa35b8cd71fd@redhat.com \
--to=palves@redhat.com \
--cc=binutils@sourceware.org \
--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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).