From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org 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 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=gsVo9lIYLtTK2ZUtgNbilWwaoiuYI1wmZwi/gGC2TTo=; b=CK3HnM47QBFr d7t0shcznlFRv6qmNKYEWdAFcLLBrlEjwXD+2ZCXzufruGzcx4gc/HssbyO5T0AmIsZtpmo3vSaQq VQIhBtwIMceQYiifwjJid6AdN8rSxJf+VfHH5MlP8AL4G208tnlTHdp/JhT+YiegTq8C0eaOO3YwW dl+y6IJlSRzmd9v9y7mUr7kqqXbaUs3J0K4/gdRJrPJjLKTiX3U7uWssPick3/hQAUEgi4FgFMZtw 3V0MLlYYXS8TQXFHQD0IEMCpvDmu31xQvtgotBp6DVDINOqjG8nyC9VXJobpICJI81KjCXt9NQ3Ef KSfbVaisO93NII4eociOLg==; 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> From: Eli Zaretskii 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-Spam-Status: No, score=-7.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > 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