[1/6] Fix "ptype INTERNAL_FUNC" (PR gdb/30105)

  Currently, looking at the type of an internal function, like below,
hits an odd error:

 (gdb) ptype $_isvoid
 type = <internal function>type not handled in c_type_print_varspec_prefix()

That is an error thrown from
c-typeprint.c:c_type_print_varspec_prefix, where it reads:

    ...
    case TYPE_CODE_DECFLOAT:
    case TYPE_CODE_FIXED_POINT:
      /* These types need no prefix.  They are listed here so that
	 gcc -Wall will reveal any types that haven't been handled.  */
      break;
    default:
      error (_("type not handled in c_type_print_varspec_prefix()"));
      break;

Internal function types have type code TYPE_CODE_INTERNAL_FUNCTION,
which is not explicitly handled by that switch.

That comment quoted above says that gcc -Wall will reveal any types
that haven't been handled, but that's not actually true, at least with
modern GCCs.  You would need to enable -Wswitch-enum for that, which
we don't.  If I do enable that warning, then I see that we're missing
handling for the following type codes:

   TYPE_CODE_INTERNAL_FUNCTION,
   TYPE_CODE_MODULE,
   TYPE_CODE_NAMELIST,
   TYPE_CODE_XMETHOD

TYPE_CODE_MODULE and TYPE_CODE_NAMELIST and Fortran-specific, so it'd
be a little weird to handle them here.

I tried to reach this code with TYPE_CODE_XMETHOD, but couldn't figure
out how to.  ptype on an xmethod isn't treated specially, it just
complains that the method doesn't exist.  I've extended the
gdb.python/py-xmethods.exp testcase to make sure of that.

My thinking is that whatever type code we add next, the most likely
scenario is that it won't need any special handling, so we'd just be
adding another case to that "do nothing" list.  If we do need special
casing for whatever type code, I think that tests added at the same
time as the feature would uncover it anyhow.  If we do miss adding the
special casing, then it still looks better to me to print the type
somewhat incompletely than to error out and make it harder for users
to debug whatever they need.  So I think that the best thing to do
here is to just remove all those explicit "do nothing" cases, along
with the error default case.

After doing that, I decided to write a testcase that iterates over all
supported languages doing "ptype INTERNAL_FUNC".  That revealed that
Pascal has a similar problem, except the default case hits a
gdb_assert instead of an error:

 (gdb) with language pascal -- ptype $_isvoid
 type =
 ../../src/gdb/p-typeprint.c:268: internal-error: type_print_varspec_prefix: unexpected type
 A problem internal to GDB has been detected,
 further debugging may prove unreliable.

That is fixed by this patch in the same way.

You'll notice that the new testcase special-cases the Ada expected
output:

	} elseif {$lang == "ada"} {
	    gdb_test "ptype \$_isvoid" "<<internal function>>"
	} else {
	    gdb_test "ptype \$_isvoid" "<internal function>"
	}

That will be subject of the following patch.

Change-Id: I81aec03523cceb338b5180a0b4c2e4ad26b4c4db
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30105
---
 gdb/c-typeprint.c                             | 51 -------------------
 gdb/p-typeprint.c                             | 46 -----------------
 .../gdb.base/internal-functions-ptype.exp     | 42 +++++++++++++++
 gdb/testsuite/gdb.python/py-xmethods.exp      |  8 +++
 4 files changed, 50 insertions(+), 97 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/internal-functions-ptype.exp
  

Pedro Alves <pedro@palves.net> writes:

