From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73454 invoked by alias); 20 Jun 2016 23:40:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 73424 invoked by uid 89); 20 Jun 2016 23:40:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=auxv, Different X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 20 Jun 2016 23:40:20 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BA34DC049D59; Mon, 20 Jun 2016 23:40:18 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5KNeHt7005309; Mon, 20 Jun 2016 19:40:17 -0400 Subject: Re: [PATCH 5/6] Add a new gdbarch method to print a single AUXV entry. To: John Baldwin , gdb-patches@sourceware.org, binutils@sourceware.org References: <20160616060202.63470-1-jhb@FreeBSD.org> <20160616060202.63470-6-jhb@FreeBSD.org> From: Pedro Alves Message-ID: <17933b55-b3e1-51e3-3494-fa35b8cd71fd@redhat.com> Date: Mon, 20 Jun 2016 23:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160616060202.63470-6-jhb@FreeBSD.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-06/txt/msg00342.txt.bz2 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