[v2] Add QUIT to inner loop in value_print_array_elements
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
Commit Message
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(-)
base-commit: a122bb30358a3aabe314bcc918494e8b50c336fb
Comments
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
@@ -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;
@@ -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);
@@ -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)
{
@@ -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. */