[v6,4/5] gdb: introduce ability to disable frame unwinders

  From: Guinevere Larsen <blarsen@redhat.com>

Sometimes, in the GDB testsuite, we want to test the ability of specific
unwinders to handle some piece of code. Usually this is done by trying
to outsmart GDB, or by coercing the compiler to remove information that
GDB would rely on.  Both approaches have problems as GDB gets smarter
with time, and that compilers might differ in version and behavior, or
simply introduce new useful information. This was requested back in 2003
in PR backtrace/8434.

To improve our ability to thoroughly test GDB, this patch introduces a
new maintenance command that allows a user to disable some unwinders,
based on either the name of the unwinder or on its class. With this
change, it will now be possible for GDB to not find any frame unwinders
for a given frame, which would previously cause GDB to assert. GDB will
now check if any frame unwinder has been disabled, and if some has, it
will just error out instead of asserting.

Unwinders can be disabled or re-enabled in 3 different ways:
* Disabling/enabling all at once (using '-all').
* By specifying an unwinder class to be disabled (option '-class').
* By specifying the name of an unwinder (option '-name').

If you give no options to the command, GDB assumes the input is an
unwinder class. '-class' would make no difference if used, is just here
for completeness.

This command is meant to be used once the inferior is already at the
desired location for the test. An example session would be:

(gdb) start
Temporary breakpoint 1, main () at omp.c:17
17          func();
(gdb) maint frame-unwinder disable ARCH
(gdb) bt
\#0  main () at omp.c:17
(gdb) maint frame-unwinder enable ARCH
(gdb) cont
Continuing.

This commit is a more generic version of commit 3c3bb0580be0,
and so, based on the final paragraph of the commit message:
    gdb: Add switch to disable DWARF stack unwinders
<...>
    If in the future we find ourselves adding more switches to disable
    different unwinders, then we should probably move to a more generic
    solution, and remove this patch.
this patch also reverts 3c3bb0580be0

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=8434
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
---
 gdb/NEWS                                      |  17 ++
 gdb/doc/gdb.texinfo                           |  49 +++--
 gdb/dwarf2/frame-tailcall.c                   |   3 -
 gdb/dwarf2/frame.c                            |  30 ---
 gdb/dwarf2/frame.h                            |   6 -
 gdb/frame-unwind.c                            | 198 +++++++++++++++++-
 gdb/frame-unwind.h                            |   9 +
 .../gdb.base/frame-info-consistent.exp        |   6 +-
 gdb/testsuite/gdb.base/frame-unwind-disable.c |  21 ++
 .../gdb.base/frame-unwind-disable.exp         | 137 ++++++++++++
 gdb/testsuite/gdb.base/maint.exp              |   4 -
 11 files changed, 399 insertions(+), 81 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c
 create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp
  

> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index b79d9eaf033..9d8005b72b2 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -92,6 +92,26 @@ frame_unwinder_class_str (frame_unwind_class uclass)
>    return unwind_class_conversion[uclass];
>  }
>  
> +/* Case insensitive search for a frame_unwind_class based on the given
> +   string.  */
> +static enum frame_unwind_class
> +str_to_frame_unwind_class (const char *class_str)
> +{
> +  const char *prefix = "FRAME_UNWIND_";
> +
> +  /* Skip the prefix if present.  */
> +  if (strncasecmp (class_str, prefix, strlen (prefix)) == 0)
> +    class_str += strlen (prefix);
> +
> +  for (int i = 0; i < UNWIND_CLASS_NUMBER; i++)
> +    {
> +      if (strcasecmp (unwind_class_conversion[i], class_str) == 0)
> +	return (frame_unwind_class) i;
> +    }
> +
> +  error (_("Unknown frame unwind class: %s"), class_str);

Did you consider my response, on the previous version, about
FRAME_UNWIND_* unnecessarily exposing internal implementation details?
After more reflexion, I really think FRAME_UNWIND_* doesn't belong in
the user interface.

> +}
> +
>  void
>  frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
>  				const struct frame_unwind *unwinder)
> @@ -180,25 +200,55 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>  
>    const struct frame_unwind *unwinder_from_target;
>  
> +  /* If we see a disabled unwinder, we assume some test is being run on
> +     GDB, and we don't want to assert at the end of this function.  */
> +  bool seen_disabled_unwinder = false;
> +  /* Lambda to factor out the logic of checking if an unwinder is enabled,
> +     testing it and otherwise recording if we saw a disable unwinder.  */
> +  auto test_unwinder = [&] (const struct frame_unwind *unwinder)
> +    {
> +      if (unwinder != nullptr)
> +	{
> +	  if (unwinder->enabled ())
> +	    {
> +	      if (frame_unwind_try_unwinder (this_frame, this_cache,
> +					     unwinder))
> +		return true;
> +	    }
> +	  else
> +	    seen_disabled_unwinder = true;
> +	}
> +      return false;
> +    };

