From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id 3C3443858416 for ; Fri, 22 Oct 2021 17:47:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3C3443858416 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wr1-x430.google.com with SMTP id e12so819197wra.4 for ; Fri, 22 Oct 2021 10:47:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Xu+2ulKV3yCx6UbdvidYF+JXkIwJi0ttsD4kNiMXS5E=; b=BSSE1PEaBMuNXSZMHBkbSdud6k6S/prWIvtEIU+pRJS/XWdQm3ItMjab+cX9SEhXoe MZ5IQ4BOosV2EpAFgvvbD8oEzVBRYeG+Xbfan7dV8QNcAzjqmLJ22A6HASzxnocdDKLS V5RuaGjYSs6Yqjzc4tQgr0TCVPm+eKExyyBWstCF+NfZUuF+7v0pAAKh8WgdG0yyWz7L z3yiBZAHqZLU09pkkS0otdeEf9H+gRUfw8EZrYQ3HTcJIuWlQ/BAoq78EOr0K+8kQokT Lxxw+AFJW30av4Ask3q4urT428DC3KXY/AtONVwqN1MxA0aKTALZ1IHZ7gZT+3zd+Z2M i4Kg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Xu+2ulKV3yCx6UbdvidYF+JXkIwJi0ttsD4kNiMXS5E=; b=Oylk7Fe79M18t8GDOqxhMegS41o7z45UL7NE/W3O6Avq3qjTGH1w5q+na0+n9dYPu2 AnZ5TRxVQOuzPKKPpHkHz4j6asDfkZdkLlKp3B77ql9g6myGwWR6x/ZOvr0ZYRZe8u24 S5hpye2wMXroLkx83qEOp8+3f0oMWqbgtUENQ2I5LjpnDJmGUbzcTT0fg1LaJd8Lm0JL AxBukgeM0SEDDoPVGoTf5wQAsolIfIfBgG5Ys3HSfVAi11Ree16U2juL4I8IiFzAw0or 1fwj6S666u1VFicYVjrTsMFMy1efF7tVViyjDnV5cb+tJBrfDmfi7kKJUK+MhQNrLa5l TsPA== X-Gm-Message-State: AOAM533pra/RL6HuAqwvzkuwmno1xeDRsz+IuE/glm8UitQYOvmR9bpC RXndBLGl8Btd+VobmHMeOQfetKh9/a0R3A== X-Google-Smtp-Source: ABdhPJz8IdHHv52mwPtSpZ2jAZ9garZfRUmjXHiDdzspkq6TMTZENWHwFwV0KtZKYTpe4rwirUFPUw== X-Received: by 2002:a5d:6d0a:: with SMTP id e10mr1539329wrq.157.1634924831252; Fri, 22 Oct 2021 10:47:11 -0700 (PDT) Received: from localhost (host212-140-123-151.range212-140.btcentralplus.com. [212.140.123.151]) by smtp.gmail.com with ESMTPSA id l2sm10620038wrs.90.2021.10.22.10.47.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Oct 2021 10:47:10 -0700 (PDT) Date: Fri, 22 Oct 2021 18:47:09 +0100 From: Andrew Burgess To: Eli Zaretskii Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/5] gdb/python: implement the print_insn extension language hook Message-ID: <20211022174709.GI19507@embecosm.com> References: <835yu0m64y.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <835yu0m64y.fsf@gnu.org> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 18:40:29 up 1 day, 9:14, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Fri, 22 Oct 2021 17:47:13 -0000 Thanks for the feedback. I have a couple of questions: * Eli Zaretskii [2021-10-14 10:12:45 +0300]: > > From: Andrew Burgess > > Date: Wed, 13 Oct 2021 22:59:10 +0100 > > > > diff --git a/gdb/NEWS b/gdb/NEWS > > index d001a03145d..fd1952a2f59 100644 > > --- a/gdb/NEWS > > +++ b/gdb/NEWS > > @@ -32,6 +32,12 @@ maint show internal-warning backtrace > > internal-error, or an internal-warning. This is on by default for > > internal-error and off by default for internal-warning. > > > > +set style disassembly on|off > > +show style disassembly > > + If GDB is compiled with Python support, and the Python pygments > > + module is available, then, when this setting is on, disassembler > > + output will have styling applied. > > If this requires Python with a module that is not available by > default, I think a general style name like "disassembly" would be > misleading. I suggest "pygment-disassembly" instead. I'm not sure I agree with this. I don't see why we'd want to leak an implementation detail (that we're using the pygment library) in a setting name. Surely, the setting should reflect what the effect is within GDB, and, as far as possible, the implementation should be hidden from the user. I'm hoping that some of the clarifications below might make this more palatable... > > > +@item set style disassembly @samp{on|off} > > +Enable or disable disassembly styling. This affects whether > > +disassembly output, such as the output of the @code{disassemble} > > +command, is styled. The default is @samp{on}. Note that disassembly > > +styling only works if styling in general is enabled, and if a source > > +highlighting library is available to @value{GDBN}. > > + > > +To highlight disassembly output @value{GDBN} must be compiled with > > +Python support, and the Python Pygments package must be available, > > So what does the default ON setting mean if pygments module is not > available, or if GDB was not compiled with Python support? You're correct, I've reworded this to reflect what actually happens. First, as this is all implemented in Python, if GDB is compiled without Python support then this setting (and the underlying feature) is just not available. If we do have Python, but not the Pygments library, then this feature will be off by default, and an attempt to turn it on will give an error that informs the user that the Python Pygments library is missing. Finally, if all the bits are in place, then this feature is on by default. > > +@node Disassembly In Python > > +@cindex Python Instruction Disassembly > > Index entries should begin with a lower-case letter, so that sorting > of the entries in the produced manual would not depend on the locale. > > > +@defivar DisassembleInfo can_emit_style_escapes > > +This is @code{True} if the output stream that the disassembler is > > +currently printing too can support escape sequences use for colors, > ^^^ > Should be "to". > > > +otherwise this attribute is @code{False}. > > Not sure why you are talking about escape sequences: we support > styling with colors also on terminals without escape sequences. Does > this mean this feature _must_ have actual escape sequence support? I took the name from the internal GDB functions that do the same check. Can you point me at the terminal that does syntax highlighting without using escape sequences, then I can see how this hooks back into GDB. Maybe I should rename this function 'supports_styling'? or something similar? Everything else I've fixed in my local tree. Thanks, Andrew > > > +@defmethod DisassembleInfo memory_error (offset) > > +This method marks the @code{DisassembleInfo} as having experienced a > > +@code{gdb.MemoryError} when trying to access memory of @var{offset} > > +bytes from @code{DisassembleInfo.address}. > > Should this text have a cross-reference to where MemoryError is > described? > > > +The optional @var{architecture} is either a string, or the value > > +@code{None}. If it is a string, then it should be the name of an > > +architecture known to @value{GDBN}, as returned either from > > +@code{gdb.Architecture.name()} > > +(@pxref{gdbpy_architecture_name,,gdb.Architecture.name}), or from > > +@code{gdb.architecture_names()} > > +(@pxref{gdb_architecture_names,,gdb.architecture_names}). > > Please remove the parentheses from the references to these methods. > > > +@defun format_address (architecture, address) > > +Returns @var{address} formatted as a string, in a style suitable for > > +including in the disassembly output of an instruction, for example a > > +formatted address might look like: > > + > > +@smallexample > > +0x00001042 > > +@end smallexample > > + > > +@var{architecture} is a @code{gdb.Architecture} (@pxref{Architectures > > +In Python}), which is required to format the addresses correctly. > > +This can be obtained from @code{DisassembleInfo.architecture}. > > This last paragraph should have @noindent before it, since it's a > continuation the description of format_address. > > > +After calling this function the result in @var{info} @emph{might} have > > +been updated to include syntax highlighting escape sequences. If > > +syntax highlighting is disabled in @value{GDBN}, or the output stream > > +doesn't support syntax highlighting, then this function will leave > > +@var{info} unchanged. > > I suggest a cross-reference to commands that enable syntax > highlighting where you mention it. > > > +This function should return a Python object that supports the buffer > > +protocol, i.e. a string, an array, or the object returned from > > Please add @: after i.e., to prevent TeX from typesetting that as an > end of a sentence. > > Thanks.