* [PATCH 1/3] gdb/testsuite: extend styling test for libopcodes styling
2022-08-30 14:16 [PATCH 0/3] Improvements for Pygments based disassembly styling Andrew Burgess
@ 2022-08-30 14:16 ` Andrew Burgess
2022-08-30 14:16 ` [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception Andrew Burgess
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-08-30 14:16 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-08-30 14:16 [PATCH 0/3] Improvements for Pygments based disassembly styling Andrew Burgess
2022-08-30 14:16 ` [PATCH 1/3] gdb/testsuite: extend styling test for libopcodes styling Andrew Burgess
@ 2022-08-30 14:16 ` Andrew Burgess
2022-10-08 2:25 ` Simon Marchi
2022-08-30 14:16 ` [PATCH 3/3] gdb/disasm: better intel flavour disassembly styling with Pygments Andrew Burgess
2022-10-02 16:38 ` [PATCH 0/3] Improvements for Pygments based disassembly styling Andrew Burgess
3 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-08-30 14:16 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-08-30 14:16 ` [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception Andrew Burgess
@ 2022-10-08 2:25 ` Simon Marchi
2022-10-08 16:00 ` Andrew Burgess
2022-10-10 11:03 ` Andrew Burgess
0 siblings, 2 replies; 14+ messages in thread
From: Simon Marchi @ 2022-10-08 2:25 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
> +# 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-08 2:25 ` Simon Marchi
@ 2022-10-08 16:00 ` Andrew Burgess
2022-10-08 16:01 ` Simon Marchi
2022-10-10 11:03 ` Andrew Burgess
1 sibling, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-10-08 16:00 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-08 16:00 ` Andrew Burgess
@ 2022-10-08 16:01 ` Simon Marchi
2022-10-10 9:32 ` Andrew Burgess
0 siblings, 1 reply; 14+ messages in thread
From: Simon Marchi @ 2022-10-08 16:01 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-08 16:01 ` Simon Marchi
@ 2022-10-10 9:32 ` Andrew Burgess
2022-10-10 10:31 ` Tom de Vries
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-10-10 9:32 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-10 9:32 ` Andrew Burgess
@ 2022-10-10 10:31 ` Tom de Vries
2022-10-10 11:16 ` Andrew Burgess
0 siblings, 1 reply; 14+ messages in thread
From: Tom de Vries @ 2022-10-10 10:31 UTC (permalink / raw)
To: Andrew Burgess, Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-10 10:31 ` Tom de Vries
@ 2022-10-10 11:16 ` Andrew Burgess
2022-10-10 13:22 ` Tom de Vries
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Burgess @ 2022-10-10 11:16 UTC (permalink / raw)
To: Tom de Vries, Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-10 11:16 ` Andrew Burgess
@ 2022-10-10 13:22 ` Tom de Vries
2022-10-10 14:04 ` Andrew Burgess
0 siblings, 1 reply; 14+ messages in thread
From: Tom de Vries @ 2022-10-10 13:22 UTC (permalink / raw)
To: Andrew Burgess, Simon Marchi, gdb-patches
[-- 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"
}
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-10 13:22 ` Tom de Vries
@ 2022-10-10 14:04 ` Andrew Burgess
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-10 14:04 UTC (permalink / raw)
To: Tom de Vries, Simon Marchi, gdb-patches
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"
> }
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception
2022-10-08 2:25 ` Simon Marchi
2022-10-08 16:00 ` Andrew Burgess
@ 2022-10-10 11:03 ` Andrew Burgess
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-10 11:03 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] gdb/disasm: better intel flavour disassembly styling with Pygments
2022-08-30 14:16 [PATCH 0/3] Improvements for Pygments based disassembly styling Andrew Burgess
2022-08-30 14:16 ` [PATCH 1/3] gdb/testsuite: extend styling test for libopcodes styling Andrew Burgess
2022-08-30 14:16 ` [PATCH 2/3] gdb: improve disassembler styling when Pygments raises an exception Andrew Burgess
@ 2022-08-30 14:16 ` Andrew Burgess
2022-10-02 16:38 ` [PATCH 0/3] Improvements for Pygments based disassembly styling Andrew Burgess
3 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-08-30 14:16 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 0/3] Improvements for Pygments based disassembly styling
2022-08-30 14:16 [PATCH 0/3] Improvements for Pygments based disassembly styling Andrew Burgess
` (2 preceding siblings ...)
2022-08-30 14:16 ` [PATCH 3/3] gdb/disasm: better intel flavour disassembly styling with Pygments Andrew Burgess
@ 2022-10-02 16:38 ` Andrew Burgess
3 siblings, 0 replies; 14+ messages in thread
From: Andrew Burgess @ 2022-10-02 16:38 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread