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.133.124]) by sourceware.org (Postfix) with ESMTPS id 1B1F53839069 for ; Tue, 30 Aug 2022 14:17:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1B1F53839069 Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-518-RRpzbhHrPI2yKXLSV-JYIg-1; Tue, 30 Aug 2022 10:17:03 -0400 X-MC-Unique: RRpzbhHrPI2yKXLSV-JYIg-1 Received: by mail-wm1-f70.google.com with SMTP id x16-20020a1c7c10000000b003a5cefa5578so2056906wmc.7 for ; Tue, 30 Aug 2022 07:17:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc; bh=VmW8GGqqAsiTo/2PLHGXm0/x2GVy9dvJIeCtt7W+Dfo=; b=3SUfAqI/azJhlD8yEhvYHWsOcoPcjNf3hUhPB9+TeqbhuSV6VfRWpjD1hMQlV/ZgQf SJvmlL0BejdIivjb3Ioh1AI8vbWXPfVMUTvuw6ePy70CfIBUFERT9LC9oPF/m76fo4Lg Oym35FtbvhEnvob1Y14IytVBmX9GVlKTqv7u1HEF2ai7BLQJ1ibRq+OPh0/+T+5DtkG6 PaDSnWF4p/F/uYvcp7V1AWfYVqAFhTzaD/ZTGG5RjV+4tLbgQ2C9hsJrUbb0BGwatZhC Z94WVkSA2j3GBUHjv6DZtvDV+KYZg8e0vuKfEOi184yUIIY9G7sJVCfa9MILpSM/90io 5vKA== X-Gm-Message-State: ACgBeo0DkDBaIcMn+MepF4pmXiuz4LFO8MQIaDdCgQlVaHvfO199U1LK e5DbKEdsYQbJTMdVPZWoz1UpIM5ZHrMNWZ4uYY3NDXJVBPdci6An2SkQUC34zYQz5n3xezV1/FS BNEXKAmfMpSsy66UyjdsmGYki85lNBzGJHUB9fBC2JRu6JJ2lFn0Y5s4a3bhFf7cHIkbkJlvZIw == X-Received: by 2002:a5d:4302:0:b0:225:5303:39e5 with SMTP id h2-20020a5d4302000000b00225530339e5mr9400276wrq.380.1661869022210; Tue, 30 Aug 2022 07:17:02 -0700 (PDT) X-Google-Smtp-Source: AA6agR5IHAwFpO+FqV78b9/OH9ypQ5VP2VI/4vaYp3HIoxoWTaIZFeoF2h+g3cHoNYdFaApfwzFMcQ== X-Received: by 2002:a5d:4302:0:b0:225:5303:39e5 with SMTP id h2-20020a5d4302000000b00225530339e5mr9400248wrq.380.1661869021678; Tue, 30 Aug 2022 07:17:01 -0700 (PDT) Received: from localhost ([31.111.84.229]) by smtp.gmail.com with ESMTPSA id l9-20020adfe9c9000000b0022586045c89sm10201385wrn.69.2022.08.30.07.17.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Aug 2022 07:17:01 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception Date: Tue, 30 Aug 2022 15:16:52 +0100 Message-Id: X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.9 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, 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 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: Tue, 30 Aug 2022 14:17:10 -0000 While working on another issue relating to GDB's use of the Python Pygments package for disassembly styling I noticed an issue in the case where the Pygments package raises an exception. The intention of the current code is that, should the Pygments package raise an exception, GDB will disable future attempts to call into the Pygments code. This was intended to prevent repeated errors during disassembly if, for some reason, the Pygments code isn't working. Since the Pygments based styling was added, GDB now supports disassembly styling using libopcodes, but this is only available for some architectures. For architectures not covered by libopcodes Pygments is still the only option. What I observed is that, if I disable the libopcodes styling, then setup GDB so that the Pygments based styling code will indicate an error (by returning None), GDB does, as expected, stop using the Pygments based styling. However, the libopcodes based styling will instead be used, despite this feature having been disabled. The problem is that the disassembler output is produced into a string_file buffer. When we are using Pygments, this buffer is created without styling support. However, when Pygments fails, we recreate the buffer with styling support. The problem is that we should only recreate the buffer with styling support only if libopcodes styling is enabled. This was an oversight in commit: commit 4cbe4ca5da5cd7e1e6331ce11f024bf3c07b9744 Date: Mon Feb 14 14:40:52 2022 +0000 gdb: add support for disassembler styling using libopcodes This commit: 1. Factors out some of the condition checking logic into two new helper functions use_ext_lang_for_styling and use_libopcodes_for_styling, 2. Reorders gdb_disassembler::m_buffer and gdb_disassembler::m_dest, this allows these fields to be initialised m_dest first, which means that the new condition checking functions can rely on m_dest being set, even when called from the gdb_disassembler constructor, 3. Make use of the new condition checking functions each time m_buffer is initialised, 4. Add a new test that forces the Python disassembler styling function to return None, this will cause GDB to disable use of Pygments for styling, and 5. When we reinitialise m_buffer, and re-disassemble the instruction, call reset the in-comment flag. If the instruction being disassembler ends in a comment then the first disassembly pass will have set the in-comment flag to true. This shouldn't be a problem as we will only be using Pygments, and thus performing a re-disassembly pass, if libopcodes is disabled, so the in-comment flag will never be checked, even if it is set incorrectly. However, I think that having the flag set correctly is a good thing, even if we don't check it (you never know what future uses might come up). --- gdb/disasm.c | 69 ++++++++++++++++++++++---------- gdb/disasm.h | 21 ++++++++-- gdb/testsuite/gdb.base/style.exp | 64 +++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 24 deletions(-) diff --git a/gdb/disasm.c b/gdb/disasm.c index db6724757ac..534d00b5fa2 100644 --- a/gdb/disasm.c +++ b/gdb/disasm.c @@ -938,26 +938,52 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch, read_memory_ftype func) : gdb_printing_disassembler (gdbarch, &m_buffer, func, dis_asm_memory_error, dis_asm_print_address), - /* The use of m_di.created_styled_output here is a bit of a cheat, but - it works fine for now. Currently, for all targets that support - libopcodes styling, this field is set during the call to - disassemble_init_for_target, which was called as part of the - initialization of gdb_printing_disassembler. And so, we are able to - spot if a target supports libopcodes styling, and create m_buffer in - the correct styling mode. - - If there's ever a target that only sets created_styled_output during - the actual disassemble phase, then the logic here will have to - change. */ - m_buffer ((!use_ext_lang_colorization_p - || (use_libopcodes_styling && m_di.created_styled_output)) - && disassembler_styling - && file->can_emit_style_escape ()), - m_dest (file) + m_dest (file), + m_buffer (!use_ext_lang_for_styling () && use_libopcodes_for_styling ()) { /* Nothing. */ } /* See disasm.h. */ +bool +gdb_disassembler::use_ext_lang_for_styling () const +{ + /* The use of m_di.created_styled_output here is a bit of a cheat, but + it works fine for now. + + This function is called in situations after m_di has been initialized, + but before the instruction has been disassembled. + + Currently, every target that supports libopcodes styling sets the + created_styled_output field in disassemble_init_for_target, which was + called as part of the initialization of gdb_printing_disassembler. + + This means that we are OK to check the created_styled_output field + here. + + If, in the future, there's ever a target that only sets the + created_styled_output field during the actual instruction disassembly + phase, then we will need to update this code. */ + return (disassembler_styling + && (!m_di.created_styled_output || !use_libopcodes_styling) + && use_ext_lang_colorization_p + && m_dest->can_emit_style_escape ()); +} + +/* See disasm.h. */ + +bool +gdb_disassembler::use_libopcodes_for_styling () const +{ + /* See the comment on the use of m_di.created_styled_output in the + gdb_disassembler::use_ext_lang_for_styling function. */ + return (disassembler_styling + && m_di.created_styled_output + && use_libopcodes_styling + && m_dest->can_emit_style_escape ()); +} + +/* See disasm.h. */ + gdb_disassemble_info::gdb_disassemble_info (struct gdbarch *gdbarch, read_memory_ftype read_memory_func, memory_error_ftype memory_error_func, @@ -1045,10 +1071,7 @@ gdb_disassembler::print_insn (CORE_ADDR memaddr, already styled the output for us, and, if the destination can support styling, then lets call into the extension languages in order to style this output. */ - if (length > 0 && disassembler_styling - && (!m_di.created_styled_output || !use_libopcodes_styling) - && use_ext_lang_colorization_p - && m_dest->can_emit_style_escape ()) + if (length > 0 && use_ext_lang_for_styling ()) { gdb::optional ext_contents; ext_contents = ext_lang_colorize_disasm (m_buffer.string (), arch ()); @@ -1062,6 +1085,10 @@ gdb_disassembler::print_insn (CORE_ADDR memaddr, extension language for styling. */ use_ext_lang_colorization_p = false; + /* We're about to disassemble this instruction again, reset the + in-comment state. */ + this->set_in_comment (false); + /* The instruction we just disassembled, and the extension languages failed to style, might have otherwise had some minimal styling applied by GDB. To regain that styling we @@ -1074,7 +1101,7 @@ gdb_disassembler::print_insn (CORE_ADDR memaddr, string_file>::value)); gdb_assert (!m_buffer.term_out ()); m_buffer.~string_file (); - new (&m_buffer) string_file (true); + new (&m_buffer) string_file (use_libopcodes_for_styling ()); length = gdb_print_insn_1 (arch (), memaddr, &m_di); gdb_assert (length > 0); } diff --git a/gdb/disasm.h b/gdb/disasm.h index 09cb3921767..7490ded000a 100644 --- a/gdb/disasm.h +++ b/gdb/disasm.h @@ -255,6 +255,9 @@ struct gdb_disassembler : public gdb_printing_disassembler, non-memory error. */ gdb::optional m_err_memaddr; + /* The stream to which disassembler output will be written. */ + ui_file *m_dest; + /* Disassembler output is built up into this buffer. Whether this string_file is created with styling support or not depends on the value of use_ext_lang_colorization_p, as well as whether disassembler @@ -262,9 +265,6 @@ struct gdb_disassembler : public gdb_printing_disassembler, styling or not. */ string_file m_buffer; - /* The stream to which disassembler output will be written. */ - ui_file *m_dest; - /* When true, m_buffer will be created without styling support, otherwise, m_buffer will be created with styling support. @@ -284,6 +284,21 @@ struct gdb_disassembler : public gdb_printing_disassembler, struct disassemble_info *info); static void dis_asm_print_address (bfd_vma addr, struct disassemble_info *info); + + /* Return true if we should use the extension language to apply + disassembler styling. This requires disassembler styling to be on + (i.e. 'set style disassembler enabled on'), the output stream needs to + support styling, and libopcode styling needs to be either off, or not + supported for the current architecture (libopcodes is used in + preference to the extension language method). */ + bool use_ext_lang_for_styling () const; + + /* Return true if we should use libopcodes to apply disassembler styling. + This requires disassembler styling to be on (i.e. 'set style + disassembler enabled on'), the output stream needs to support styling, + and libopcodes styling needs to be supported for the current + architecture, and not disabled by the user. */ + bool use_libopcodes_for_styling () const; }; /* An instruction to be disassembled. */ diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp index 3126748bb1c..c6ed996c280 100644 --- a/gdb/testsuite/gdb.base/style.exp +++ b/gdb/testsuite/gdb.base/style.exp @@ -394,6 +394,66 @@ proc test_disable_disassembler_styling { } { } } +# Disassemble a single isntruction at the start of main, then strip +# off the address and symbol information, returning just the +# disassembled instruction part. +proc get_single_disassembled_insn {} { + set disasm_line [capture_command_output "x/1i *main" ""] + regexp "^\[^:\]+:\\s*(.*)$" $disasm_line whole_match insn + return $insn +} + +# Check that, if the user is using Python Pygments for disassembler +# styling, then the styling correctly switches off when an error is +# detected in the Python code. +proc test_disassembler_error_handling { } { + + # This test requires the Python Pygments module to be installed + # and used by GDB. + if { !$::python_disassembly_styling } { + return + } + + save_vars { env(TERM) } { + # We need an ANSI-capable terminal to get the output. + setenv TERM ansi + + # Restart GDB with the correct TERM variable setting, this + # means that GDB will enable styling. + clean_restart_and_disable "restart 4" $::binfile + + # Disable use of libopcodes for styling. As this function is + # only called when Python Pygments module is available, we + # should now be using that module to style the disassembler + # output. + gdb_test_no_output "maint set libopcodes-styling enabled off" + + # Disassemble a single instruction and ensure that the output + # has styling markers in it. + set insn_before [get_single_disassembled_insn] + gdb_assert { [regexp "\033" $insn_before] } \ + "have style markers when Pygments is working fine" + + # Now replace the standard function that colorizes the + # disassembler output, with a new function that always returns + # None, this should cause GDB to stop using the Pygments + # module for disassembler styling. + gdb_py_test_silent_cmd \ + [multi_line_input \ + "python" \ + "def replacement_colorize_disasm(content,gdbarch):" \ + " return None" \ + "gdb.styling.colorize_disasm = replacement_colorize_disasm" \ + "\004"] \ + "setup replacement colorize_disasm function" \ + true + + set insn_after [get_single_disassembled_insn] + gdb_assert { ![regexp "\033" $insn_after] } \ + "have no style markers when Pygments is broken" + } +} + # A separate test from the above as the styled text this checks can't # currently be disabled (the text is printed too early in GDB's # startup process). @@ -435,5 +495,9 @@ foreach style { title file function highlight variable \ # Check that the disassembler styling can be disabled. test_disable_disassembler_styling +# Check that GDB handles an error in the Python Pygments disassembly +# styling code. +test_disassembler_error_handling + # Finally, check the styling of the version string during startup. test_startup_version_string -- 2.25.4