From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1726) id 5F7D83858285; Sun, 2 Oct 2022 16:36:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5F7D83858285 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1664728592; bh=r+sZWkgh2Fk8Bdfcfg8c1qoJSWAAIGExvAX6JrvtQIk=; h=From:To:Subject:Date:From; b=HtS5xkjA/MA2GXUh67XM5VIcrV50ByIxXI2TRrpiJ33m/OMByi2MTvkhe01QRt0kv XQ9QnIXpeAUXk0m/++nxdeSgFR1QyBWdXYssPYBam6v2P846Y69TjummUXmtsQFXiA BsM0lE6+y6Mxue8Xx/LJM3Uys4xWFoChelX1QjOo= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Andrew Burgess To: gdb-cvs@sourceware.org Subject: [binutils-gdb] gdb: improve disassembler styling when Pygments raises an exception X-Act-Checkin: binutils-gdb X-Git-Author: Andrew Burgess X-Git-Refname: refs/heads/master X-Git-Oldrev: 5b0e2b48edb9b6c53ed5c2de02b315838a1adae8 X-Git-Newrev: f22c50c22cbff208e3b024e344c0cb7d88dc0835 Message-Id: <20221002163632.5F7D83858285@sourceware.org> Date: Sun, 2 Oct 2022 16:36:32 +0000 (GMT) List-Id: https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3Df22c50c22cbf= f208e3b024e344c0cb7d88dc0835 commit f22c50c22cbff208e3b024e344c0cb7d88dc0835 Author: Andrew Burgess Date: Fri Aug 26 21:19:14 2022 +0100 gdb: improve disassembler styling when Pygments raises an exception =20 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. =20 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. =20 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. =20 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. =20 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. =20 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: =20 commit 4cbe4ca5da5cd7e1e6331ce11f024bf3c07b9744 Date: Mon Feb 14 14:40:52 2022 +0000 =20 gdb: add support for disassembler styling using libopcodes =20 This commit: =20 1. Factors out some of the condition checking logic into two new helper functions use_ext_lang_for_styling and use_libopcodes_for_styling, =20 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, =20 3. Make use of the new condition checking functions each time m_buffer is initialised, =20 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 =20 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). Diff: --- 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 b5e503fd71d..de44aac3263 100644 --- a/gdb/disasm.c +++ b/gdb/disasm.c @@ -972,26 +972,52 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *g= dbarch, 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. */ } =20 /* See disasm.h. */ =20 +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_fun= c, @@ -1079,10 +1105,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 =3D ext_lang_colorize_disasm (m_buffer.string (), arch = ()); @@ -1096,6 +1119,10 @@ gdb_disassembler::print_insn (CORE_ADDR memaddr, extension language for styling. */ use_ext_lang_colorization_p =3D false; =20 + /* 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 @@ -1108,7 +1135,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 =3D gdb_print_insn_1 (arch (), memaddr, &m_di); gdb_assert (length > 0); } diff --git a/gdb/disasm.h b/gdb/disasm.h index dab6116cd00..b7d16744c20 100644 --- a/gdb/disasm.h +++ b/gdb/disasm.h @@ -255,6 +255,9 @@ private: non-memory error. */ gdb::optional m_err_memaddr; =20 + /* 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 @@ private: styling or not. */ string_file m_buffer; =20 - /* 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. =20 @@ -284,6 +284,21 @@ private: 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; }; =20 /* An instruction to be disassembled. */ diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/styl= e.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 { } { } } =20 +# 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 =3D 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 =20 +# 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