From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1695 invoked by alias); 13 Jan 2019 13:05:35 -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 1682 invoked by uid 89); 13 Jan 2019 13:05:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=trial, notion, 0400 X-HELO: mailsec119.isp.belgacom.be Received: from mailsec119.isp.belgacom.be (HELO mailsec119.isp.belgacom.be) (195.238.20.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 13 Jan 2019 13:05:30 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1547384730; x=1578920730; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=XuTQAd+/bQj7hm8nvVMb9jg/rqXzTqkM4dZAJ4psaKw=; b=z4AGRHhrPXydydjxN9XZrcH2Vgv+Kmow7Nrfpj5UxWsSplCqz10f8Qrg 1DLrx+O1hVkREMHbhRMZ12ETekK0hA==; Received: from 184.205-67-87.adsl-dyn.isp.belgacom.be (HELO md) ([87.67.205.184]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 13 Jan 2019 14:05:27 +0100 Message-ID: <1547384727.5979.15.camel@skynet.be> Subject: Re: [RFA 1/3] Use function_name_style to print Ada and C function names From: Philippe Waroquiers To: Joel Brobecker , Tom Tromey Cc: gdb-patches@sourceware.org Date: Sun, 13 Jan 2019 13:05:00 -0000 In-Reply-To: <20190113051818.GE22922@adacore.com> References: <20190110220113.26169-1-philippe.waroquiers@skynet.be> <20190110220113.26169-2-philippe.waroquiers@skynet.be> <87bm4lx2v3.fsf@tromey.com> <1547310557.5979.5.camel@skynet.be> <87zhs5trcq.fsf@tromey.com> <20190113051818.GE22922@adacore.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00312.txt.bz2 On Sun, 2019-01-13 at 09:18 +0400, Joel Brobecker wrote: > > > > > > > "Philippe" == Philippe Waroquiers writes: > > > > Philippe> ada-typeprint.c:print_func_type is only called to print types that are > > Philippe> functions. So, that effectively prints a function name. > > > > No opinion on that one, maybe Joel could say. > > I tried to think about the general context, and the guideline in > my mind is that there is a fine line where coloring is helpful and > when it becomes too much, reducing the effectiveness of colorization. > In this case, this patch seems to advocate in favor of colorization? > > Perhaps we might indeed find it useful to colorize type names. The patch does not color type names. It colors function names that happens to be passed along with the type to the type printing code. See below longer explanation. > But, for the sake of consistency, let's color all type names, > rather than just function ones. And perhaps one thing we could do > to avoid an explosion of colorization options, which themselves > can lead to visual-confusion-by-color-mosaic effect, is to compromise > a bit by saying type names should be styled similarly to their object > counterparts. Eg: same colour, but with a "dim" filter? For instance, > if addresses are in blue, then types which are pointers/refs, etc, > could be printed in dim blue? > > One thing I might do, regardless of the decision, is define in the code > a different style for this kind of entity, even if, internally, the style > itself is just a reference (an "alias", if you will), of an existing > style. That way, it is easier to keep track of the various kinds of > entities and their style, thus allowing us the option of upgrading > this kind of entities to their own style if we wanted to. Otherwise, > we'd have to audit all the existing calls to styling to see which > ones we want to style separately. As I understand (and the trial I did confirms), the code is really coloring *function names*, and is not coloring *function type names*. (I have tried to clarify this in the commit msg of the RFAv2). Here is an example when debugging a piece of valgrind code with the patched GDB. Valgrind defines a function type HT_Cmp_t: typedef Word  (*HT_Cmp_t) ( const void* node1, const void* node2 ); (a comparison function type, to be used in hash tables). (gdb) info func HT_Cmp_t All functions matching regular expression "HT_Cmp_t": # this does not show anything, as expected, as # the type HT_Cmp_t is not a function. (gdb) info types HT_Cmp_t All types matching regular expression "HT_Cmp_t": File ../include/pub_tool_hashtable.h: 75: typedef Word (*)(const void *, const void *) HT_Cmp_t; # this shows the function type. The only part colored in # the above is the filename. In particular, the type name HT_Cmp_t # is not colored (as expected, because the patch aims at coloring # function names). (gdb) info func cmp_pool_str All functions matching regular expression "cmp_pool_str": File m_deduppoolalloc.c: 205: static Word cmp_pool_str(const void *, const void *); # this shows a specific function of the type HT_Cmp_t # and cmp_pool_str is colored (as expected) in function name style. The comments in ada-typeprint.c are pretty clear (but I however cannot replicate easily completely the above in Ada, as there is no real notion of function type in Ada, or at least I could not find it in the Ada Reference Manual : as I understand, the concept of 'Ada function type' is a GDB concept. /* Print function or procedure type TYPE on STREAM.  Make it a header    for function or procedure NAME if NAME is not null.  */ static void print_func_type (struct type *type, struct ui_file *stream, const char *name,  const struct type_print_options *flags) So, print_func_type effectively prints a function type, and optionally prints a function name given via the NAME arg. The patch only colors NAME using function name style. The c code is less clear documented than the ada print_func_type : it uses varstring as argname. The comments I found around the c and Ada type printing that references VARSTRING in upper case are: ada-typeprint.c:817:   If VARSTRING is a non-empty string, print as an Ada variable/field typeprint.c:396:   variable named VARSTRING.  (VARSTRING is demangled if necessary.) IMO, these comments are somewhat misleading, because at that stage, VARSTRING is not necessarily a variable or a field name, it can also be a function name (and maybe other things?). So, VARSTRING might better be a more general NAME (or ENTITY_NAME, ...). So, in summary: I think the patch ensures type printing code is only coloring function names (given as a 'side information' to the type to print) using function name style, it is not coloring the type name, that is probably somewhere deep in the type data structure. Whether or not we want to style type names is another question (and another patch then). IMO, the patch looks reasonable, and produces a much nicer/more readable output :). Thanks Philippe