> Currently, looking at the type of an internal function, like below,
> hits an odd error:
>
>  (gdb) ptype $_isvoid
>  type = <internal function>type not handled in c_type_print_varspec_prefix()
>
> That is an error thrown from
> c-typeprint.c:c_type_print_varspec_prefix, where it reads:
>
>     ...
>     case TYPE_CODE_DECFLOAT:
>     case TYPE_CODE_FIXED_POINT:
>       /* These types need no prefix.  They are listed here so that
> 	 gcc -Wall will reveal any types that haven't been handled.  */
>       break;
>     default:
>       error (_("type not handled in c_type_print_varspec_prefix()"));
>       break;
>
> Internal function types have type code TYPE_CODE_INTERNAL_FUNCTION,
> which is not explicitly handled by that switch.
>
> That comment quoted above says that gcc -Wall will reveal any types
> that haven't been handled, but that's not actually true, at least with
> modern GCCs.  You would need to enable -Wswitch-enum for that, which
> we don't.  If I do enable that warning, then I see that we're missing
> handling for the following type codes:
>
>    TYPE_CODE_INTERNAL_FUNCTION,
>    TYPE_CODE_MODULE,
>    TYPE_CODE_NAMELIST,
>    TYPE_CODE_XMETHOD
>
> TYPE_CODE_MODULE and TYPE_CODE_NAMELIST and Fortran-specific, so it'd
> be a little weird to handle them here.
>
> I tried to reach this code with TYPE_CODE_XMETHOD, but couldn't figure
> out how to.  ptype on an xmethod isn't treated specially, it just
> complains that the method doesn't exist.  I've extended the
> gdb.python/py-xmethods.exp testcase to make sure of that.
>
> My thinking is that whatever type code we add next, the most likely
> scenario is that it won't need any special handling, so we'd just be
> adding another case to that "do nothing" list.  If we do need special
> casing for whatever type code, I think that tests added at the same
> time as the feature would uncover it anyhow.  If we do miss adding the
> special casing, then it still looks better to me to print the type
> somewhat incompletely than to error out and make it harder for users
> to debug whatever they need.  So I think that the best thing to do
> here is to just remove all those explicit "do nothing" cases, along
> with the error default case.
>
> After doing that, I decided to write a testcase that iterates over all
> supported languages doing "ptype INTERNAL_FUNC".  That revealed that
> Pascal has a similar problem, except the default case hits a
> gdb_assert instead of an error:
>
>  (gdb) with language pascal -- ptype $_isvoid
>  type =
>  ../../src/gdb/p-typeprint.c:268: internal-error: type_print_varspec_prefix: unexpected type
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
>
> That is fixed by this patch in the same way.
>
> You'll notice that the new testcase special-cases the Ada expected
> output:
>
> 	} elseif {$lang == "ada"} {
> 	    gdb_test "ptype \$_isvoid" "<<internal function>>"
> 	} else {
> 	    gdb_test "ptype \$_isvoid" "<internal function>"
> 	}
>
> That will be subject of the following patch.
>
> Change-Id: I81aec03523cceb338b5180a0b4c2e4ad26b4c4db
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30105

LGTM.

Thanks,
Andrew

