[PATCH 0/3] Improvements for Pygments based disassembly styling

[PATCH 0/3] Improvements for Pygments based disassembly styling

[Andrew Burgess]

This series was inspired by this stackoverflow post:

  https://stackoverflow.com/questions/73491793/why-is-there-a-%c2%b1-in-lea-rax-rip-%c2%b1-0xeb3

which highlighted some issues with the Python Pygments based
disassembler styling that was being used for all architectures in GDB
12.

The next release of GDB will use libopcodes styling for many of the
most popular architectures, but lots of the smaller architectures are
still using Pygments, and Pygments can still be used for all
architectures if a user chooses to disable use of libopcodes styling,
though I don't know why they would want to do that.

---

Andrew Burgess (3):
  gdb/testsuite: extend styling test for libopcodes styling
  gdb: improve disassembler styling when Pygments raises an exception
  gdb/disasm: better intel flavour disassembly styling with Pygments

 gdb/disasm.c                     |  69 ++++++++++----
 gdb/disasm.h                     |  21 +++-
 gdb/python/lib/gdb/styling.py    |  59 +++++++++++-
 gdb/testsuite/gdb.base/style.exp | 159 +++++++++++++++++++++++++------
 4 files changed, 250 insertions(+), 58 deletions(-)

-- 
2.25.4

[PATCH 1/3] gdb/testsuite: extend styling test for libopcodes styling

[Andrew Burgess]

This commit extends the gdb.base/style.exp test to cover disassembler
styling using libopcodes (where available).

The test will try to enable libopcode based styling, if this
works (because such styling is available) then some tests are run to
check that the output is styled, and that the styling can be disabled
using 'set style disassembler enabled off'.  If libopcodes styling is
not available on the current architecture then these new tests will be
skipped.

I've moved the Python Pygments module check inside the
test_disable_disassembler_styling function now, so that the test will
be run even when Python Pygments is not available, this allows the new
tests discussed above.

If the Pygments module is not available then the Pygments based tests
will be skipped just as they were before.
---
 gdb/testsuite/gdb.base/style.exp | 95 ++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 30 deletions(-)

diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index 2242c5bf743..3126748bb1c 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -317,39 +317,76 @@ proc test_disable_disassembler_styling { } {
 	set styled_hex [limited_style $::hex address]
 	set main [limited_style main function]
 
-	foreach_with_prefix disasm_styling { on off } {
-	    gdb_test_no_output "set style disassembler enabled ${disasm_styling}"
-
-	    set saw_header_line false
-	    set saw_styled_output_line false
-	    set saw_unstyled_output_line false
-	    gdb_test_multiple "disassemble main" "" {
-		-re "disassemble main\r\n" {
-		    exp_continue
-		}
-		-re "^Dump of assembler code for function $main:" {
-		    set saw_header_line true
+	foreach_with_prefix libopcodes { on off } {
+
+	    set command_failed false
+	    gdb_test_multiple "maint set libopcodes-styling enabled ${libopcodes}" "" {
+		-re "^maint set libopcodes-styling enabled ${libopcodes}\r\n" {
 		    exp_continue
 		}
-		-re "^\\s+${styled_hex}\\s+<\[^>\]+>:\\s+\[^\r\n\033\]+\r\n" {
-		    set saw_unstyled_output_line true
+
+		-re "Use of libopcodes styling not supported on architecture \[^\r\n\]+\r\n" {
+		    set command_failed true
 		    exp_continue
 		}
-		-re "^\\s+${styled_hex}\\s+<\[^>\]+>:\\s+\[^\r\n\]+\033\[^\r\n\]+\r\n" {
-		    set saw_styled_output_line true
-		    exp_continue
+
+		-re "^$::gdb_prompt $" {
+		    gdb_assert { !$command_failed || [string equal $libopcodes "on"] } \
+			$gdb_test_name
 		}
-		-re "^End of assembler dump\\.\r\n" {
-		    exp_continue
+	    }
+
+	    if { $libopcodes == "on" && $command_failed } {
+		# We tried to turn on libopcodes disassembler styling,
+		# but this is not supported on this architecture.
+		continue
+	    }
+
+	    foreach_with_prefix disasm_styling { on off } {
+		gdb_test_no_output "set style disassembler enabled ${disasm_styling}"
+
+		if { $libopcodes == "off" && $disasm_styling == "on" \
+			 && !$::python_disassembly_styling} {
+		    # We have libopcodes based styling off, but
+		    # disassembler styling turned on.  We're expecting
+		    # Python Pygments to be used to add styling.
+		    #
+		    # However, if we get here, then we don't have the
+		    # Pygments module, so skip this test.
+		    continue
 		}
-		-re "^$::gdb_prompt $" {
-		    gdb_assert { $saw_header_line }
-		    if { $disasm_styling } {
-			gdb_assert { $saw_styled_output_line }
-			gdb_assert { !$saw_unstyled_output_line }
-		    } else {
-			gdb_assert { !$saw_styled_output_line }
-			gdb_assert { $saw_unstyled_output_line }
+
+		set saw_header_line false
+		set saw_styled_output_line false
+		set saw_unstyled_output_line false
+		gdb_test_multiple "disassemble main" "" {
+		    -re "disassemble main\r\n" {
+			exp_continue
+		    }
+		    -re "^Dump of assembler code for function $main:" {
+			set saw_header_line true
+			exp_continue
+		    }
+		    -re "^\\s+${styled_hex}\\s+<\[^>\]+>:\\s+\[^\r\n\033\]+\r\n" {
+			set saw_unstyled_output_line true
+			exp_continue
+		    }
+		    -re "^\\s+${styled_hex}\\s+<\[^>\]+>:\\s+\[^\r\n\]+\033\[^\r\n\]+\r\n" {
+			set saw_styled_output_line true
+			exp_continue
+		    }
+		    -re "^End of assembler dump\\.\r\n" {
+			exp_continue
+		    }
+		    -re "^$::gdb_prompt $" {
+			gdb_assert { $saw_header_line }
+			if { $disasm_styling } {
+			    gdb_assert { $saw_styled_output_line }
+			    gdb_assert { !$saw_unstyled_output_line }
+			} else {
+			    gdb_assert { !$saw_styled_output_line }
+			    gdb_assert { $saw_unstyled_output_line }
+			}
 		    }
 		}
 	    }
@@ -396,9 +433,7 @@ foreach style { title file function highlight variable \
 }
 
 # Check that the disassembler styling can be disabled.
-if { $python_disassembly_styling } {
-    test_disable_disassembler_styling
-}
+test_disable_disassembler_styling
 
 # Finally, check the styling of the version string during startup.
 test_startup_version_string
-- 
2.25.4

[PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Andrew Burgess]

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<std::string> 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<CORE_ADDR> 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

[PATCH 3/3] gdb/disasm: better intel flavour disassembly styling with Pygments

[Andrew Burgess]

This commit was inspired by this stackoverflow post:

  https://stackoverflow.com/questions/73491793/why-is-there-a-%C2%B1-in-lea-rax-rip-%C2%B1-0xeb3

One of the comments helpfully links to this Python test case:

  from pygments import formatters, lexers, highlight

  def colorize_disasm(content, gdbarch):
      try:
          lexer = lexers.get_lexer_by_name("asm")
          formatter = formatters.TerminalFormatter()
          return highlight(content, lexer, formatter).rstrip().encode()
      except:
          return None

  print(colorize_disasm("lea [rip+0x211]  # COMMENT", None).decode())

Run the test case and you should see that the '+' character is
underlined, and could be confused with a combined +/- symbol.

What's happening is that Pygments is failing to parse the input text,
and the '+' is actually being marked in the error style.  The error
style is red and underlined.

It is worth noting that the assembly instruction being disassembled
here is an x86-64 instruction in the 'intel' disassembly style, rather
than the default att style.  Clearly the Pygments module expects the
att syntax by default.

If we change the test case to this:

  from pygments import formatters, lexers, highlight

  def colorize_disasm(content, gdbarch):
      try:
          lexer = lexers.get_lexer_by_name("asm")
          lexer.add_filter('raiseonerror')
          formatter = formatters.TerminalFormatter()
          return highlight(content, lexer, formatter).rstrip().encode()
      except:
          return None

  res = colorize_disasm("lea rax,[rip+0xeb3] # COMMENT", None)
  if res:
      print(res.decode())
  else:
      print("No result!")

Here I've added the call: lexer.add_filter('raiseonerror'), and I am
now checking to see if the result is None or not.  Running this and
the test now print 'No result!' - instead of styling the '+' in the
error style, we instead give up on the styling attempt.

There are two things we need to fix relating to this disassembly
text.  First, Pygments is expecting att style disassembly, not the
intel style that this example uses.  Fortunately, Pygments also
supports the intel style, all we need to do is use the 'nasm' lexer
instead of the 'asm' lexer.

However, this leads to the second problem; in our disassembler line we
have '# COMMENT'.  The "official" Intel disassembler style uses ';'
for its comment character, however, gas and libopcodes use '#' as the
comment character, as gas uses ';' for an instruction separator.

Unfortunately, Pygments expects ';' as the comment character, and
treats '#' as an error, which means, with the addition of the
'raiseonerror' filter, that any line containing a '#' comment, will
not get styled correctly.

However, as the i386 disassembler never produces a '#' character other
than for comments, we can easily "fix" Pygments parsing of the
disassembly line.  This is done by creating a filter.  This filter
looks for an Error token with the value '#', we then change this into
a comment token.  Every token after this (until the end of the line)
is also converted into a comment.

In this commit I do the following:

  1. Check the 'disassembly-flavor' setting and select between the
  'asm' and 'nasm' lexers based on the setting.  If the setting is not
  available then the 'asm' lexer is used by default,

  2. Use "add_filter('raiseonerror')" to ensure that the formatted
  output will not include any error text, which would be underlined,
  and might be confusing,

  3. If the 'nasm' lexer is selected, then add an additional filter
  that will format '#' and all other text on the line, as a comment,
  and

  4. If Pygments throws an exception, instead of returning None,
  return the original, unmodified content.  This will mean that this
  one instruction is printed without styling, but GDB will continue to
  call into the Python code to style later instructions.

I haven't included a test specifically for the above error case,
though I have manually check that the above case now styles
correctly (with no underline).  The existing style tests check that
the disassembler styling still works though, so I know I've not
generally broken things.

One final thought I have after looking at this issue is that I wonder
now if using Pygments for styling disassembly from every architecture
is actually a good idea?

Clearly, the 'asm' lexer is OK with att style x86-64, but not OK with
intel style x86-64, so who knows how well it will handle other random
architectures?

When I first added this feature I tested it against some random
RISC-V, ARM, and X86-64 (att style) code, and it seemed fine, but I
never tried to make an exhaustive check of all instructions, so its
quite possible that there are corner cases where things are styled
incorrectly.

With the above changes I think that things should be a bit better
now.  If a particular instruction doesn't parse correctly then our
Pygments based styling code will just not style that one instruction.
This is combined with the fact that many architectures are now moving
to libopcodes based styling, which is much more reliable.

So, I think it is fine to keep using Pygments as a fallback mechanism
for styling all architectures, even if we know it might not be perfect
in all cases.
---
 gdb/python/lib/gdb/styling.py | 59 ++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 4 deletions(-)

diff --git a/gdb/python/lib/gdb/styling.py b/gdb/python/lib/gdb/styling.py
index aef39c6857c..b97f1dd7fb8 100644
--- a/gdb/python/lib/gdb/styling.py
+++ b/gdb/python/lib/gdb/styling.py
@@ -20,26 +20,77 @@ import gdb
 
 try:
     from pygments import formatters, lexers, highlight
+    from pygments.token import Error, Comment, Text
+    from pygments.filters import TokenMergeFilter
+
+    _formatter = None
+
+    def get_formatter():
+        global _formatter
+        if _formatter is None:
+            _formatter = formatters.TerminalFormatter()
+        return _formatter
 
     def colorize(filename, contents):
         # Don't want any errors.
         try:
             lexer = lexers.get_lexer_for_filename(filename, stripnl=False)
-            formatter = formatters.TerminalFormatter()
+            formatter = get_formatter()
             return highlight(contents, lexer, formatter).encode(
                 gdb.host_charset(), "backslashreplace"
             )
         except:
             return None
 
+    class HandleNasmComments(TokenMergeFilter):
+        @staticmethod
+        def fix_comments(lexer, stream):
+            in_comment = False
+            for ttype, value in stream:
+                if ttype is Error and value == "#":
+                    in_comment = True
+                if in_comment:
+                    if ttype is Text and value == "\n":
+                        in_comment = False
+                    else:
+                        ttype = Comment.Single
+                yield ttype, value
+
+        def filter(self, lexer, stream):
+            f = HandleNasmComments.fix_comments
+            return super().filter(lexer, f(lexer, stream))
+
+    _asm_lexers = {}
+
+    def __get_asm_lexer(gdbarch):
+        lexer_type = "asm"
+        try:
+            # For an i386 based architecture, in 'intel' mode, use the nasm
+            # lexer.
+            flavor = gdb.parameter("disassembly-flavor")
+            if flavor == "intel" and gdbarch.name()[:4] == "i386":
+                lexer_type = "nasm"
+        except:
+            # If GDB is built without i386 support then attempting to fetch
+            # the 'disassembly-flavor' parameter will throw an error, which we
+            # ignore.
+            pass
+
+        global _asm_lexers
+        if lexer_type not in _asm_lexers:
+            _asm_lexers[lexer_type] = lexers.get_lexer_by_name(lexer_type)
+            _asm_lexers[lexer_type].add_filter(HandleNasmComments())
+            _asm_lexers[lexer_type].add_filter("raiseonerror")
+        return _asm_lexers[lexer_type]
+
     def colorize_disasm(content, gdbarch):
         # Don't want any errors.
         try:
-            lexer = lexers.get_lexer_by_name("asm")
-            formatter = formatters.TerminalFormatter()
+            lexer = __get_asm_lexer(gdbarch)
+            formatter = get_formatter()
             return highlight(content, lexer, formatter).rstrip().encode()
         except:
-            return None
+            return content
 
 except:
 
-- 
2.25.4

Re: [PATCH 0/3] Improvements for Pygments based disassembly styling

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> This series was inspired by this stackoverflow post:
>
>   https://stackoverflow.com/questions/73491793/why-is-there-a-%c2%b1-in-lea-rax-rip-%c2%b1-0xeb3
>
> which highlighted some issues with the Python Pygments based
> disassembler styling that was being used for all architectures in GDB
> 12.

I've gone ahead and pushed this series.  Let me know if anyone runs into
any problems.

Thanks,
Andrew

>
> The next release of GDB will use libopcodes styling for many of the
> most popular architectures, but lots of the smaller architectures are
> still using Pygments, and Pygments can still be used for all
> architectures if a user chooses to disable use of libopcodes styling,
> though I don't know why they would want to do that.
>
> ---
>
> Andrew Burgess (3):
>   gdb/testsuite: extend styling test for libopcodes styling
>   gdb: improve disassembler styling when Pygments raises an exception
>   gdb/disasm: better intel flavour disassembly styling with Pygments
>
>  gdb/disasm.c                     |  69 ++++++++++----
>  gdb/disasm.h                     |  21 +++-
>  gdb/python/lib/gdb/styling.py    |  59 +++++++++++-
>  gdb/testsuite/gdb.base/style.exp | 159 +++++++++++++++++++++++++------
>  4 files changed, 250 insertions(+), 58 deletions(-)
>
> -- 
> 2.25.4

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Simon Marchi]

> +# 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"] \

Any reason you are using \004 here, instead of end?  I don't quite
understand why, but it seems to cause some random failures.  Running the
test under `taskset -c 2` makes it fail most of the time.  Running it
with check-read1 makes it fail consistently:

  FAIL: gdb.base/style.exp: capture_command_output for x/1i *main

When changing \004 for end, it passes.  I don't have an explanation why
though.

Simon

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

>> +# 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"] \
>
> Any reason you are using \004 here, instead of end?  I don't quite
> understand why, but it seems to cause some random failures.  Running the
> test under `taskset -c 2` makes it fail most of the time.  Running it
> with check-read1 makes it fail consistently:
>
>   FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>
> When changing \004 for end, it passes.  I don't have an explanation why
> though.

It'll be a copy&paste.  There's a couple of other places in the
testsuite where this pattern is used.

The patch below changes all three to use 'end'.  How's this?

Thanks,
Andrew

---

commit eccf885d59a785cc3d9a76bd75184a9b9420f6b9
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Oct 8 16:58:00 2022 +0100

    gdb/testsuite: use 'end' and the end of python blocks
    
    Use the keyword 'end' to terminate blocks of Python code being sent to
    GDB, rather than sending \004.

diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index c6ed996c280..146e2b6d757 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -444,7 +444,7 @@ proc test_disassembler_error_handling { } {
 		 "def replacement_colorize_disasm(content,gdbarch):" \
 		 "  return None" \
 		 "gdb.styling.colorize_disasm = replacement_colorize_disasm" \
-		 "\004"] \
+		 "end"] \
 	    "setup replacement colorize_disasm function" \
 	    true
 
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-groups.exp b/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
index 9503e6c10f5..f2cf8b0e6ec 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
@@ -102,5 +102,5 @@ gdb_py_test_silent_cmd \
 	 "    raise gdb.GdbError (\"miss-matched names\")" \
 	 "  if (r1 != r2):" \
 	 "    raise gdb.GdbError (\"miss-matched objects\")" \
-	 "\004" ] \
+	 "end" ] \
     "check names and objects match" 1
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-names.exp b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
index d3c600ffc0f..62c47e8200e 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-names.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
@@ -102,7 +102,7 @@ gdb_py_test_silent_cmd \
 	 "    raise gdb.GdbError (\"miss-matched names\")" \
 	 "  if (r1 != r2):" \
 	 "    raise gdb.GdbError (\"miss-matched objects\")" \
-	 "\004" ] \
+	 "end" ] \
     "check names and objects match" 1
 
 # Ensure that the '.find' method on the iterator returns the same

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Simon Marchi]

On 2022-10-08 12:00, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>>> +# 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"] \
>>
>> Any reason you are using \004 here, instead of end?  I don't quite
>> understand why, but it seems to cause some random failures.  Running the
>> test under `taskset -c 2` makes it fail most of the time.  Running it
>> with check-read1 makes it fail consistently:
>>
>>   FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>>
>> When changing \004 for end, it passes.  I don't have an explanation why
>> though.
> 
> It'll be a copy&paste.  There's a couple of other places in the
> testsuite where this pattern is used.
> 
> The patch below changes all three to use 'end'.  How's this?
> 
> Thanks,
> Andrew

This is fine with me, but again, I didn't dig into it so I don't really
know what sending a \004 was causing,

Thanks!

Simon

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 2022-10-08 12:00, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>>> +# 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"] \
>>>
>>> Any reason you are using \004 here, instead of end?  I don't quite
>>> understand why, but it seems to cause some random failures.  Running the
>>> test under `taskset -c 2` makes it fail most of the time.  Running it
>>> with check-read1 makes it fail consistently:
>>>
>>>   FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>>>
>>> When changing \004 for end, it passes.  I don't have an explanation why
>>> though.
>> 
>> It'll be a copy&paste.  There's a couple of other places in the
>> testsuite where this pattern is used.
>> 
>> The patch below changes all three to use 'end'.  How's this?
>> 
>> Thanks,
>> Andrew
>
> This is fine with me, but again, I didn't dig into it so I don't really
> know what sending a \004 was causing,

Hi,

Given you were seeing problems with the use of \004, and the tests
definitely were not about whether \004 could be used or not, I've pushed
the patch I proposed (exact version is below).

Thanks,
Andrew

---

commit f91822c2b9f9fed0c2717b17f380e5216502ea32
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Oct 8 16:58:00 2022 +0100

    gdb/testsuite: use 'end' at the end of python blocks
    
    Within the testsuite, use the keyword 'end' to terminate blocks of
    Python code being sent to GDB, rather than sending \004.  I could only
    find three instances of this, all in tests that I originally wrote.  I
    have no memory of there being any special reason why I used \004
    instead of 'end' - I assume I copied this from somewhere else that has
    since changed.
    
    Non of the tests being changed here are specifically about whether
    \004 can be used to terminate a Python block, so I think switching to
    the more standard 'end' keyword is the right choice.

diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index c6ed996c280..146e2b6d757 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -444,7 +444,7 @@ proc test_disassembler_error_handling { } {
 		 "def replacement_colorize_disasm(content,gdbarch):" \
 		 "  return None" \
 		 "gdb.styling.colorize_disasm = replacement_colorize_disasm" \
-		 "\004"] \
+		 "end"] \
 	    "setup replacement colorize_disasm function" \
 	    true
 
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-groups.exp b/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
index 9503e6c10f5..f2cf8b0e6ec 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-groups.exp
@@ -102,5 +102,5 @@ gdb_py_test_silent_cmd \
 	 "    raise gdb.GdbError (\"miss-matched names\")" \
 	 "  if (r1 != r2):" \
 	 "    raise gdb.GdbError (\"miss-matched objects\")" \
-	 "\004" ] \
+	 "end" ] \
     "check names and objects match" 1
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-names.exp b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
index d3c600ffc0f..62c47e8200e 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-names.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
@@ -102,7 +102,7 @@ gdb_py_test_silent_cmd \
 	 "    raise gdb.GdbError (\"miss-matched names\")" \
 	 "  if (r1 != r2):" \
 	 "    raise gdb.GdbError (\"miss-matched objects\")" \