Since you moved this logic into a lambda, you could write it in "early
return" style, which I think is more readable.

  auto test_unwinder = [&] (const struct frame_unwind *unwinder)
    {
      if (unwinder == nullptr)
	return false;

      if (!unwinder->enabled ())
	{
	  seen_disabled_unwinder = true;
	  return false;
	}

      return static_cast<bool> (frame_unwind_try_unwinder (this_frame,
							   this_cache,
							   unwinder));
    };

Note about the static_cast: if you first push a trivial patch to make
frame_unwind_try_unwinder return bool instead of int, you can get rid of
it.

> +
>    unwinder_from_target = target_get_unwinder ();
> -  if (unwinder_from_target != NULL
> -      && frame_unwind_try_unwinder (this_frame, this_cache,
> -				   unwinder_from_target))
> +  if (test_unwinder (unwinder_from_target))
>      return;
>  
>    unwinder_from_target = target_get_tailcall_unwinder ();
> -  if (unwinder_from_target != NULL
> -      && frame_unwind_try_unwinder (this_frame, this_cache,
> -				   unwinder_from_target))
> +  if (test_unwinder (unwinder_from_target))
>      return;

I would get rid of the unwinder_from_target variable and inline the
function calls.

>  
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    std::vector<const frame_unwind *> &table = get_frame_unwind_table (gdbarch);
>    for (auto unwinder : table)
> -    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
> -      return;
> +    {
> +      /*
> +      if (!unwinder->enabled ())
> +	{
> +	  seen_disabled_unwinder = true;
> +	  continue;
> +	}
> +      if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
> +      */

I think this is meant to be removed.

> @@ -239,6 +245,9 @@ class frame_unwind
>       unwinding.  */
>    frame_unwind_class m_unwinder_class;
>    const frame_data *m_unwind_data;
> +
> +  /* Whether this unwinder can be used when sniffing.  */
> +  mutable bool m_enabled = true;

I'm wondering if this is an appropriate use of mutable.  Doesn't
enabling / disabling an unwinder change its state, conceptually?

> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.c b/gdb/testsuite/gdb.base/frame-unwind-disable.c
> new file mode 100644
> index 00000000000..078517c011e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.c
> @@ -0,0 +1,21 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2024 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int main ()

int
main ()

> +{
> +    return 0;

Indent this line with this spaces.

> +}
> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.exp b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
> new file mode 100644
> index 00000000000..049374ae08f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
> @@ -0,0 +1,137 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test multiple situations in which we may use the maintenance command to
> +# disable and enable frame unwinders, and check that they really are
> +# disabled when they say the are.
> +
> +standard_testfile .c
> +
> +# Proc to check if the unwinder of the given name is in the desired state.
> +# STATE can be either Y or N.
> +proc check_unwinder_state { unwinder_name state {testname ""} } {
> +    set should_pass false
> +    set command "maint info frame-unwinders"
> +    if {${testname} == ""} {
> +	set testname "checking state ${state} for ${unwinder_name}"
> +    }
> +    gdb_test_multiple "${command}" "${testname}" -lbl {
> +	-re "${unwinder_name}\\s+\\w+\\s+\\w+\\s+${state}\\s+" {
> +	    set should_pass true
> +	    exp_continue
> +	}
> +	-re "${command}" {
> +	    exp_continue
> +	}
> +	-re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+" {
> +	    exp_continue
> +	}
> +	-re -wrap "" {
> +	    # We can't use -wrap here because it expects the line with start
> +	    # with \r\n, but the previous commands eat that

I don't understand this comment saying we can't use -wrap.  Not sure
where it would be used.

> +	    gdb_assert {${should_pass} == true} "${gdb_test_name}"

The `== true` seems unnecessary.

> +	}
> +    }
> +}
> +
> +proc check_unwinder_class { unwinder_class state {testname ""} } {

Can you add a comment for this proc?

> +    set command "maint info frame-unwinders"
> +    set seen_arches 0
> +    set correct_state 0
> +    if {$testname == ""} {
> +	set testname "checking if ${unwinder_class} state is ${state}"
> +    }
> +    gdb_test_multiple "${command}" "${testname}" -lbl {
> +	-re "^\[^\r\n\]+\\s+\\w+\\s+${unwinder_class}\\s+\(\[YN\]\)\\s+(?=\r\n)" {
> +	    # The unwinder name may have multiple words, so we need to use the
> +	    # more generic [^\r\n] pattern to match the unwinders.
> +	    set should_pass true
> +	    incr seen_arches
> +	    set cur_state $expect_out(1,string)
> +	    if {$cur_state == $state} {
> +		incr correct_state
> +	    }
> +	    exp_continue
> +	}
> +	-re "${command}" {
> +	    exp_continue
> +	}
> +	-re -wrap "" {
> +	    gdb_assert {${correct_state} == ${seen_arches}} "${gdb_test_name}"

Instead of keeping correct_state and seen_arches, it looks like you
could just keep a "should_pass" flag like you did above.  Except it
would start as true and be set to false when encountering an unwinder
with the wrong state.

> +	}
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} ]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "couldn't run to main"
> +    return
> +}
> +
> +# Test disabling all unwinders
> +check_unwinder_class "\\w+" "Y" \
> +    "Checking all unwinders before any changes"