> ---
>  gdb/c-typeprint.c                             | 51 -------------------
>  gdb/p-typeprint.c                             | 46 -----------------
>  .../gdb.base/internal-functions-ptype.exp     | 42 +++++++++++++++
>  gdb/testsuite/gdb.python/py-xmethods.exp      |  8 +++
>  4 files changed, 50 insertions(+), 97 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/internal-functions-ptype.exp
>
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index dca96231117..7e9d941a435 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -441,31 +441,6 @@ c_type_print_varspec_prefix (struct type *type,
>  				   stream, show, passed_a_ptr, 0,
>  				   language, flags, podata);
>        break;
> -
> -    case TYPE_CODE_UNDEF:
> -    case TYPE_CODE_STRUCT:
> -    case TYPE_CODE_UNION:
> -    case TYPE_CODE_ENUM:
> -    case TYPE_CODE_FLAGS:
> -    case TYPE_CODE_INT:
> -    case TYPE_CODE_FLT:
> -    case TYPE_CODE_VOID:
> -    case TYPE_CODE_ERROR:
> -    case TYPE_CODE_CHAR:
> -    case TYPE_CODE_BOOL:
> -    case TYPE_CODE_SET:
> -    case TYPE_CODE_RANGE:
> -    case TYPE_CODE_STRING:
> -    case TYPE_CODE_COMPLEX:
> -    case TYPE_CODE_NAMESPACE:
> -    case TYPE_CODE_DECFLOAT:
> -    case TYPE_CODE_FIXED_POINT:
> -      /* These types need no prefix.  They are listed here so that
> -	 gcc -Wall will reveal any types that haven't been handled.  */
> -      break;
> -    default:
> -      error (_("type not handled in c_type_print_varspec_prefix()"));
> -      break;
>      }
>  }
>  
> @@ -821,32 +796,6 @@ c_type_print_varspec_suffix (struct type *type,
>        c_type_print_varspec_suffix (type->target_type (), stream,
>  				   show, passed_a_ptr, 0, language, flags);
>        break;
> -
> -    case TYPE_CODE_UNDEF:
> -    case TYPE_CODE_STRUCT:
> -    case TYPE_CODE_UNION:
> -    case TYPE_CODE_FLAGS:
> -    case TYPE_CODE_ENUM:
> -    case TYPE_CODE_INT:
> -    case TYPE_CODE_FLT:
> -    case TYPE_CODE_VOID:
> -    case TYPE_CODE_ERROR:
> -    case TYPE_CODE_CHAR:
> -    case TYPE_CODE_BOOL:
> -    case TYPE_CODE_SET:
> -    case TYPE_CODE_RANGE:
> -    case TYPE_CODE_STRING:
> -    case TYPE_CODE_COMPLEX:
> -    case TYPE_CODE_NAMESPACE:
> -    case TYPE_CODE_DECFLOAT:
> -    case TYPE_CODE_FIXED_POINT:
> -      /* These types do not need a suffix.  They are listed so that
> -	 gcc -Wall will report types that may not have been
> -	 considered.  */
> -      break;
> -    default:
> -      error (_("type not handled in c_type_print_varspec_suffix()"));
> -      break;
>      }
>  }
>  
> diff --git a/gdb/p-typeprint.c b/gdb/p-typeprint.c
> index e8542d6845a..7458aa6c095 100644
> --- a/gdb/p-typeprint.c
> +++ b/gdb/p-typeprint.c
> @@ -244,29 +244,6 @@ pascal_language::type_print_varspec_prefix (struct type *type,
>  		    plongest (type->bounds ()->high.const_val ()));
>        gdb_printf (stream, "of ");
>        break;
> -
> -    case TYPE_CODE_UNDEF:
> -    case TYPE_CODE_STRUCT:
> -    case TYPE_CODE_UNION:
> -    case TYPE_CODE_ENUM:
> -    case TYPE_CODE_INT:
> -    case TYPE_CODE_FLT:
> -    case TYPE_CODE_VOID:
> -    case TYPE_CODE_ERROR:
> -    case TYPE_CODE_CHAR:
> -    case TYPE_CODE_BOOL:
> -    case TYPE_CODE_SET:
> -    case TYPE_CODE_RANGE:
> -    case TYPE_CODE_STRING:
> -    case TYPE_CODE_COMPLEX:
> -    case TYPE_CODE_TYPEDEF:
> -    case TYPE_CODE_FIXED_POINT:
> -      /* These types need no prefix.  They are listed here so that
> -	 gcc -Wall will reveal any types that haven't been handled.  */
> -      break;
> -    default:
> -      gdb_assert_not_reached ("unexpected type");
> -      break;
>      }
>  }
>  
> @@ -377,29 +354,6 @@ pascal_language::type_print_varspec_suffix (struct type *type,
>        type_print_func_varspec_suffix (type, stream, show,
>  					     passed_a_ptr, 0, flags);
>        break;
> -
> -    case TYPE_CODE_UNDEF:
> -    case TYPE_CODE_STRUCT:
> -    case TYPE_CODE_UNION:
> -    case TYPE_CODE_ENUM:
> -    case TYPE_CODE_INT:
> -    case TYPE_CODE_FLT:
> -    case TYPE_CODE_VOID:
> -    case TYPE_CODE_ERROR:
> -    case TYPE_CODE_CHAR:
> -    case TYPE_CODE_BOOL:
> -    case TYPE_CODE_SET:
> -    case TYPE_CODE_RANGE:
> -    case TYPE_CODE_STRING:
> -    case TYPE_CODE_COMPLEX:
> -    case TYPE_CODE_TYPEDEF:
> -    case TYPE_CODE_FIXED_POINT:
> -      /* These types do not need a suffix.  They are listed so that
> -	 gcc -Wall will report types that may not have been considered.  */
> -      break;
> -    default:
> -      gdb_assert_not_reached ("unexpected type");
> -      break;
>      }
>  }
>  
> diff --git a/gdb/testsuite/gdb.base/internal-functions-ptype.exp b/gdb/testsuite/gdb.base/internal-functions-ptype.exp
> new file mode 100644
> index 00000000000..42caae05aad
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/internal-functions-ptype.exp
> @@ -0,0 +1,42 @@
> +# Copyright 2023 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 "ptype INTERNAL_FUNCTION" in all languages.
> +
> +proc test_ptype_internal_function {} {
> +    set all_languages [get_set_option_choices "set language"]
> +
> +    foreach_with_prefix lang $all_languages {
> +	if { $lang == "auto" || $lang == "local" } {
> +	    # Avoid duplicate testing.
> +	    continue
> +	}
> +
> +	gdb_test_no_output "set language $lang"
> +
> +	if {$lang == "unknown"} {
> +	    gdb_test "ptype \$_isvoid" \
> +		"expression parsing not implemented for language \"Unknown\""
> +	} elseif {$lang == "ada"} {
> +	    gdb_test "ptype \$_isvoid" "<<internal function>>"
> +	} else {
> +	    gdb_test "ptype \$_isvoid" "<internal function>"
> +	}
> +    }
> +}
> +
> +clean_restart
> +
> +test_ptype_internal_function
> diff --git a/gdb/testsuite/gdb.python/py-xmethods.exp b/gdb/testsuite/gdb.python/py-xmethods.exp
> index 97d560476fc..2cf7bbb68b0 100644
> --- a/gdb/testsuite/gdb.python/py-xmethods.exp
> +++ b/gdb/testsuite/gdb.python/py-xmethods.exp
> @@ -52,6 +52,9 @@ gdb_test "p a_geta" ".* = 1" "before: a_geta 1"
>  gdb_test "p ++a1" "No symbol.*" "before: ++a1"
>  gdb_test "p a1.getarrayind(5)" "Couldn't find method.*" \
>    "before: a1.getarrayind(5)"
> +gdb_test "ptype a1.getarrayind" \
> +    "There is no member or method named getarrayind\\." \
> +    "before: ptype a1.getarrayind"
>  
>  gdb_test "p a_ptr->geta()" ".* = 60" "before: a_ptr->geta()"
>  gdb_test "p b_geta" ".* = 1" "before: b_geta 1"
> @@ -94,9 +97,14 @@ gdb_test "p b1 - a1" ".* = 25" "after: b1 - a1"
>  gdb_test "p a_minus_a" ".* = 4" "after: a_minus_a 4"
>  
>  gdb_test "p a1.geta()" "From Python <A_geta>.*5" "after: a1.geta()"
> +
>  gdb_test "p ++a1" "From Python <plus_plus_A>.*6" "after: ++a1"
>  gdb_test "p a1.getarrayind(5)" "From Python <A_getarrayind>.*5" \
>    "after: a1.getarrayind(5)"
> +gdb_test "ptype a1.getarrayind" \
> +  "There is no member or method named getarrayind\\." \
> +  "after: ptype a1.getarrayind"
> +
>  gdb_test "p a1\[6\]" ".*int &.*6" "after a1\[\]"
>  gdb_test "p b1\[7\]" ".*const int &.*7" "after b1\[\]"
>  # Note the following test.  Xmethods on dynamc types are not looked up
> -- 
> 2.36.0
  

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> That comment quoted above says that gcc -Wall will reveal any types
Pedro> that haven't been handled, but that's not actually true, at least with
Pedro> modern GCCs.  You would need to enable -Wswitch-enum for that, which
Pedro> we don't.