-	 "\004" ] \
+	 "end" ] \
     "check names and objects match" 1
 
 # Ensure that the '.find' method on the iterator returns the same

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Tom de Vries]

On 10/10/22 11:32, Andrew Burgess via Gdb-patches wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2022-10-08 12:00, Andrew Burgess wrote:
>>> Simon Marchi <simark@simark.ca> writes:
>>>
>>>>> +# 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"] \
>>>>
>>>> Any reason you are using \004 here, instead of end?  I don't quite
>>>> understand why, but it seems to cause some random failures.  Running the
>>>> test under `taskset -c 2` makes it fail most of the time.  Running it
>>>> with check-read1 makes it fail consistently:
>>>>
>>>>    FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>>>>
>>>> When changing \004 for end, it passes.  I don't have an explanation why
>>>> though.
>>>
>>> It'll be a copy&paste.  There's a couple of other places in the
>>> testsuite where this pattern is used.
>>>
>>> The patch below changes all three to use 'end'.  How's this?
>>>
>>> Thanks,
>>> Andrew
>>
>> This is fine with me, but again, I didn't dig into it so I don't really
>> know what sending a \004 was causing,
> 
> Hi,
> 
> Given you were seeing problems with the use of \004, and the tests
> definitely were not about whether \004 could be used or not, I've pushed
> the patch I proposed (exact version is below).
> 