Our test names are not capitalized.

For the test names, I would suggest dropping the "Checking" word, it's
implied that we check stuff.  Maybe "unwinder states before any changes"
or "all unwinders enabled before any changes" (same idea for other test
names below).

Simon
  

On 10/10/24 5:05 PM, Simon Marchi wrote:
>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>> index b79d9eaf033..9d8005b72b2 100644
>> --- a/gdb/frame-unwind.c
>> +++ b/gdb/frame-unwind.c
>> @@ -92,6 +92,26 @@ frame_unwinder_class_str (frame_unwind_class uclass)
>>     return unwind_class_conversion[uclass];
>>   }
>>   
>> +/* Case insensitive search for a frame_unwind_class based on the given
>> +   string.  */
>> +static enum frame_unwind_class
>> +str_to_frame_unwind_class (const char *class_str)
>> +{
>> +  const char *prefix = "FRAME_UNWIND_";
>> +
>> +  /* Skip the prefix if present.  */
>> +  if (strncasecmp (class_str, prefix, strlen (prefix)) == 0)
>> +    class_str += strlen (prefix);
>> +
>> +  for (int i = 0; i < UNWIND_CLASS_NUMBER; i++)
>> +    {
>> +      if (strcasecmp (unwind_class_conversion[i], class_str) == 0)
>> +	return (frame_unwind_class) i;
>> +    }
>> +
>> +  error (_("Unknown frame unwind class: %s"), class_str);
> Did you consider my response, on the previous version, about
> FRAME_UNWIND_* unnecessarily exposing internal implementation details?
> After more reflexion, I really think FRAME_UNWIND_* doesn't belong in
> the user interface.
Oh oops. I read it, agreed with you then got pulled into something else 
and when I came back I was sure I had already done it. I have fixed it now.
>
>> +}
>> +
>>   void
>>   frame_unwind_prepend_unwinder (struct gdbarch *gdbarch,
>>   				const struct frame_unwind *unwinder)
>> @@ -180,25 +200,55 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>>   
>>     const struct frame_unwind *unwinder_from_target;
>>   
>> +  /* If we see a disabled unwinder, we assume some test is being run on
>> +     GDB, and we don't want to assert at the end of this function.  */
>> +  bool seen_disabled_unwinder = false;
>> +  /* Lambda to factor out the logic of checking if an unwinder is enabled,
>> +     testing it and otherwise recording if we saw a disable unwinder.  */
>> +  auto test_unwinder = [&] (const struct frame_unwind *unwinder)
>> +    {
>> +      if (unwinder != nullptr)
>> +	{
>> +	  if (unwinder->enabled ())
>> +	    {
>> +	      if (frame_unwind_try_unwinder (this_frame, this_cache,
>> +					     unwinder))
>> +		return true;
>> +	    }
>> +	  else
>> +	    seen_disabled_unwinder = true;
>> +	}
>> +      return false;
>> +    };
> Since you moved this logic into a lambda, you could write it in "early
> return" style, which I think is more readable.
>
>    auto test_unwinder = [&] (const struct frame_unwind *unwinder)
>      {
>        if (unwinder == nullptr)
> 	return false;
>
>        if (!unwinder->enabled ())
> 	{
> 	  seen_disabled_unwinder = true;
> 	  return false;
> 	}
>
>        return static_cast<bool> (frame_unwind_try_unwinder (this_frame,
> 							   this_cache,
> 							   unwinder));
>      };
>
> Note about the static_cast: if you first push a trivial patch to make
> frame_unwind_try_unwinder return bool instead of int, you can get rid of
> it.
Makes sense. I'll push the trivial patch as soon as I can (hopefully 
still today).
>
>> +
>>     unwinder_from_target = target_get_unwinder ();
>> -  if (unwinder_from_target != NULL
>> -      && frame_unwind_try_unwinder (this_frame, this_cache,
>> -				   unwinder_from_target))
>> +  if (test_unwinder (unwinder_from_target))
>>       return;
>>   
>>     unwinder_from_target = target_get_tailcall_unwinder ();
>> -  if (unwinder_from_target != NULL
>> -      && frame_unwind_try_unwinder (this_frame, this_cache,
>> -				   unwinder_from_target))
>> +  if (test_unwinder (unwinder_from_target))
>>       return;
> I would get rid of the unwinder_from_target variable and inline the
> function calls.
Fixed
>
>>   
>>     struct gdbarch *gdbarch = get_frame_arch (this_frame);
>>     std::vector<const frame_unwind *> &table = get_frame_unwind_table (gdbarch);
>>     for (auto unwinder : table)
>> -    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
>> -      return;
>> +    {
>> +      /*
>> +      if (!unwinder->enabled ())
>> +	{
>> +	  seen_disabled_unwinder = true;
>> +	  continue;
>> +	}
>> +      if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
>> +      */
> I think this is meant to be removed.
Yes, sorry. Fixed.
>
>> @@ -239,6 +245,9 @@ class frame_unwind
>>        unwinding.  */
>>     frame_unwind_class m_unwinder_class;
>>     const frame_data *m_unwind_data;
>> +
>> +  /* Whether this unwinder can be used when sniffing.  */
>> +  mutable bool m_enabled = true;
> I'm wondering if this is an appropriate use of mutable.  Doesn't
> enabling / disabling an unwinder change its state, conceptually?

This question makes sense. My original idea is that what really is 
immutable is the function pointers, and that is the reason why all 
frame_unwind objects were declared as constant, and in this case, 
enabling or disabling the unwinder is a mutable characteristic, because 
we're not storing state, we're storing functionality.

However, I guess the c++-ification makes it so the object doesn't need 
to store functionality anymore, so we can drop making them consts, and 
then m_enable can just be a regular member.I'll make the change for v7

>> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.c b/gdb/testsuite/gdb.base/frame-unwind-disable.c
>> new file mode 100644
>> index 00000000000..078517c011e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.c
>> @@ -0,0 +1,21 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2024 Free Software Foundation, Inc.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see<http://www.gnu.org/licenses/>.  */
>> +
>> +int main ()
> int
> main ()
Fixed
>
>> +{
>> +    return 0;
> Indent this line with this spaces.
fixed
>
>> +}
>> diff --git a/gdb/testsuite/gdb.base/frame-unwind-disable.exp b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
>> new file mode 100644
>> index 00000000000..049374ae08f
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/frame-unwind-disable.exp
>> @@ -0,0 +1,137 @@
>> +# Copyright 2024 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see<http://www.gnu.org/licenses/>.
>> +
>> +# Test multiple situations in which we may use the maintenance command to
>> +# disable and enable frame unwinders, and check that they really are
>> +# disabled when they say the are.
>> +
>> +standard_testfile .c
>> +
>> +# Proc to check if the unwinder of the given name is in the desired state.
>> +# STATE can be either Y or N.
>> +proc check_unwinder_state { unwinder_name state {testname ""} } {
>> +    set should_pass false
>> +    set command "maint info frame-unwinders"
>> +    if {${testname} == ""} {
>> +	set testname "checking state ${state} for ${unwinder_name}"
>> +    }
>> +    gdb_test_multiple "${command}" "${testname}" -lbl {
>> +	-re "${unwinder_name}\\s+\\w+\\s+\\w+\\s+${state}\\s+" {
>> +	    set should_pass true
>> +	    exp_continue
>> +	}
>> +	-re "${command}" {
>> +	    exp_continue
>> +	}
>> +	-re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+" {
>> +	    exp_continue
>> +	}
>> +	-re -wrap "" {
>> +	    # We can't use -wrap here because it expects the line with start
>> +	    # with \r\n, but the previous commands eat that
> I don't understand this comment saying we can't use -wrap.  Not sure
> where it would be used.
This is a left-over from a previous iteration. I removed it.
>
>> +	    gdb_assert {${should_pass} == true} "${gdb_test_name}"
> The `== true` seems unnecessary.
fixed
>> +	}
>> +    }
>> +}
>> +
>> +proc check_unwinder_class { unwinder_class state {testname ""} } {
> Can you add a comment for this proc?

Fixed, added the following,

+# Check if all unwinders of class UNWINDER_CLASS are in the state STATE.
+# STATE can be either Y or N.
+# UNWINDER_CLASS can be one of: GDB, ARCH, EXTENSION, DEBUGINFO. It can
+# also be \\w+ if checking all unwinders.

>
>> +    set command "maint info frame-unwinders"
>> +    set seen_arches 0
>> +    set correct_state 0
>> +    if {$testname == ""} {
>> +	set testname "checking if ${unwinder_class} state is ${state}"
>> +    }
>> +    gdb_test_multiple "${command}" "${testname}" -lbl {
>> +	-re "^\[^\r\n\]+\\s+\\w+\\s+${unwinder_class}\\s+\(\[YN\]\)\\s+(?=\r\n)" {
>> +	    # The unwinder name may have multiple words, so we need to use the
>> +	    # more generic [^\r\n] pattern to match the unwinders.
>> +	    set should_pass true
>> +	    incr seen_arches
>> +	    set cur_state $expect_out(1,string)
>> +	    if {$cur_state == $state} {
>> +		incr correct_state
>> +	    }
>> +	    exp_continue
>> +	}
>> +	-re "${command}" {
>> +	    exp_continue
>> +	}
>> +	-re -wrap "" {
>> +	    gdb_assert {${correct_state} == ${seen_arches}} "${gdb_test_name}"
> Instead of keeping correct_state and seen_arches, it looks like you
> could just keep a "should_pass" flag like you did above.  Except it
> would start as true and be set to false when encountering an unwinder
> with the wrong state.

Changed.

I usually don't like this paradigm because if I mess up the failure 
detection, we don't actually get to test the feature, we just run 
through the motions, but in this case it's already been battle tested, I 
think it should be fine

>
>> +	}
>> +    }
>> +}
>> +
>> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} ]} {
>> +    return -1
>> +}
>> +
>> +if {![runto_main]} {
>> +    untested "couldn't run to main"
>> +    return
>> +}
>> +
>> +# Test disabling all unwinders
>> +check_unwinder_class "\\w+" "Y" \
>> +    "Checking all unwinders before any changes"
> Our test names are not capitalized.
>
> For the test names, I would suggest dropping the "Checking" word, it's
> implied that we check stuff.  Maybe "unwinder states before any changes"
> or "all unwinders enabled before any changes" (same idea for other test
> names below).
>
> Simon
>
Fixed all of these for v7. I'll just wait for you to review patch 5 
before sending it.
  

