From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id IZk6OZARLGSZ8yIAWB0awg (envelope-from ) for ; Tue, 04 Apr 2023 08:01:20 -0400 Received: by simark.ca (Postfix, from userid 112) id DAC581E15D; Tue, 4 Apr 2023 08:01:20 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=QWnmm+GN; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-7.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 41FB51E0D3 for ; Tue, 4 Apr 2023 08:01:17 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C4A8D385084A for ; Tue, 4 Apr 2023 12:01:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C4A8D385084A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1680609675; bh=gsVo9lIYLtTK2ZUtgNbilWwaoiuYI1wmZwi/gGC2TTo=; h=Date:To:Cc:In-Reply-To:Subject:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=QWnmm+GNDwmciNLC1P0YkQtWrB4NPDs9bbGbzJsUvRbyTPONAeiKwmaieVUrZoOgm 0uv4FsLXssXhdt1FYSL71lsXczB9VhlMqdQhdE/zBchghILkn1QYAwxKRPA/hGM9iB e2Z3LGh8Lae0374k5xIVtv2R4ro7WdrfetAtImCU= Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id F20873858C54 for ; Tue, 4 Apr 2023 12:00:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F20873858C54 Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pjfLK-0000eF-TC; Tue, 04 Apr 2023 08:00:55 -0400 Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pjfLJ-0002Q8-H4; Tue, 04 Apr 2023 08:00:54 -0400 Date: Tue, 04 Apr 2023 15:01:17 +0300 Message-Id: <83v8ibua9e.fsf@gnu.org> To: Andrew Burgess Cc: gdb-patches@sourceware.org In-Reply-To: <62700c9aaddb83cc05e1b5e51e115d4eea8281e4.1680596378.git.aburgess@redhat.com> (message from Andrew Burgess via Gdb-patches on Tue, 4 Apr 2023 09:21:07 +0100) Subject: Re: [PATCH 5/5] gdb/python: extend the Python Disassembler API to allow for styling References: <62700c9aaddb83cc05e1b5e51e115d4eea8281e4.1680596378.git.aburgess@redhat.com> X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Eli Zaretskii via Gdb-patches Reply-To: Eli Zaretskii Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" > Cc: Andrew Burgess > Date: Tue, 4 Apr 2023 09:21:07 +0100 > From: Andrew Burgess via Gdb-patches > > gdb/NEWS | 19 + > gdb/doc/python.texi | 313 +++++++++- > gdb/python/py-disasm.c | 818 +++++++++++++++++++++++-- > gdb/testsuite/gdb.python/py-disasm.exp | 94 ++- > gdb/testsuite/gdb.python/py-disasm.py | 158 ++++- > 5 files changed, 1309 insertions(+), 93 deletions(-) Thanks. > diff --git a/gdb/NEWS b/gdb/NEWS > index d1726842299..dceccf09c6d 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -155,6 +155,25 @@ info main > ** It is now no longer possible to sub-class the > gdb.disassembler.DisassemblerResult type. > > + ** The Disassembler API from the gdb.disassembler module has been > + extended to include styling support: > + > + - The DisassemblerResult class can now be initialized with a list > + of parts. Each part represents part of the disassembled > + instruction along with the associated style information. This > + list of parts can be accessed with the new > + DisassemblerResult.parts property. > + > + - New constants gdb.disassembler.STYLE_* representing all the > + different styles part of an instruction might have. > + > + - New methods DisassembleInfo.text_part and > + DisassembleInfo.address_part which are used to create the new > + styled parts of a disassembled instruction. > + > + - Changes are backwards compatible, the older API can still be > + used to disassemble instructions without styling. > + > *** Changes in GDB 13 This part is OK. > +@defun DisassembleInfo.text_part (@var{style}, @var{string}) Same question about @var inside @defun. > +@defun DisassembleInfo.address_part (@var{address}) > +Create a new @code{DisassemblerAddressPart}. @var{address} is the > +value of the absolute address this part represents. When @value{GDBN} > +displays an address part it display the absolute address, and also an > +associated symbol. The last sentence is problematic ("When GDB dipslays [...] it display...") Suggest to rephrase. > +Only one of @var{string} or @var{parts} should be used to initialize a > +new @code{DisassemblerResult}, whichever is not used should be passed > +the value @code{None}, or the arguments can be passed by name, and the > +unused argument can be ignored. This sentence is hard to parse. Suggest to reword: Only one of @var{string} or @var{parts} should be used to initialize a new @code{DisassemblerResult}; the other one should be passed the value @code{None}. Alternatively, the arguments can be passed by name, and the unused argument can be ignored. > +The @var{string} argument, if not @code{None}, is a non-empty string > +that represents the entire disassembled instruction. Building a result ^^ Two spaces there. > +If this instance was initialized using a single string rather than a ^ That "a" should be deleted. > +The following table lists all of the disassembler styles that are > +available. @value{GDBN} maps these style constants onto its style > +settings (@pxref{Output Styling}). In some cases multiple of these > +style constants map to the same style setting. Suggest to reword the last sentence above: In some cases, several style constants produce the same style settings, and thus will produce the same visual effect on the screen. > This could change in > +future releases of @value{GDBN}, so care should be taken to select the > +correct style constant to ensure correct output styling on future > +releases of @value{GDBN}. ^^ "in", not "on". > +@vindex STYLE_IMMEDIATE > +@item gdb.disassembler.STYLE_IMMEDIATE > +This style is used for styling numerical values within a disassembled > +instruction when those values are not addresses, address offsets, or > +register numbers, use @code{STYLE_ADDRESS}, > +@code{STYLE_ADDRESS_OFFSET}, or @code{STYLE_REGISTER} in those cases. This should be rewritten as 2 separate sentences: one describing which values use STYLE_IMMEDIATE, the other (perhaps in parentheses) describing the alternative style constants for other kinds of values. > +For any other numerical value within a disassembled instruction, > +@code{STYLE_IMMEDIATE} should be used. This reads awkward, both because it uses passive voice, and because the verb is at the end. Reword: Use the @code{STYLE_IMMEDIATE} for any other values within a disassembled instruction. Also, it sounds like this just repeats the first of the two sentences above, so perhaps it should be removed? It might be a good idea to describe the "other" styles for values first, and then the "immediate" style, as the catchall for the other values. > +referring too. For example, this x86-64 instruction: > + > +@smallexample > +call 0x401136 > +@end smallexample > + > +@noindent > +Here @code{foo} is the name of a symbol, and should be given the > +@code{STYLE_SYMBOL} style. Since you start a new sentence after @noindent, the "For example" part should be reworded, perhaps leaving just "For example:". Reviewed-By: Eli Zaretskii