I found this independently and filed a PR ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=29664 ) and wrote the 
same fix (in the same three test-cases), and was about to commit ...

The FAIL is caused by using gdb_py_test_silent_cmd, which adds a "\n" at 
the end of the command in combination with "\004", causing a stray "\n" 
that generates an extra prompt, which makes the prompt matching 
unpredictable.

Thanks,
- Tom

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

>> +# 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"] \
>
> Any reason you are using \004 here, instead of end?  I don't quite
> understand why, but it seems to cause some random failures.  Running the
> test under `taskset -c 2` makes it fail most of the time.  Running it
> with check-read1 makes it fail consistently:
>
>   FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>
> When changing \004 for end, it passes.  I don't have an explanation why
> though.

Turns out the explanation is really simple once you know what it is :)

gdb_py_test_silent_cmd forwards to gdb_test_multiple.  The command I was
sending above, once multi_line_input has done its thing is like this:

  python\ndef ...\n  return None\ngdb.styling....\n\004

This gets sent to gdb_test_multiple, which then sends the command to
GDB, along with a trailing newline - the newline which it thinks will
cause the command to dispatch to GDB.  So our command ends like this:

   ....\n\004\n

Of course, a \004 doesn't need a trailing newline, it is handled
immediately, so that final '\n' is not part of the multi-line python
block.