Just one comment. In any case:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Guinevere Larsen <guinevere@redhat.com> writes:
>
>>  create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c
>>  create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp

I'm getting these failures on aarch64-linux (but not x86_64-linux) when
running "make check", but not with "make check-read1":

FAIL: gdb.base/frame-unwind-disable.exp: successfully enabled dummy unwinder (timeout)
FAIL: gdb.base/frame-unwind-disable.exp: check if dummy continues enabled (timeout)
FAIL: gdb.base/frame-unwind-disable.exp: maint frame-unwinder enable -class GDB

I'm still trying to understand what's going on.
  

Sorry, I missed this draft message I had stored. I have a few nits here,
and one question.

But the Reviewed-by stands.

Guinevere Larsen <guinevere@redhat.com> writes:

> @@ -180,25 +200,55 @@ frame_unwind_find_by_frame (const frame_info_ptr &this_frame, void **this_cache)
>  
>    const struct frame_unwind *unwinder_from_target;
>  
> +  /* If we see a disabled unwinder, we assume some test is being run on
> +     GDB, and we don't want to assert at the end of this function.  */
> +  bool seen_disabled_unwinder = false;
> +  /* Lambda to factor out the logic of checking if an unwinder is enabled,
> +     testing it and otherwise recording if we saw a disable unwinder.  */
> +  auto test_unwinder = [&] (const struct frame_unwind *unwinder)
> +    {
> +      if (unwinder != nullptr)
> +	{
> +	  if (unwinder->enabled ())
> +	    {
> +	      if (frame_unwind_try_unwinder (this_frame, this_cache,
> +					     unwinder))
> +		return true;
> +	    }
> +	  else
> +	    seen_disabled_unwinder = true;
> +	}
> +      return false;
> +    };
> +
>    unwinder_from_target = target_get_unwinder ();
> -  if (unwinder_from_target != NULL
> -      && frame_unwind_try_unwinder (this_frame, this_cache,
> -				   unwinder_from_target))
> +  if (test_unwinder (unwinder_from_target))
>      return;
>  
>    unwinder_from_target = target_get_tailcall_unwinder ();
> -  if (unwinder_from_target != NULL
> -      && frame_unwind_try_unwinder (this_frame, this_cache,
> -				   unwinder_from_target))
> +  if (test_unwinder (unwinder_from_target))
>      return;
>  
>    struct gdbarch *gdbarch = get_frame_arch (this_frame);
>    std::vector<const frame_unwind *> &table = get_frame_unwind_table (gdbarch);
>    for (auto unwinder : table)
> -    if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
> -      return;
> +    {
> +      /*
> +      if (!unwinder->enabled ())
> +	{
> +	  seen_disabled_unwinder = true;
> +	  continue;
> +	}
> +      if (frame_unwind_try_unwinder (this_frame, this_cache, unwinder))
> +      */

Remove the commented-out code.

> +      if (test_unwinder (unwinder))
> +	return;
> +    }
>  
> -  internal_error (_("frame_unwind_find_by_frame failed"));
> +  if (seen_disabled_unwinder)
> +    error (_("frame_unwind_find_by_frame failed"));
> +  else
> +    internal_error (_("frame_unwind_find_by_frame failed"));
>  }

