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

[Andrew Burgess <aburgess@redhat.com>]

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