[2/6] Make "ptype INTERNAL_FUNCTION" in Ada print like other languages

  Currently, printing the type of an internal function in Ada shows
double <>s, like:

 (gdb) with language ada -- ptype $_isvoid
 type = <<internal function>>

while all other languages print it with a single <>, like:

 (gdb) with language c -- ptype $_isvoid
 type = <internal function>

I don't think there's a reason that Ada needs to be different.  We
currently print the double <>s because we take this path in
ada_print_type:

    switch (type->code ())
      {
      default:
	gdb_printf (stream, "<");
	c_print_type (type, "", stream, show, level, language_ada, flags);
	gdb_printf (stream, ">");
	break;

... and the type's name already has the <>s.

Fix this by simply adding an early check for
TYPE_CODE_INTERNAL_FUNCTION.

Change-Id: Ic2b6527b9240a367471431023f6e27e6daed5501
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30105
---
 gdb/ada-typeprint.c                                 | 7 +++++++
 gdb/testsuite/gdb.base/internal-functions-ptype.exp | 2 --
 2 files changed, 7 insertions(+), 2 deletions(-)
  

Pedro Alves <pedro@palves.net> writes:

> Currently, printing the type of an internal function in Ada shows
> double <>s, like:
>
>  (gdb) with language ada -- ptype $_isvoid
>  type = <<internal function>>
>
> while all other languages print it with a single <>, like:
>
>  (gdb) with language c -- ptype $_isvoid
>  type = <internal function>
>
> I don't think there's a reason that Ada needs to be different.  We
> currently print the double <>s because we take this path in
> ada_print_type:
>
>     switch (type->code ())
>       {
>       default:
> 	gdb_printf (stream, "<");
> 	c_print_type (type, "", stream, show, level, language_ada, flags);
> 	gdb_printf (stream, ">");
> 	break;
>
> ... and the type's name already has the <>s.
>
> Fix this by simply adding an early check for
> TYPE_CODE_INTERNAL_FUNCTION.

I confess, this is not the solution I though you'd go with.  I was
expecting you to handle TYPE_CODE_INTERNAL_FUNCTION in the switch, just
to leave things consistent.

I guess it doesn't hurt though, so LGTM.

Thanks,
Andrew

>
> Change-Id: Ic2b6527b9240a367471431023f6e27e6daed5501
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30105
> ---
>  gdb/ada-typeprint.c                                 | 7 +++++++
>  gdb/testsuite/gdb.base/internal-functions-ptype.exp | 2 --
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/ada-typeprint.c b/gdb/ada-typeprint.c
> index e95034c9285..3866b2d35eb 100644
> --- a/gdb/ada-typeprint.c
> +++ b/gdb/ada-typeprint.c
> @@ -941,6 +941,13 @@ ada_print_type (struct type *type0, const char *varstring,
>  		struct ui_file *stream, int show, int level,
>  		const struct type_print_options *flags)
>  {
> +  if (type0->code () == TYPE_CODE_INTERNAL_FUNCTION)
> +    {
> +      c_print_type (type0, "", stream, show, level,
> +		    language_ada, flags);
> +      return;
> +    }
> +
>    struct type *type = ada_check_typedef (ada_get_base_type (type0));
>    /* If we can decode the original type name, use it.  However, there
>       are cases where the original type is an internally-generated type
> diff --git a/gdb/testsuite/gdb.base/internal-functions-ptype.exp b/gdb/testsuite/gdb.base/internal-functions-ptype.exp
> index 42caae05aad..748f33a87cd 100644
> --- a/gdb/testsuite/gdb.base/internal-functions-ptype.exp
> +++ b/gdb/testsuite/gdb.base/internal-functions-ptype.exp
> @@ -29,8 +29,6 @@ proc test_ptype_internal_function {} {
>  	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>"
>  	}
> -- 
> 2.36.0
  

>> I don't think there's a reason that Ada needs to be different.

Agreed.

>> Fix this by simply adding an early check for
>> TYPE_CODE_INTERNAL_FUNCTION.

Andrew> I confess, this is not the solution I though you'd go with.  I was
Andrew> expecting you to handle TYPE_CODE_INTERNAL_FUNCTION in the switch, just
Andrew> to leave things consistent.

I think this would be better.

Tom
  

On 2023-02-14 3:30 p.m., Tom Tromey wrote:

>>> Fix this by simply adding an early check for
>>> TYPE_CODE_INTERNAL_FUNCTION.
> 
> Andrew> I confess, this is not the solution I though you'd go with.  I was
> Andrew> expecting you to handle TYPE_CODE_INTERNAL_FUNCTION in the switch, just
> Andrew> to leave things consistent.
> 
> I think this would be better.

My point with adding this check early is that these functions' type never
has anything to do with Ada, so all that code at the beginning of ada_print_type,
like decoded_type_name, ada_is_aligner_type, ada_is_constrained_packed_array_type,
etc. is always a nop for internal functions, so might as well skip it all.

I actually started out by considering moving TYPE_CODE_INTERNAL_FUNCTION printing
to common code (say, rename the virtual language_defn::print_type to do_print_type,
and add a new non-virtual wrapper language_defn::print_type method and print
TYPE_CODE_INTERNAL_FUNCTION there), and I didn't pursue that as I couldn't really convince
myself that a different language might want to print it differently (say, some other
characters instead of "<>"), and I guess that's why I ended up with putting it at the start
of the function, as that is the closest to putting it at the caller instead.
  

On 2023-02-15 1:38 p.m., Pedro Alves wrote:
> On 2023-02-14 3:30 p.m., Tom Tromey wrote:
> 
>>>> Fix this by simply adding an early check for
>>>> TYPE_CODE_INTERNAL_FUNCTION.
>>
>> Andrew> I confess, this is not the solution I though you'd go with.  I was
>> Andrew> expecting you to handle TYPE_CODE_INTERNAL_FUNCTION in the switch, just
>> Andrew> to leave things consistent.
>>
>> I think this would be better.
> 
> My point with adding this check early is that these functions' type never
> has anything to do with Ada, so all that code at the beginning of ada_print_type,
> like decoded_type_name, ada_is_aligner_type, ada_is_constrained_packed_array_type,
> etc. is always a nop for internal functions, so might as well skip it all.
> 
> I actually started out by considering moving TYPE_CODE_INTERNAL_FUNCTION printing
> to common code (say, rename the virtual language_defn::print_type to do_print_type,
> and add a new non-virtual wrapper language_defn::print_type method and print
> TYPE_CODE_INTERNAL_FUNCTION there), and I didn't pursue that as I couldn't really convince
> myself that a different language might want to print it differently (say, some other
> characters instead of "<>"), and I guess that's why I ended up with putting it at the start
> of the function, as that is the closest to putting it at the caller instead.
> 

Here's the alternative patch handling TYPE_CODE_INTERNAL_FUNCTION in the switch.

WDYT?

From: Pedro Alves <pedro@palves.net>
Subject: [PATCH 2/6] Make "ptype INTERNAL_FUNCTION" in Ada print like other
 languages

Currently, printing the type of an internal function in Ada shows
double <>s, like:

 (gdb) with language ada -- ptype $_isvoid
 type = <<internal function>>

while all other languages print it with a single <>, like:

 (gdb) with language c -- ptype $_isvoid
 type = <internal function>

I don't think there's a reason that Ada needs to be different.  We
currently print the double <>s because we take this path in
ada_print_type:

    switch (type->code ())
      {
      default:
	gdb_printf (stream, "<");
	c_print_type (type, "", stream, show, level, language_ada, flags);
	gdb_printf (stream, ">");
	break;

... and the type's name already has the <>s.

Fix this by making ada_print_type handle TYPE_CODE_INTERNAL_FUNCTION.

Change-Id: Ic2b6527b9240a367471431023f6e27e6daed5501
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30105
---
 gdb/ada-typeprint.c                                 | 3 +++
 gdb/testsuite/gdb.base/internal-functions-ptype.exp | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/ada-typeprint.c b/gdb/ada-typeprint.c
index e95034c9285..e094bc4c9f4 100644
--- a/gdb/ada-typeprint.c
+++ b/gdb/ada-typeprint.c
@@ -991,6 +991,9 @@ ada_print_type (struct type *type0, const char *varstring,
 	c_print_type (type, "", stream, show, level, language_ada, flags);
 	gdb_printf (stream, ">");
 	break;
+      case TYPE_CODE_INTERNAL_FUNCTION:
+	c_print_type (type, "", stream, show, level, language_ada, flags);
+	break;
       case TYPE_CODE_PTR:
       case TYPE_CODE_TYPEDEF:
 	/* An __XVL field is not truly a pointer, so don't print
diff --git a/gdb/testsuite/gdb.base/internal-functions-ptype.exp b/gdb/testsuite/gdb.base/internal-functions-ptype.exp
index 42caae05aad..748f33a87cd 100644
--- a/gdb/testsuite/gdb.base/internal-functions-ptype.exp
+++ b/gdb/testsuite/gdb.base/internal-functions-ptype.exp
@@ -29,8 +29,6 @@ proc test_ptype_internal_function {} {
 	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>"
 	}

base-commit: 5036bde964bc1a18282dde536a95aecd0d2c08fb
  

>> I think this would be better.

Pedro> My point with adding this check early is that these functions' type never
Pedro> has anything to do with Ada, so all that code at the beginning of ada_print_type,
Pedro> like decoded_type_name, ada_is_aligner_type, ada_is_constrained_packed_array_type,
Pedro> etc. is always a nop for internal functions, so might as well skip it all.

I see.  That makes sense to me, thanks.

Sorry about the noise, as far as I'm concerned you can land either
version of the patch.

Pedro> I actually started out by considering moving TYPE_CODE_INTERNAL_FUNCTION printing
Pedro> to common code (say, rename the virtual language_defn::print_type to do_print_type,
Pedro> and add a new non-virtual wrapper language_defn::print_type method and print
Pedro> TYPE_CODE_INTERNAL_FUNCTION there), and I didn't pursue that as I couldn't really convince
Pedro> myself that a different language might want to print it differently (say, some other
Pedro> characters instead of "<>")

I think this is something we can simply force on languages.  The type of
these things isn't a language feature and probably isn't worth
customizing anyway.

More generally I think it's better for gdb to try to do more in the
common code and leave less to the languages.

thanks,
Tom