<snip>

> +/* Helper function to both enable and disable frame unwinders.
> +   if ENABLE is true, this call will be enabling unwinders,
> +   otherwise the unwinders will be disabled.  */
> +static void
> +enable_disable_frame_unwinders (const char *args, int from_tty, bool enable)
> +{
> +  if (args == nullptr)
> +    {
> +      if (enable)
> +	error (_("Specify which frame unwinder(s) should be enabled"));
> +      else
> +	error (_("Specify which frame unwinder(s) should be disabled"));
> +    }
> +
> +  struct gdbarch* gdbarch = current_inferior ()->arch ();
> +  std::vector<const frame_unwind *> unwinder_list
> +    = gdbarch_unwinder_list (gdbarch);

Shouldn't this call get_frame_unwind_table () instead, to make sure that
the list is initialized?

> +
> +  /* First see if the user wants to change all unwinders.  */
> +  if (check_for_argument (&args, "-all"))
> +    {
> +      for (const frame_unwind *u : unwinder_list)
> +	u->set_enabled (enable);
> +
> +      reinit_frame_cache ();
> +      return;
> +    }
> +
> +  /* If user entered a specific unwinder name, handle it here.  If the
> +     unwinder is already at the expected state, error out.  */
> +  if (check_for_argument (&args, "-name"))
> +    {
> +      bool did_something = false;
> +      for (const frame_unwind *unwinder : unwinder_list)
> +	{
> +	  if (strcasecmp (unwinder->name (), args) == 0)
> +	    {
> +	      if (unwinder->enabled () == enable)
> +		{
> +		  if (unwinder->enabled ())
> +		    error (_("unwinder %s is already enabled"),
> +			     unwinder->name ());
> +		  else
> +		    error (_("unwinder %s is already disabled"),
> +			     unwinder->name ());
> +		}
> +	      unwinder->set_enabled (enable);
> +
> +	      did_something = true;
> +	      break;
> +	    }
> +	}
> +      if (!did_something)
> +	error (_("couldn't find unwinder named %s"),args);

Space after comma.

> +    }
> +  else
> +    {
> +      /* Discard '-class' argument, if provided.  */
> +      check_for_argument (&args, "-class");
> +      enum frame_unwind_class dclass = str_to_frame_unwind_class (args);
> +      for (auto unwinder: unwinder_list)
> +	{
> +	  if (unwinder->unwinder_class () == dclass)
> +	    unwinder->set_enabled (enable);
> +	}
> +    }
> +
> +  reinit_frame_cache ();
> +}

