[PATCH v2] Add QUIT to inner loop in value_print_array_elements

[PATCH v2] 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.  In v2 this also moves a
QUIT in the string-printing code, pushing it into a more important
loop.

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/char-print.c   | 4 ++--
 gdb/f-valprint.c   | 5 +++++
 gdb/valprint.c     | 2 ++
 4 files changed, 11 insertions(+), 2 deletions(-)

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/char-print.c b/gdb/char-print.c
index f3425ac2cbc..ba1cec4ff9f 100644
--- a/gdb/char-print.c
+++ b/gdb/char-print.c
@@ -278,6 +278,8 @@ wchar_printer::count_next_character (wchar_iterator *iter,
 
       while (1)
 	{
+	  QUIT;
+
 	  /* Get the next character.  */
 	  d.num_chars = iter->iterate (&d.result, &chars, &d.buf, &d.buflen);
 
@@ -526,8 +528,6 @@ wchar_printer::print (struct ui_file *stream, const gdb_byte *string,
     {
       int r;
 
-      QUIT;
-
       /* Grab the next character and repeat count.  */
       r = count_next_character (&iter, &converted_chars);
 
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: a122bb30358a3aabe314bcc918494e8b50c336fb
-- 
2.55.0

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

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

> 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.  In v2 this also moves a
> QUIT in the string-printing code, pushing it into a more important
> loop.
>
> 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

I find it odd that your text mentions PR exp/32681, but the bug link is
for 17613.

I think the body text should mention 17613 and what it's about even if
it's just:

  This work is not interruptible (see PR gdb/17613), and it is somewhat
  slow (see PR exp/32681).

Also, I think that every bug mentioned should get a 'Bug: ...' line as
these are super easy to search for in the git log.

I agree that testing this would be hard, the last thing we need are more
racy tests.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew




> ---
>  gdb/ada-valprint.c | 2 ++
>  gdb/char-print.c   | 4 ++--
>  gdb/f-valprint.c   | 5 +++++
>  gdb/valprint.c     | 2 ++
>  4 files changed, 11 insertions(+), 2 deletions(-)
>
> 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/char-print.c b/gdb/char-print.c
> index f3425ac2cbc..ba1cec4ff9f 100644
> --- a/gdb/char-print.c
> +++ b/gdb/char-print.c
> @@ -278,6 +278,8 @@ wchar_printer::count_next_character (wchar_iterator *iter,
>  
>        while (1)
>  	{
> +	  QUIT;
> +
>  	  /* Get the next character.  */
>  	  d.num_chars = iter->iterate (&d.result, &chars, &d.buf, &d.buflen);
>  
> @@ -526,8 +528,6 @@ wchar_printer::print (struct ui_file *stream, const gdb_byte *string,
>      {
>        int r;
>  
> -      QUIT;
> -
>        /* Grab the next character and repeat count.  */
>        r = count_next_character (&iter, &converted_chars);
>  
> 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: a122bb30358a3aabe314bcc918494e8b50c336fb
> -- 
> 2.55.0