And so, after \004 has cause the Python block to finish, and the
contents to be processed by the Python interpreter, we still have an
unspent newline character in the input stream, this causes GDB to print
a second prompt, the gdb.log contains:

  (gdb) PASS: gdb.base/style.exp: have style markers when Pygments is working fine
  python
  >def replacement_colorize_disasm(content,gdbarch):
  >  return None
  >gdb.styling.colorize_disasm = replacement_colorize_disasm
  >quit
  (gdb) PASS: gdb.base/style.exp: setup replacement colorize_disasm function
  
  (gdb) FAIL: gdb.base/style.exp: capture_command_output for x/1i *main

And so, we have a race with DejaGnu.  If the two prompts appear slowly
enough, then DejaGnu will match the first one with the end of the 'setup
replacement colorize_disasm function', and the second prompt will be
used to prematurely match with the end of the 'capture_command_output
for x/1i *main' test.

If the two prompts appear quickly then both prompts will be matched as
part of the 'setup replacement colorize_disasm function' test.

The patch I already pushed will resolve this issue completely.

Thanks for finding this bug.

Andrew

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Andrew Burgess]

Tom de Vries <tdevries@suse.de> writes:

> On 10/10/22 11:32, Andrew Burgess via Gdb-patches wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 2022-10-08 12:00, Andrew Burgess wrote:
>>>> Simon Marchi <simark@simark.ca> writes:
>>>>
>>>>>> +# 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"] \
>>>>>
>>>>> Any reason you are using \004 here, instead of end?  I don't quite
>>>>> understand why, but it seems to cause some random failures.  Running the
>>>>> test under `taskset -c 2` makes it fail most of the time.  Running it
>>>>> with check-read1 makes it fail consistently:
>>>>>
>>>>>    FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>>>>>
>>>>> When changing \004 for end, it passes.  I don't have an explanation why
>>>>> though.
>>>>
>>>> It'll be a copy&paste.  There's a couple of other places in the
>>>> testsuite where this pattern is used.
>>>>
>>>> The patch below changes all three to use 'end'.  How's this?
>>>>
>>>> Thanks,
>>>> Andrew
>>>
>>> This is fine with me, but again, I didn't dig into it so I don't really
>>> know what sending a \004 was causing,
>> 
>> Hi,
>> 
>> Given you were seeing problems with the use of \004, and the tests
>> definitely were not about whether \004 could be used or not, I've pushed
>> the patch I proposed (exact version is below).
>> 
>
> I found this independently and filed a PR ( 
> https://sourceware.org/bugzilla/show_bug.cgi?id=29664 ) and wrote the 
> same fix (in the same three test-cases), and was about to commit ...
>
> The FAIL is caused by using gdb_py_test_silent_cmd, which adds a "\n" at 
> the end of the command in combination with "\004", causing a stray "\n" 
> that generates an extra prompt, which makes the prompt matching 
> unpredictable.

Apologies for the duplicated effort.  Also, thanks for the explanation,
which, if I'd spotted sooner would have saved me investigating the
problem.

Again, thanks for picking up after me, and apologies for wasting your
time.

thanks,
Andrew

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 4455 bytes --]

On 10/10/22 13:16, Andrew Burgess wrote:
> Tom de Vries <tdevries@suse.de> writes:
> 
>> On 10/10/22 11:32, Andrew Burgess via Gdb-patches wrote:
>>> Simon Marchi <simark@simark.ca> writes:
>>>
>>>> On 2022-10-08 12:00, Andrew Burgess wrote:
>>>>> Simon Marchi <simark@simark.ca> writes:
>>>>>
>>>>>>> +# 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"] \
>>>>>>
>>>>>> Any reason you are using \004 here, instead of end?  I don't quite
>>>>>> understand why, but it seems to cause some random failures.  Running the
>>>>>> test under `taskset -c 2` makes it fail most of the time.  Running it
>>>>>> with check-read1 makes it fail consistently:
>>>>>>
>>>>>>     FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>>>>>>
>>>>>> When changing \004 for end, it passes.  I don't have an explanation why
>>>>>> though.
>>>>>
>>>>> It'll be a copy&paste.  There's a couple of other places in the
>>>>> testsuite where this pattern is used.
>>>>>
>>>>> The patch below changes all three to use 'end'.  How's this?
>>>>>
>>>>> Thanks,
>>>>> Andrew
>>>>
>>>> This is fine with me, but again, I didn't dig into it so I don't really
>>>> know what sending a \004 was causing,
>>>
>>> Hi,
>>>
>>> Given you were seeing problems with the use of \004, and the tests
>>> definitely were not about whether \004 could be used or not, I've pushed
>>> the patch I proposed (exact version is below).
>>>
>>
>> I found this independently and filed a PR (
>> https://sourceware.org/bugzilla/show_bug.cgi?id=29664 ) and wrote the
>> same fix (in the same three test-cases), and was about to commit ...
>>
>> The FAIL is caused by using gdb_py_test_silent_cmd, which adds a "\n" at
>> the end of the command in combination with "\004", causing a stray "\n"
>> that generates an extra prompt, which makes the prompt matching
>> unpredictable.
> 
> Apologies for the duplicated effort.  Also, thanks for the explanation,
> which, if I'd spotted sooner would have saved me investigating the
> problem.
> 
> Again, thanks for picking up after me, and apologies for wasting your
> time.
> 

Hi Andrew,

thanks for following up.

Luckily most of my time went to investigating the root cause (which had 
me puzzled for quite a bit), so that's not effort wasted.

Also, I forgot to check the mailing list to see if the FAIL was already 
noted there, so that's on me.

Anyway, I propose to error out in gdb_test_multiple on this specific 
pattern.  Dealing with ^C / ^D for now.

Not tested yet (I'll start testing in now in combination with the 
proposed patch to fix the build).

WDYT?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Detect-trailing-C-D-in-command.patch --]
[-- Type: text/x-patch, Size: 2213 bytes --]