We could do it for selected switches using a pragma, if we wanted.
(What would be cool is an attribute to apply to the switch so we didn't
have to wrap the whole thing.)

I tried enabling it globally but it has a lot of noise, and adding
"default" all over defeats the purpose of the flag.

Tom
  

On 2023-02-14 3:26 p.m., Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> That comment quoted above says that gcc -Wall will reveal any types
> Pedro> that haven't been handled, but that's not actually true, at least with
> Pedro> modern GCCs.  You would need to enable -Wswitch-enum for that, which
> Pedro> we don't.
> 
> We could do it for selected switches using a pragma, if we wanted.

Yeah, that is actually what I had done locally, to check which enums we
weren't handling, with the patch below.

> (What would be cool is an attribute to apply to the switch so we didn't
> have to wrap the whole thing.)

Yeah!  I have the vague impression of once seeing that clang has something
like that, but I may be wrong.

> 
> I tried enabling it globally but it has a lot of noise, and adding
> "default" all over defeats the purpose of the flag.

*nod*

From 715b0e7d83f02fe2e6356ea297d81dd6e3105858 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 9 Feb 2023 21:26:50 +0000
Subject: [PATCH] -Wswitch-enum

Change-Id: I09d8bab56abe241e21564812e910452a68752b1e
---
 gdb/c-typeprint.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index dca96231117..3966961f149 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -372,6 +372,9 @@ c_type_print_varspec_prefix (struct type *type,
 
   QUIT;
 
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_ERROR ("-Wswitch-enum")
+
   switch (type->code ())
     {
     case TYPE_CODE_PTR:
@@ -467,6 +470,8 @@ c_type_print_varspec_prefix (struct type *type,
       error (_("type not handled in c_type_print_varspec_prefix()"));
       break;
     }
+
+DIAGNOSTIC_POP
 }
 
 /* Print out "const" and "volatile" attributes,
@@ -763,6 +768,9 @@ c_type_print_varspec_suffix (struct type *type,
 
   QUIT;
 
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_ERROR ("-Wswitch-enum")
+
   switch (type->code ())
     {
     case TYPE_CODE_ARRAY:
@@ -848,6 +856,9 @@ c_type_print_varspec_suffix (struct type *type,
       error (_("type not handled in c_type_print_varspec_suffix()"));
       break;
     }
+
+DIAGNOSTIC_POP
+
 }
 
 /* A helper for c_type_print_base that displays template

base-commit: b885aea1bb987435929b4298982ac6fc27f69403
prerequisite-patch-id: 8137bd248a83bbcb2cacfb0250eef1994eafaeac
prerequisite-patch-id: 78b3c4f199e7af4170c5fc713bd88fd4ac62dc36
  

>> (What would be cool is an attribute to apply to the switch so we didn't
>> have to wrap the whole thing.)

Pedro> Yeah!  I have the vague impression of once seeing that clang has something
Pedro> like that, but I may be wrong.

I filed a GCC bug about it.

Tom