<snip>

> @@ -422,4 +559,45 @@ _initialize_frame_unwind ()
>  	   _("List the frame unwinders currently in effect, "
>  	     "starting with the highest priority."),
>  	   &maintenanceinfolist);
> +
> +  /* Add "maint frame-unwinder disable/enable".  */
> +  static struct cmd_list_element *maint_frame_unwinder;
> +
> +  add_basic_prefix_cmd ("frame-unwinder", class_maintenance,
> +			_("Commands handling frame unwinders."),
> +			&maint_frame_unwinder, 0, &maintenancelist);
> +
> +  add_cmd ("disable", class_maintenance, maintenance_disable_frame_unwinders,
> +	   _("\
> +Disable one or more frame unwinder(s).\n\
> +Usage: maint frame-unwinder disable [-all | -name NAME | [-class] CLASS]\n\
> +\n\
> +These are the meanings of the options:\n\
> +\t-all    - All available unwinders will be disabled\n\
> +\t-name   - NAME is the exact name of the frame unwinder to be disabled\n\
> +\t-class  - CLASS is the class of unwinders to be disabled.\n\

I suggest adding the "Valid classes are:" test here as well, like in the
"enable" command.

> +\t\tGDB       - Unwinders added by GDB core;\n\
> +\t\tEXTENSION - Unwinders added by extension languages;\n\
> +\t\tDEBUGINFO - Unwinders that handle debug information;\n\
> +\t\tARCH      - Unwinders that use architecture-specific information;\n\
> +\n\
> +UNWINDER and NAME are case insensitive."),
> +	   & maint_frame_unwinder);
> +
> +  add_cmd ("enable", class_maintenance, maintenance_enable_frame_unwinders,
> +	   _("\
> +Enable one or more frame unwinder(s).\n\
> +Usage: maint frame-unwinder enable [-all | -name NAME | [-class] CLASS]\n\
> +\n\
> +These are the meanings of the options:\n\
> +\t-all    - All available unwinders will be enabled\n\
> +\t-name   - NAME is the exact name of the frame unwinder to be enabled\n\
> +\t-class  - CLASS is the class of unwinders to be enabled. Valid classes are:\n\
> +\t\tGDB       - Unwinders added by GDB core;\n\
> +\t\tEXTENSION - Unwinders added by extension languages;\n\
> +\t\tDEBUGINFO - Unwinders that handle debug information;\n\
> +\t\tARCH      - Unwinders that use architecture-specific information;\n\
> +\n\
> +UNWINDER and NAME are case insensitive."),
> +	   & maint_frame_unwinder);
>  }
  

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>
>> Guinevere Larsen <guinevere@redhat.com> writes:
>>
>>>  create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c
>>>  create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp
>
> I'm getting these failures on aarch64-linux (but not x86_64-linux) when
> running "make check", but not with "make check-read1":
>
> FAIL: gdb.base/frame-unwind-disable.exp: successfully enabled dummy unwinder (timeout)
> FAIL: gdb.base/frame-unwind-disable.exp: check if dummy continues enabled (timeout)
> FAIL: gdb.base/frame-unwind-disable.exp: maint frame-unwinder enable -class GDB
>
> I'm still trying to understand what's going on.

