From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 254973858D37 for ; Fri, 28 Apr 2023 23:11:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 254973858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1682723498; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=zSANkiSOSstNnn5r3QZdc4G6nGS/WFJtTe8SmAV0wm0=; b=g5XFYKa1VO/ukBPOqQBrKgc2C6ediKTOJgbVIWYMVUiJ9j/YQpX/nFuRXbvegQpHN3Tb6A b3YFk2iU3eiyuW4HGui9WYSWHdsTPpeqmxwBDmKpLhzMcsAD/B1pxrF6uFzYAe5m/DI40L EZOiujHcYcnTnYcrU3D102Xx5wKv7Vc= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-325-TBnpOU5yMUaX_9W8QgkP6w-1; Fri, 28 Apr 2023 19:11:37 -0400 X-MC-Unique: TBnpOU5yMUaX_9W8QgkP6w-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-3f18c2b2110so681865e9.3 for ; Fri, 28 Apr 2023 16:11:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682723496; x=1685315496; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=zSANkiSOSstNnn5r3QZdc4G6nGS/WFJtTe8SmAV0wm0=; b=RTCf5UNJi8J5LAmlPcOAtqjIb+53CJ/Ta/DhISJ6ZHaHGFJrc0C3Pba4fbQadSklM2 Wt+HB3Bnym29ZZMd0jN8ufZCxNU6UvtvZ/GmKCZx3ukIRCw+aT91HVJUn/kWntzq0BqA WpLEe7cqtv//sJF9kXA2nar9yx+Vy22TGIyLpZOsu7X9Q5fkbT6St5bTHa64GYlr124s j/dCO6SmUAcZdnvOKPg+ziOf0p/mdEM9RC2m0GCJuowK8G/tv6U/gi681K2700FJYF1C aU6FmidCQ3NXT8iK/luTthCFARcopuMQREoBGs/EHxexj4K+KCa7/KS8OztQ4DwDjShJ Hgzg== X-Gm-Message-State: AC+VfDzFA6KwGe8kzr66Xg9ACCs2BvfBizur999rlg0cPU6rv/1xtO1b wv4a7GSrnOdxWT+swjMzWEQYfzCL77zvYbF3VT3pKr3i6UGraHNyBygU9N9ZItGKiDSbKSrXIoy dA4/w+5crjwDFa/KVXd5wNkkM36Gbeg== X-Received: by 2002:adf:f80d:0:b0:2f4:cfb4:57f7 with SMTP id s13-20020adff80d000000b002f4cfb457f7mr5409538wrp.61.1682723496254; Fri, 28 Apr 2023 16:11:36 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4CBzcFivhfKdSQB0KxUsZoW7MrzNSf6gg30gVkJlqACFUm1inaoQBfTOzY9F0GvKq6ehxsnQ== X-Received: by 2002:adf:f80d:0:b0:2f4:cfb4:57f7 with SMTP id s13-20020adff80d000000b002f4cfb457f7mr5409531wrp.61.1682723495989; Fri, 28 Apr 2023 16:11:35 -0700 (PDT) Received: from localhost (11.72.115.87.dyn.plus.net. [87.115.72.11]) by smtp.gmail.com with ESMTPSA id z9-20020a05600c114900b003f1745c7df3sm2277719wmz.23.2023.04.28.16.11.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Apr 2023 16:11:35 -0700 (PDT) From: Andrew Burgess To: Eli Zaretskii Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/5] gdb/python: extend the Python Disassembler API to allow for styling In-Reply-To: <83v8ibua9e.fsf@gnu.org> References: <62700c9aaddb83cc05e1b5e51e115d4eea8281e4.1680596378.git.aburgess@redhat.com> <83v8ibua9e.fsf@gnu.org> Date: Sat, 29 Apr 2023 00:11:34 +0100 Message-ID: <87a5yra9g9.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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: Eli Zaretskii writes: >> 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 Eli, Thanks for the feedback. I've included all these fixes in the updated patch that I replied to Tom with. And now with no excessive @var uses :) Thanks, Andrew