[gdb/testsuite] Detect trailing ^C/^D in command

Detect a trailing ^C/^D in the command argument of gdb_test, and error out.

Tested on x86_64-linux.

---
 gdb/testsuite/gdb.testsuite/gdb-test.exp | 34 ++++++++++++++++++++++++++------
 gdb/testsuite/lib/gdb.exp                |  4 ++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.testsuite/gdb-test.exp b/gdb/testsuite/gdb.testsuite/gdb-test.exp
index 2ce8eb31d73..e891f81e7b0 100644
--- a/gdb/testsuite/gdb.testsuite/gdb-test.exp
+++ b/gdb/testsuite/gdb.testsuite/gdb-test.exp
@@ -19,10 +19,32 @@ clean_restart
 
 # Check that a command with trailing newline triggers an error.
 
-set results [catch {
-    gdb_test "pwd\n" ".*" "cmd with trailing newline"
-} output]
+with_test_prefix "cmd with trailing newline" {
+    set results [catch {
+	gdb_test "pwd\n" ".*" "pwd"
+    } output]
 
-gdb_assert { $results == 1 }
-set expected_error_msg "Invalid trailing newline in \"pwd\n\" command"
-gdb_assert { [string equal $output $expected_error_msg] }
+    gdb_assert { $results == 1 }
+    set expected_error_msg "Invalid trailing newline in \"pwd\n\" command"
+    gdb_assert { [string equal $output $expected_error_msg] }
+}
+
+with_test_prefix "cmd with trailing control code" {
+    foreach_with_prefix control_code {^C ^D} {
+	switch $control_code {
+	    ^C {
+		set cmd "\003"
+	    }
+	    ^D {
+		set cmd "\004"
+	    }
+	}
+	set results [catch {
+	    gdb_test $cmd ".*" "control code"
+	} output]
+
+	gdb_assert { $results == 1 }
+	set expected_error_msg "Invalid trailing control code in \"$cmd\" command"
+	gdb_assert { [string equal $output $expected_error_msg] }
+    }
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 5f0acfaa530..ac28ede1b08 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1003,6 +1003,10 @@ proc gdb_test_multiple { command message args } {
 	error "Invalid trailing newline in \"$command\" command"
     }
 
+    if [string match "*\[\003\004\]" $command] {
+	error "Invalid trailing control code in \"$command\" command"
+    }
+
     if [string match "*\[\r\n\]*" $message] {
 	error "Invalid newline in \"$message\" test"
     }

Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception

[Andrew Burgess]

Tom de Vries <tdevries@suse.de> writes:

> On 10/10/22 13:16, Andrew Burgess wrote:
>> Tom de Vries <tdevries@suse.de> writes:
>> 
>>> On 10/10/22 11:32, Andrew Burgess via Gdb-patches wrote:
>>>> Simon Marchi <simark@simark.ca> writes:
>>>>
>>>>> On 2022-10-08 12:00, Andrew Burgess wrote:
>>>>>> Simon Marchi <simark@simark.ca> writes:
>>>>>>
>>>>>>>> +# 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"] \
>>>>>>>
>>>>>>> Any reason you are using \004 here, instead of end?  I don't quite
>>>>>>> understand why, but it seems to cause some random failures.  Running the
>>>>>>> test under `taskset -c 2` makes it fail most of the time.  Running it
>>>>>>> with check-read1 makes it fail consistently:
>>>>>>>
>>>>>>>     FAIL: gdb.base/style.exp: capture_command_output for x/1i *main
>>>>>>>
>>>>>>> When changing \004 for end, it passes.  I don't have an explanation why
>>>>>>> though.
>>>>>>
>>>>>> It'll be a copy&paste.  There's a couple of other places in the
>>>>>> testsuite where this pattern is used.
>>>>>>
>>>>>> The patch below changes all three to use 'end'.  How's this?
>>>>>>
>>>>>> Thanks,
>>>>>> Andrew
>>>>>
>>>>> This is fine with me, but again, I didn't dig into it so I don't really
>>>>> know what sending a \004 was causing,
>>>>
>>>> Hi,
>>>>
>>>> Given you were seeing problems with the use of \004, and the tests
>>>> definitely were not about whether \004 could be used or not, I've pushed
>>>> the patch I proposed (exact version is below).
>>>>
>>>
>>> I found this independently and filed a PR (
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=29664 ) and wrote the
>>> same fix (in the same three test-cases), and was about to commit ...
>>>
>>> The FAIL is caused by using gdb_py_test_silent_cmd, which adds a "\n" at
>>> the end of the command in combination with "\004", causing a stray "\n"
>>> that generates an extra prompt, which makes the prompt matching
>>> unpredictable.
>> 
>> Apologies for the duplicated effort.  Also, thanks for the explanation,
>> which, if I'd spotted sooner would have saved me investigating the
>> problem.
>> 
>> Again, thanks for picking up after me, and apologies for wasting your
>> time.
>> 
>
> Hi Andrew,
>
> thanks for following up.
>
> Luckily most of my time went to investigating the root cause (which had 
> me puzzled for quite a bit), so that's not effort wasted.
>
> Also, I forgot to check the mailing list to see if the FAIL was already 
> noted there, so that's on me.
>
> Anyway, I propose to error out in gdb_test_multiple on this specific 
> pattern.  Dealing with ^C / ^D for now.
>
> Not tested yet (I'll start testing in now in combination with the 
> proposed patch to fix the build).
>
> WDYT?

This seems like a sensible precaution, I like it.

Patch looks good.

Thanks,
Andrew

>
> Thanks,
> - Tom
> [gdb/testsuite] Detect trailing ^C/^D in command
>
> Detect a trailing ^C/^D in the command argument of gdb_test, and error out.
>
> Tested on x86_64-linux.
>
> ---
>  gdb/testsuite/gdb.testsuite/gdb-test.exp | 34 ++++++++++++++++++++++++++------
>  gdb/testsuite/lib/gdb.exp                |  4 ++++
>  2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.testsuite/gdb-test.exp b/gdb/testsuite/gdb.testsuite/gdb-test.exp
> index 2ce8eb31d73..e891f81e7b0 100644
> --- a/gdb/testsuite/gdb.testsuite/gdb-test.exp
> +++ b/gdb/testsuite/gdb.testsuite/gdb-test.exp
> @@ -19,10 +19,32 @@ clean_restart
>  
>  # Check that a command with trailing newline triggers an error.
>  
> -set results [catch {
> -    gdb_test "pwd\n" ".*" "cmd with trailing newline"
> -} output]
> +with_test_prefix "cmd with trailing newline" {
> +    set results [catch {
> +	gdb_test "pwd\n" ".*" "pwd"
> +    } output]
>  
> -gdb_assert { $results == 1 }
> -set expected_error_msg "Invalid trailing newline in \"pwd\n\" command"
> -gdb_assert { [string equal $output $expected_error_msg] }
> +    gdb_assert { $results == 1 }
> +    set expected_error_msg "Invalid trailing newline in \"pwd\n\" command"
> +    gdb_assert { [string equal $output $expected_error_msg] }
> +}
> +
> +with_test_prefix "cmd with trailing control code" {
> +    foreach_with_prefix control_code {^C ^D} {
> +	switch $control_code {
> +	    ^C {
> +		set cmd "\003"
> +	    }
> +	    ^D {
> +		set cmd "\004"
> +	    }
> +	}
> +	set results [catch {
> +	    gdb_test $cmd ".*" "control code"
> +	} output]
> +
> +	gdb_assert { $results == 1 }
> +	set expected_error_msg "Invalid trailing control code in \"$cmd\" command"
> +	gdb_assert { [string equal $output $expected_error_msg] }
> +    }
> +}
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 5f0acfaa530..ac28ede1b08 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -1003,6 +1003,10 @@ proc gdb_test_multiple { command message args } {
>  	error "Invalid trailing newline in \"$command\" command"
>      }
>  
> +    if [string match "*\[\003\004\]" $command] {
> +	error "Invalid trailing control code in \"$command\" command"
> +    }
> +
>      if [string match "*\[\r\n\]*" $message] {
>  	error "Invalid newline in \"$message\" test"
>      }