I found the problem.

> +# Proc to check if the unwinder of the given name is in the desired state.
> +# STATE can be either Y or N.
> +proc check_unwinder_state { unwinder_name state {testname ""} } {
> +    set should_pass false
> +    set command "maint info frame-unwinders"
> +    if {${testname} == ""} {
> +	set testname "checking state ${state} for ${unwinder_name}"
> +    }
> +    gdb_test_multiple "${command}" "${testname}" -lbl {
> +	-re "${unwinder_name}\\s+\\w+\\s+\\w+\\s+${state}\\s+" {

The failures go away when I add "(?=\r\n)" at the end of this pattern ...

> +	    set should_pass true
> +	    exp_continue
> +	}
> +	-re "${command}" {
> +	    exp_continue
> +	}
> +	-re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+" {

... and also at the end of this pattern.

> +	    exp_continue
> +	}
> +	-re -wrap "" {
> +	    # We can't use -wrap here because it expects the line with start
> +	    # with \r\n, but the previous commands eat that
> +	    gdb_assert {${should_pass} == true} "${gdb_test_name}"
> +	}
> +    }
> +}
  

On 10/12/24 12:43 AM, Thiago Jung Bauermann wrote:
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>
>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>>
>>> Guinevere Larsen <guinevere@redhat.com> writes:
>>>
>>>>   create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.c
>>>>   create mode 100644 gdb/testsuite/gdb.base/frame-unwind-disable.exp
>> I'm getting these failures on aarch64-linux (but not x86_64-linux) when
>> running "make check", but not with "make check-read1":
>>
>> FAIL: gdb.base/frame-unwind-disable.exp: successfully enabled dummy unwinder (timeout)
>> FAIL: gdb.base/frame-unwind-disable.exp: check if dummy continues enabled (timeout)
>> FAIL: gdb.base/frame-unwind-disable.exp: maint frame-unwinder enable -class GDB
>>
>> I'm still trying to understand what's going on.
> I found the problem.
>
>> +# Proc to check if the unwinder of the given name is in the desired state.
>> +# STATE can be either Y or N.
>> +proc check_unwinder_state { unwinder_name state {testname ""} } {
>> +    set should_pass false
>> +    set command "maint info frame-unwinders"
>> +    if {${testname} == ""} {
>> +	set testname "checking state ${state} for ${unwinder_name}"
>> +    }
>> +    gdb_test_multiple "${command}" "${testname}" -lbl {
>> +	-re "${unwinder_name}\\s+\\w+\\s+\\w+\\s+${state}\\s+" {
> The failures go away when I add "(?=\r\n)" at the end of this pattern ...
>
>> +	    set should_pass true
>> +	    exp_continue
>> +	}
>> +	-re "${command}" {
>> +	    exp_continue
>> +	}
>> +	-re "\\w+\\s+\\w+\\s+\\w+\\s+\\w+\\s+" {
> ... and also at the end of this pattern.
>
>> +	    exp_continue
>> +	}
>> +	-re -wrap "" {
>> +	    # We can't use -wrap here because it expects the line with start
>> +	    # with \r\n, but the previous commands eat that
>> +	    gdb_assert {${should_pass} == true} "${gdb_test_name}"
>> +	}
>> +    }
>> +}

AH ha! thanks for this. I don't think I would have solved this on my own 
lol.

I've added this fix and the nits from the other reply to my local 
version, thanks for the review