[PATCH] Add QUIT to inner loop in value_print_array_elements

[PATCH] Add QUIT to inner loop in value_print_array_elements

[Tom Tromey]

When printing an array, some work is done to detect repeated elements.
This work is not interruptible, and it is somewhat slow (see
PR exp/32681).

This patch adds QUITs to the relevant spots.

I am not sure there is a way to write a non-racy test for this.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17613
---
 gdb/ada-valprint.c | 2 ++
 gdb/f-valprint.c   | 5 +++++
 gdb/valprint.c     | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
index d66f9f84d52..020478bdf40 100644
--- a/gdb/ada-valprint.c
+++ b/gdb/ada-valprint.c
@@ -183,6 +183,8 @@ val_print_packed_array_elements (struct type *type, const gdb_byte *valaddr,
 					   bitsize, elttype);
       while (1)
 	{
+	  QUIT;
+
 	  /* Make sure to free any values in the inner loop.  */
 	  scoped_value_mark free_values;
 
diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c
index d0b2030554f..2b54c8bf1c0 100644
--- a/gdb/f-valprint.c
+++ b/gdb/f-valprint.c
@@ -36,6 +36,7 @@
 #include "cli/cli-style.h"
 #include "gdbarch.h"
 #include "f-array-walker.h"
+#include "event-top.h"
 
 static void f77_get_dynamic_length_of_aggregate (struct type *);
 
@@ -188,6 +189,8 @@ class fortran_array_printer_impl : public fortran_array_walker_base_impl
 			  struct type *elt_type, LONGEST elt_off,
 			  LONGEST index, bool last_p)
   {
+    QUIT;
+
     size_t dim_indx = m_dimension - 1;
     struct type *elt_type_prev = m_elt_type_prev;
     LONGEST elt_off_prev = m_elt_off_prev;
@@ -262,6 +265,8 @@ class fortran_array_printer_impl : public fortran_array_walker_base_impl
     LONGEST elt_off_prev = m_elt_off_prev;
     bool repeated = false;
 
+    QUIT;
+
     if (m_options.repeat_count_threshold < UINT_MAX
 	&& elt_type_prev != nullptr)
       {
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 83a442dcef5..7e41943f66f 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -2038,6 +2038,8 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
 
 	  while (rep1 < len)
 	    {
+	      QUIT;
+
 	      /* When printing large arrays this spot is called frequently, so
 		 clean up temporary values asap to prevent allocating a large
 		 amount of them.  */

base-commit: 123e4a5eab2db300b44288dcc1a29c903f9622f9
-- 
2.55.0

Re: [PATCH] Add QUIT to inner loop in value_print_array_elements

[Guinevere Larsen]

On 9/22/26 5:12 PM, Tom Tromey wrote:
> When printing an array, some work is done to detect repeated elements.
> This work is not interruptible, and it is somewhat slow (see
> PR exp/32681).
>
> This patch adds QUITs to the relevant spots.
>
> I am not sure there is a way to write a non-racy test for this.

Hi Tom!

The code change seems pretty obvious. I tried to give a test some 
thought, and based on how long it takes to print a 1M sized array, we 
might be able to add a wait for a second or two and attempt to send a 
Ctrl+C signal, and check for a timeout, I think. Without real testing 
(and using the code in the bugzilla example), I wonder if this is workable:

gdb_test_no_output "set max-value-size unlimited"
send_gdb "p *p\n"
# make sure that we're in the code looking for repeats
sleep 1
gdb_test_multiple "\003" "cancel printing" {
   -ex -wrap "" {
     pass $gdb_test_name
   }
}

I think this may work. The main racy issue I can see would be false 
positive, where we send QUIT before reaching the problem code, which 
isn't as much of an issue as the false negatives I don't think.

That said, I just tried applying this and running that test manually and 
it didn't quit, there's still a large uninterruptible pause going on, so 
I think you may have missed a spot?

-- 
Cheers,
Guinevere Larsen
it/its
she/her (deprecated)

>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17613
> ---
>   gdb/ada-valprint.c | 2 ++
>   gdb/f-valprint.c   | 5 +++++
>   gdb/valprint.c     | 2 ++
>   3 files changed, 9 insertions(+)
>
> diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
> index d66f9f84d52..020478bdf40 100644
> --- a/gdb/ada-valprint.c
> +++ b/gdb/ada-valprint.c
> @@ -183,6 +183,8 @@ val_print_packed_array_elements (struct type *type, const gdb_byte *valaddr,
>   					   bitsize, elttype);
>         while (1)
>   	{
> +	  QUIT;
> +
>   	  /* Make sure to free any values in the inner loop.  */
>   	  scoped_value_mark free_values;
>   
> diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c
> index d0b2030554f..2b54c8bf1c0 100644
> --- a/gdb/f-valprint.c
> +++ b/gdb/f-valprint.c
> @@ -36,6 +36,7 @@
>   #include "cli/cli-style.h"
>   #include "gdbarch.h"
>   #include "f-array-walker.h"
> +#include "event-top.h"
>   
>   static void f77_get_dynamic_length_of_aggregate (struct type *);
>   
> @@ -188,6 +189,8 @@ class fortran_array_printer_impl : public fortran_array_walker_base_impl
>   			  struct type *elt_type, LONGEST elt_off,
>   			  LONGEST index, bool last_p)
>     {
> +    QUIT;
> +
>       size_t dim_indx = m_dimension - 1;
>       struct type *elt_type_prev = m_elt_type_prev;
>       LONGEST elt_off_prev = m_elt_off_prev;
> @@ -262,6 +265,8 @@ class fortran_array_printer_impl : public fortran_array_walker_base_impl
>       LONGEST elt_off_prev = m_elt_off_prev;
>       bool repeated = false;
>   
> +    QUIT;
> +
>       if (m_options.repeat_count_threshold < UINT_MAX
>   	&& elt_type_prev != nullptr)
>         {
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index 83a442dcef5..7e41943f66f 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -2038,6 +2038,8 @@ value_print_array_elements (struct value *val, struct ui_file *stream,
>   
>   	  while (rep1 < len)
>   	    {
> +	      QUIT;
> +
>   	      /* When printing large arrays this spot is called frequently, so
>   		 clean up temporary values asap to prevent allocating a large
>   		 amount of them.  */
>
> base-commit: 123e4a5eab2db300b44288dcc1a29c903f9622f9

Re: [PATCH] Add QUIT to inner loop in value_print_array_elements

[Kevin Buettner]

On Tue, 22 Sep 2026 14:12:02 -0600
Tom Tromey <tromey@adacore.com> wrote:

> When printing an array, some work is done to detect repeated elements.
> This work is not interruptible, and it is somewhat slow (see
> PR exp/32681).
> 
> This patch adds QUITs to the relevant spots.

My AI-enabled patch review workflow confirmed Gwen's finding.
It said:

    Finding 1 -- the dominant uninterruptible loop is in char-print.c,
    not the array code.

    I profiled the exact repro from the thread (perf record on a
    patched build, 8 MiB static char p[], "print p"). 99% of samples
    are:

      print_command_1 -> print_value -> value_print -> c_value_print
	-> c_value_print_array -> printstr -> generic_printstr
	-> count_next_character -> iterate -> gdb_iconv -> iconv
	(98.98%)

    value_print_array_elements never appears. The reason is
    gdb/c-valprint.c:259: a char array that passes
    c_textual_element_type is printed through
    current_language->printstr -- the string path -- which entirely
    bypasses the code this patch modifies.

    Two measurements confirm this on the patched build (maint time
    1, print elements 10, max-value-size unlimited). A char array of
    8 MiB, printed through generic_printstr and iconv, takes 43.3 s.
    An int array of the same 8 MiB, which goes through
    value_print_array_elements, takes 0.29 s.

    So Guinevere's "you may have missed a spot" is correct, and the
    missed spot is char-print.c:279. The outer loop there, at
    char-print.c:525, already has a QUIT at line 529, but it only
    runs once per distinct character -- for a uniform array the
    inner while (1) consumes all 8M characters inside a single outer
    iteration, which is why the existing QUIT never fires.

    The fix is small: event-top.h is already included at
    char-print.c:21, so adding a QUIT as the first statement of the
    while (1) body at line 279 would do it, either in this patch or
    in an immediate follow-up. The commit message might also be
    softened: "This patch adds QUITs to the relevant spots" reads as
    a complete fix for the PR 17613 symptom, which it is not.

AI model used was GLM 5.3 Flash.  It had 3 other findings too, but
they were minor.

Kevin