[PATCH] Correct -fdump-go-spec's handling of incomplete types

[PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

This patch corrects -fdump-go-spec's handling of incomplete types.
To my knowledge the issue fixed here has not been previously
reported. It was exposed by an in-progress port of gccgo to FreeBSD.

Given the following C code

	struct s_fwd v_fwd;
	struct s_fwd { };

-fdump-go-spec currently produces the following Go code

	var v_fwd struct {};
	type s_fwd s_fwd;

whereas the correct Go code is:

	var v_fwd s_fwd;
	type s_fwd struct {};

(Go is considerably more permissive than C with out-of-order
declarations, so anywhere an out-of-order declaration is valid in
C it is valid in Go.)

gcc/:
	* godump.c (go_format_type): Don't consider whether a type has
	been seen when determining whether to output a type by name.
	Consider only the use_type_name parameter.
	(go_output_typedef): When outputting a typedef, format the
	declaration's original type, which contains the name of the
	underlying type rather than the name of the typedef.
gcc/testsuite:
	* gcc.misc-tests/godump-1.c: Add test case.

diff --git a/gcc/godump.c b/gcc/godump.c
index 29a45ce8979..b457965bdc8 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -697,9 +697,8 @@ go_format_type (class godump_container *container, tree type,
    ret = true;
    ob = &container->type_obstack;
  
-  if (TYPE_NAME (type) != NULL_TREE
-      && (container->decls_seen.contains (type)
-	  || container->decls_seen.contains (TYPE_NAME (type)))
+  if (use_type_name
+      && TYPE_NAME (type) != NULL_TREE
        && (AGGREGATE_TYPE_P (type)
  	  || POINTER_TYPE_P (type)
  	  || TREE_CODE (type) == FUNCTION_TYPE))
@@ -707,6 +706,12 @@ go_format_type (class godump_container *container, tree type,
        tree name;
        void **slot;
  
+      /* References to complex builtin types cannot be translated to
+	 Go.  */
+      if (DECL_P (TYPE_NAME (type))
+	  && DECL_IS_UNDECLARED_BUILTIN (TYPE_NAME (type)))
+	ret = false;
+
        name = TYPE_IDENTIFIER (type);
  
        slot = htab_find_slot (container->invalid_hash, IDENTIFIER_POINTER (name),
@@ -714,13 +719,17 @@ go_format_type (class godump_container *container, tree type,
        if (slot != NULL)
  	ret = false;
  
+      /* References to incomplete structs are permitted in many
+	 contexts, like behind a pointer or inside of a typedef. So
+	 consider any referenced struct a potential dummy type.  */
+      if (RECORD_OR_UNION_TYPE_P (type))
+	container->pot_dummy_types.add (IDENTIFIER_POINTER (name));
+
        obstack_1grow (ob, '_');
        go_append_string (ob, name);
        return ret;
      }
  
-  container->decls_seen.add (type);
-
    switch (TREE_CODE (type))
      {
      case TYPE_DECL:
@@ -821,34 +830,6 @@ go_format_type (class godump_container *container, tree type,
        break;
  
      case POINTER_TYPE:
-      if (use_type_name
-          && TYPE_NAME (TREE_TYPE (type)) != NULL_TREE
-          && (RECORD_OR_UNION_TYPE_P (TREE_TYPE (type))
-	      || (POINTER_TYPE_P (TREE_TYPE (type))
-                  && (TREE_CODE (TREE_TYPE (TREE_TYPE (type)))
-		      == FUNCTION_TYPE))))
-        {
-	  tree name;
-	  void **slot;
-
-	  name = TYPE_IDENTIFIER (TREE_TYPE (type));
-
-	  slot = htab_find_slot (container->invalid_hash,
-				 IDENTIFIER_POINTER (name), NO_INSERT);
-	  if (slot != NULL)
-	    ret = false;
-
-	  obstack_grow (ob, "*_", 2);
-	  go_append_string (ob, name);
-
-	  /* The pointer here can be used without the struct or union
-	     definition.  So this struct or union is a potential dummy
-	     type.  */
-	  if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (type)))
-	    container->pot_dummy_types.add (IDENTIFIER_POINTER (name));
-
-	  return ret;
-        }
        if (TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE)
  	obstack_grow (ob, "func", 4);
        else
@@ -1182,8 +1163,8 @@ go_output_typedef (class godump_container *container, tree decl)
  	return;
        *slot = CONST_CAST (void *, (const void *) type);
  
-      if (!go_format_type (container, TREE_TYPE (decl), true, false, NULL,
-			   false))
+      if (!go_format_type (container, DECL_ORIGINAL_TYPE (decl), true, false,
+			   NULL, false))
  	{
  	  fprintf (go_dump_file, "// ");
  	  slot = htab_find_slot (container->invalid_hash, type, INSERT);
diff --git a/gcc/testsuite/gcc.misc-tests/godump-1.c b/gcc/testsuite/gcc.misc-tests/godump-1.c
index f97bbecc9bc..96c25863374 100644
--- a/gcc/testsuite/gcc.misc-tests/godump-1.c
+++ b/gcc/testsuite/gcc.misc-tests/godump-1.c
@@ -471,6 +471,9 @@ typedef struct s_undef_t s_undef_t2;
  typedef struct s_fwd *s_fwd_p;
  /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd_p \\*_s_fwd$" } } */
  
+struct s_fwd v_fwd;
+/* { dg-final { scan-file godump-1.out "(?n)^var _v_fwd _s_fwd" } } */
+
  struct s_fwd { };
  /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd struct \{ \}$" } } */
  

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On Tue, Dec 8, 2020 at 2:57 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>
> This patch corrects -fdump-go-spec's handling of incomplete types.
> To my knowledge the issue fixed here has not been previously
> reported. It was exposed by an in-progress port of gccgo to FreeBSD.
>
> Given the following C code
>
>         struct s_fwd v_fwd;
>         struct s_fwd { };
>
> -fdump-go-spec currently produces the following Go code
>
>         var v_fwd struct {};
>         type s_fwd s_fwd;
>
> whereas the correct Go code is:
>
>         var v_fwd s_fwd;
>         type s_fwd struct {};
>
> (Go is considerably more permissive than C with out-of-order
> declarations, so anywhere an out-of-order declaration is valid in
> C it is valid in Go.)
>
> gcc/:
>         * godump.c (go_format_type): Don't consider whether a type has
>         been seen when determining whether to output a type by name.
>         Consider only the use_type_name parameter.
>         (go_output_typedef): When outputting a typedef, format the
>         declaration's original type, which contains the name of the
>         underlying type rather than the name of the typedef.
> gcc/testsuite:
>         * gcc.misc-tests/godump-1.c: Add test case.

Thanks.  I changed function types to use type names, and committed like so.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4228 bytes --]

73cf5da233b4cd0f140dd997270e88de63e27db7
diff --git a/gcc/godump.c b/gcc/godump.c
index 29a45ce8979..033b2c59f3c 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -697,9 +697,8 @@ go_format_type (class godump_container *container, tree type,
   ret = true;
   ob = &container->type_obstack;
 
-  if (TYPE_NAME (type) != NULL_TREE
-      && (container->decls_seen.contains (type)
-	  || container->decls_seen.contains (TYPE_NAME (type)))
+  if (use_type_name
+      && TYPE_NAME (type) != NULL_TREE
       && (AGGREGATE_TYPE_P (type)
 	  || POINTER_TYPE_P (type)
 	  || TREE_CODE (type) == FUNCTION_TYPE))
@@ -707,6 +706,12 @@ go_format_type (class godump_container *container, tree type,
       tree name;
       void **slot;
 
+      /* References to complex builtin types cannot be translated to
+	Go.  */
+      if (DECL_P (TYPE_NAME (type))
+	  && DECL_IS_UNDECLARED_BUILTIN (TYPE_NAME (type)))
+	ret = false;
+
       name = TYPE_IDENTIFIER (type);
 
       slot = htab_find_slot (container->invalid_hash, IDENTIFIER_POINTER (name),
@@ -714,13 +719,17 @@ go_format_type (class godump_container *container, tree type,
       if (slot != NULL)
 	ret = false;
 
+      /* References to incomplete structs are permitted in many
+	 contexts, like behind a pointer or inside of a typedef. So
+	 consider any referenced struct a potential dummy type.  */
+      if (RECORD_OR_UNION_TYPE_P (type))
+       container->pot_dummy_types.add (IDENTIFIER_POINTER (name));
+
       obstack_1grow (ob, '_');
       go_append_string (ob, name);
       return ret;
     }
 
-  container->decls_seen.add (type);
-
   switch (TREE_CODE (type))
     {
     case TYPE_DECL:
@@ -821,34 +830,6 @@ go_format_type (class godump_container *container, tree type,
       break;
 
     case POINTER_TYPE:
-      if (use_type_name
-          && TYPE_NAME (TREE_TYPE (type)) != NULL_TREE
-          && (RECORD_OR_UNION_TYPE_P (TREE_TYPE (type))
-	      || (POINTER_TYPE_P (TREE_TYPE (type))
-                  && (TREE_CODE (TREE_TYPE (TREE_TYPE (type)))
-		      == FUNCTION_TYPE))))
-        {
-	  tree name;
-	  void **slot;
-
-	  name = TYPE_IDENTIFIER (TREE_TYPE (type));
-
-	  slot = htab_find_slot (container->invalid_hash,
-				 IDENTIFIER_POINTER (name), NO_INSERT);
-	  if (slot != NULL)
-	    ret = false;
-
-	  obstack_grow (ob, "*_", 2);
-	  go_append_string (ob, name);
-
-	  /* The pointer here can be used without the struct or union
-	     definition.  So this struct or union is a potential dummy
-	     type.  */
-	  if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (type)))
-	    container->pot_dummy_types.add (IDENTIFIER_POINTER (name));
-
-	  return ret;
-        }
       if (TREE_CODE (TREE_TYPE (type)) == FUNCTION_TYPE)
 	obstack_grow (ob, "func", 4);
       else
@@ -1107,7 +1088,7 @@ go_output_type (class godump_container *container)
 static void
 go_output_fndecl (class godump_container *container, tree decl)
 {
-  if (!go_format_type (container, TREE_TYPE (decl), false, true, NULL, false))
+  if (!go_format_type (container, TREE_TYPE (decl), true, true, NULL, false))
     fprintf (go_dump_file, "// ");
   fprintf (go_dump_file, "func _%s ",
 	   IDENTIFIER_POINTER (DECL_NAME (decl)));
@@ -1182,8 +1163,8 @@ go_output_typedef (class godump_container *container, tree decl)
 	return;
       *slot = CONST_CAST (void *, (const void *) type);
 
-      if (!go_format_type (container, TREE_TYPE (decl), true, false, NULL,
-			   false))
+      if (!go_format_type (container, DECL_ORIGINAL_TYPE (decl), true, false,
+			   NULL, false))
 	{
 	  fprintf (go_dump_file, "// ");
 	  slot = htab_find_slot (container->invalid_hash, type, INSERT);
diff --git a/gcc/testsuite/gcc.misc-tests/godump-1.c b/gcc/testsuite/gcc.misc-tests/godump-1.c
index f97bbecc9bc..96c25863374 100644
--- a/gcc/testsuite/gcc.misc-tests/godump-1.c
+++ b/gcc/testsuite/gcc.misc-tests/godump-1.c
@@ -471,6 +471,9 @@ typedef struct s_undef_t s_undef_t2;
 typedef struct s_fwd *s_fwd_p;
 /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd_p \\*_s_fwd$" } } */
 
+struct s_fwd v_fwd;
+/* { dg-final { scan-file godump-1.out "(?n)^var _v_fwd _s_fwd" } } */
+
 struct s_fwd { };
 /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd struct \{ \}$" } } */
 

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Rainer Orth]

Hi Ian,

> On Tue, Dec 8, 2020 at 2:57 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>>
>> This patch corrects -fdump-go-spec's handling of incomplete types.
>> To my knowledge the issue fixed here has not been previously
>> reported. It was exposed by an in-progress port of gccgo to FreeBSD.
>>
>> Given the following C code
>>
>>         struct s_fwd v_fwd;
>>         struct s_fwd { };
>>
>> -fdump-go-spec currently produces the following Go code
>>
>>         var v_fwd struct {};
>>         type s_fwd s_fwd;
>>
>> whereas the correct Go code is:
>>
>>         var v_fwd s_fwd;
>>         type s_fwd struct {};
>>
>> (Go is considerably more permissive than C with out-of-order
>> declarations, so anywhere an out-of-order declaration is valid in
>> C it is valid in Go.)
>>
>> gcc/:
>>         * godump.c (go_format_type): Don't consider whether a type has
>>         been seen when determining whether to output a type by name.
>>         Consider only the use_type_name parameter.
>>         (go_output_typedef): When outputting a typedef, format the
>>         declaration's original type, which contains the name of the
>>         underlying type rather than the name of the typedef.
>> gcc/testsuite:
>>         * gcc.misc-tests/godump-1.c: Add test case.
>
> Thanks.  I changed function types to use type names, and committed like so.

This patch badly broke Solaris bootstrap:

runtime_sysinfo.go:623:6: error: invalid recursive type
  623 | type ___FILE ___FILE
      |      ^
runtime_sysinfo.go:7045:6: error: redefinition of ‘_mld_hdr_t’
 7045 | type _mld_hdr_t _mld_hdr
      |      ^
runtime_sysinfo.go:1510:6: note: previous definition of ‘_mld_hdr_t’ was here
 1510 | type _mld_hdr_t _mld_hdr
      |      ^
runtime_sysinfo.go:7070:6: error: redefinition of ‘_upad128_t’
 7070 | type _upad128_t struct { _l [4]uint32; }
      |      ^
runtime_sysinfo.go:7029:6: note: previous definition of ‘_upad128_t’ was here
 7029 | type _upad128_t struct {}
      |      ^
runtime_sysinfo.go:7071:6: error: redefinition of ‘_zone_net_addr_t’
 7071 | type _zone_net_addr_t _zone_net_addr
      |      ^
runtime_sysinfo.go:1079:6: note: previous definition of ‘_zone_net_addr_t’ was here
 1079 | type _zone_net_addr_t _zone_net_addr
      |      ^
runtime_sysinfo.go:7072:6: error: redefinition of ‘_flow_arp_desc_t’
 7072 | type _flow_arp_desc_t _flow_arp_desc_s
      |      ^
runtime_sysinfo.go:1127:6: note: previous definition of ‘_flow_arp_desc_t’ was here
 1127 | type _flow_arp_desc_t _flow_arp_desc_s
      |      ^
runtime_sysinfo.go:7073:6: error: redefinition of ‘_flow_l3_desc_t’
 7073 | type _flow_l3_desc_t _flow_l3_desc_s
      |      ^
runtime_sysinfo.go:1130:6: note: previous definition of ‘_flow_l3_desc_t’ was here
 1130 | type _flow_l3_desc_t _flow_l3_desc_s
      |      ^
runtime_sysinfo.go:7074:6: error: redefinition of ‘_mac_ipaddr_t’
 7074 | type _mac_ipaddr_t _mac_ipaddr_s
      |      ^
runtime_sysinfo.go:1150:6: note: previous definition of ‘_mac_ipaddr_t’ was here
 1150 | type _mac_ipaddr_t _mac_ipaddr_s
      |      ^
runtime_sysinfo.go:7075:6: error: redefinition of ‘_mactun_info_t’
 7075 | type _mactun_info_t _mactun_info_s
      |      ^
runtime_sysinfo.go:1213:6: note: previous definition of ‘_mactun_info_t’ was here
 1213 | type _mactun_info_t _mactun_info_s
      |      ^
runtime_sysinfo.go:187:19: error: use of undefined type ‘_timespec’
  187 | type _timestruc_t _timespec
      |                   ^
runtime_sysinfo.go:1213:21: error: use of undefined type ‘_mactun_info_s’
 1213 | type _mactun_info_t _mactun_info_s
      |                     ^
runtime_sysinfo.go:741:13: error: use of undefined type ‘_ip6_hdr’
  741 | type _ip6_t _ip6_hdr
      |             ^
runtime_sysinfo.go:1079:23: error: use of undefined type ‘_zone_net_addr’
 1079 | type _zone_net_addr_t _zone_net_addr
      |                       ^
runtime_sysinfo.go:1127:23: error: use of undefined type ‘_flow_arp_desc_s’
 1127 | type _flow_arp_desc_t _flow_arp_desc_s
      |                       ^
runtime_sysinfo.go:1130:22: error: use of undefined type ‘_flow_l3_desc_s’
 1130 | type _flow_l3_desc_t _flow_l3_desc_s
      |                      ^
runtime_sysinfo.go:1150:20: error: use of undefined type ‘_mac_ipaddr_s’
 1150 | type _mac_ipaddr_t _mac_ipaddr_s
      |                    ^
runtime_sysinfo.go:1210:28: error: use of undefined type ‘_mac_resource_props_s’
 1210 | type _mac_resource_props_t _mac_resource_props_s
      |                            ^
runtime_sysinfo.go:1510:17: error: use of undefined type ‘_mld_hdr’
 1510 | type _mld_hdr_t _mld_hdr
      |                 ^
runtime_sysinfo.go:1519:17: error: use of undefined type ‘_mld2mar’
 1519 | type _mld2mar_t _mld2mar
      |                 ^
runtime_sysinfo.go:1535:29: error: use of undefined type ‘_nd_neighbor_solicit’
 1535 | type _nd_neighbor_solicit_t _nd_neighbor_solicit
      |                             ^
runtime_sysinfo.go:1538:28: error: use of undefined type ‘_nd_neighbor_advert’
 1538 | type _nd_neighbor_advert_t _nd_neighbor_advert
      |                            ^
runtime_sysinfo.go:1541:21: error: use of undefined type ‘_nd_redirect’
 1541 | type _nd_redirect_t _nd_redirect
      |                     ^
runtime_sysinfo.go:1548:28: error: use of undefined type ‘_nd_opt_prefix_info’
 1548 | type _nd_opt_prefix_info_t _nd_opt_prefix_info
      |                            ^
make: *** [Makefile:2959: runtime.lo] Error 1

Comparing runtime_sysinfo.go with a slightly older version in two
instances:

-type ___gnuc_va_list *int8
-type ___va_list *int8
-type ___FILE struct { _cnt int32; _ptr *uint8; _base *uint8; _flag uint8; _magic uint8; Godump_0_pad [2]byte; }
+type ___FILE ___FILE

+type _mld_hdr_t _mld_hdr

-type _mld_hdr_t struct { mld_icmp6_hdr _icmp6_hdr; mld_addr [16]byte; }
+type _mld_hdr_t _mld_hdr

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

Sorry about this, Rainer. I think I see the issue, though it's hard to
be certain without access to a Solaris machine. Assuming the icmp6.h
header hasn't changed since the last time Solaris code was open source
[0], I think the issue is likely to be typedefs that define a named
struct and an alias for that struct in one shot. I'll start pulling on
that thread.

[0]: https://github.com/kofemann/opensolaris/blob/80192cd83/usr/src/uts/common/netinet/icmp6.h#L71-L74

On Thu, Dec 10, 2020 at 2:18 PM Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote:
>
> Hi Ian,
>
> > On Tue, Dec 8, 2020 at 2:57 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
> >>
> >> This patch corrects -fdump-go-spec's handling of incomplete types.
> >> To my knowledge the issue fixed here has not been previously
> >> reported. It was exposed by an in-progress port of gccgo to FreeBSD.
> >>
> >> Given the following C code
> >>
> >>         struct s_fwd v_fwd;
> >>         struct s_fwd { };
> >>
> >> -fdump-go-spec currently produces the following Go code
> >>
> >>         var v_fwd struct {};
> >>         type s_fwd s_fwd;
> >>
> >> whereas the correct Go code is:
> >>
> >>         var v_fwd s_fwd;
> >>         type s_fwd struct {};
> >>
> >> (Go is considerably more permissive than C with out-of-order
> >> declarations, so anywhere an out-of-order declaration is valid in
> >> C it is valid in Go.)
> >>
> >> gcc/:
> >>         * godump.c (go_format_type): Don't consider whether a type has
> >>         been seen when determining whether to output a type by name.
> >>         Consider only the use_type_name parameter.
> >>         (go_output_typedef): When outputting a typedef, format the
> >>         declaration's original type, which contains the name of the
> >>         underlying type rather than the name of the typedef.
> >> gcc/testsuite:
> >>         * gcc.misc-tests/godump-1.c: Add test case.
> >
> > Thanks.  I changed function types to use type names, and committed like so.
>
> This patch badly broke Solaris bootstrap:
>
> runtime_sysinfo.go:623:6: error: invalid recursive type
>   623 | type ___FILE ___FILE
>       |      ^
> runtime_sysinfo.go:7045:6: error: redefinition of ‘_mld_hdr_t’
>  7045 | type _mld_hdr_t _mld_hdr
>       |      ^
> runtime_sysinfo.go:1510:6: note: previous definition of ‘_mld_hdr_t’ was here
>  1510 | type _mld_hdr_t _mld_hdr
>       |      ^
> runtime_sysinfo.go:7070:6: error: redefinition of ‘_upad128_t’
>  7070 | type _upad128_t struct { _l [4]uint32; }
>       |      ^
> runtime_sysinfo.go:7029:6: note: previous definition of ‘_upad128_t’ was here
>  7029 | type _upad128_t struct {}
>       |      ^
> runtime_sysinfo.go:7071:6: error: redefinition of ‘_zone_net_addr_t’
>  7071 | type _zone_net_addr_t _zone_net_addr
>       |      ^
> runtime_sysinfo.go:1079:6: note: previous definition of ‘_zone_net_addr_t’ was here
>  1079 | type _zone_net_addr_t _zone_net_addr
>       |      ^
> runtime_sysinfo.go:7072:6: error: redefinition of ‘_flow_arp_desc_t’
>  7072 | type _flow_arp_desc_t _flow_arp_desc_s
>       |      ^
> runtime_sysinfo.go:1127:6: note: previous definition of ‘_flow_arp_desc_t’ was here
>  1127 | type _flow_arp_desc_t _flow_arp_desc_s
>       |      ^
> runtime_sysinfo.go:7073:6: error: redefinition of ‘_flow_l3_desc_t’
>  7073 | type _flow_l3_desc_t _flow_l3_desc_s
>       |      ^
> runtime_sysinfo.go:1130:6: note: previous definition of ‘_flow_l3_desc_t’ was here
>  1130 | type _flow_l3_desc_t _flow_l3_desc_s
>       |      ^
> runtime_sysinfo.go:7074:6: error: redefinition of ‘_mac_ipaddr_t’
>  7074 | type _mac_ipaddr_t _mac_ipaddr_s
>       |      ^
> runtime_sysinfo.go:1150:6: note: previous definition of ‘_mac_ipaddr_t’ was here
>  1150 | type _mac_ipaddr_t _mac_ipaddr_s
>       |      ^
> runtime_sysinfo.go:7075:6: error: redefinition of ‘_mactun_info_t’
>  7075 | type _mactun_info_t _mactun_info_s
>       |      ^
> runtime_sysinfo.go:1213:6: note: previous definition of ‘_mactun_info_t’ was here
>  1213 | type _mactun_info_t _mactun_info_s
>       |      ^
> runtime_sysinfo.go:187:19: error: use of undefined type ‘_timespec’
>   187 | type _timestruc_t _timespec
>       |                   ^
> runtime_sysinfo.go:1213:21: error: use of undefined type ‘_mactun_info_s’
>  1213 | type _mactun_info_t _mactun_info_s
>       |                     ^
> runtime_sysinfo.go:741:13: error: use of undefined type ‘_ip6_hdr’
>   741 | type _ip6_t _ip6_hdr
>       |             ^
> runtime_sysinfo.go:1079:23: error: use of undefined type ‘_zone_net_addr’
>  1079 | type _zone_net_addr_t _zone_net_addr
>       |                       ^
> runtime_sysinfo.go:1127:23: error: use of undefined type ‘_flow_arp_desc_s’
>  1127 | type _flow_arp_desc_t _flow_arp_desc_s
>       |                       ^
> runtime_sysinfo.go:1130:22: error: use of undefined type ‘_flow_l3_desc_s’
>  1130 | type _flow_l3_desc_t _flow_l3_desc_s
>       |                      ^
> runtime_sysinfo.go:1150:20: error: use of undefined type ‘_mac_ipaddr_s’
>  1150 | type _mac_ipaddr_t _mac_ipaddr_s
>       |                    ^
> runtime_sysinfo.go:1210:28: error: use of undefined type ‘_mac_resource_props_s’
>  1210 | type _mac_resource_props_t _mac_resource_props_s
>       |                            ^
> runtime_sysinfo.go:1510:17: error: use of undefined type ‘_mld_hdr’
>  1510 | type _mld_hdr_t _mld_hdr
>       |                 ^
> runtime_sysinfo.go:1519:17: error: use of undefined type ‘_mld2mar’
>  1519 | type _mld2mar_t _mld2mar
>       |                 ^
> runtime_sysinfo.go:1535:29: error: use of undefined type ‘_nd_neighbor_solicit’
>  1535 | type _nd_neighbor_solicit_t _nd_neighbor_solicit
>       |                             ^
> runtime_sysinfo.go:1538:28: error: use of undefined type ‘_nd_neighbor_advert’
>  1538 | type _nd_neighbor_advert_t _nd_neighbor_advert
>       |                            ^
> runtime_sysinfo.go:1541:21: error: use of undefined type ‘_nd_redirect’
>  1541 | type _nd_redirect_t _nd_redirect
>       |                     ^
> runtime_sysinfo.go:1548:28: error: use of undefined type ‘_nd_opt_prefix_info’
>  1548 | type _nd_opt_prefix_info_t _nd_opt_prefix_info
>       |                            ^
> make: *** [Makefile:2959: runtime.lo] Error 1
>
> Comparing runtime_sysinfo.go with a slightly older version in two
> instances:
>
> -type ___gnuc_va_list *int8
> -type ___va_list *int8
> -type ___FILE struct { _cnt int32; _ptr *uint8; _base *uint8; _flag uint8; _magic uint8; Godump_0_pad [2]byte; }
> +type ___FILE ___FILE
>
> +type _mld_hdr_t _mld_hdr
>
> -type _mld_hdr_t struct { mld_icmp6_hdr _icmp6_hdr; mld_addr [16]byte; }
> +type _mld_hdr_t _mld_hdr
>
>         Rainer
>
> --
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Rainer Orth]

Hi Nikhil,

> Sorry about this, Rainer. I think I see the issue, though it's hard to
> be certain without access to a Solaris machine. Assuming the icmp6.h
> header hasn't changed since the last time Solaris code was open source
> [0], I think the issue is likely to be typedefs that define a named
> struct and an alias for that struct in one shot. I'll start pulling on
> that thread.
>
> [0]:
> https://github.com/kofemann/opensolaris/blob/80192cd83/usr/src/uts/common/netinet/icmp6.h#L71-L74

I've just checked: <netinet/icmp6.h> is effectively unchanged since
Solaris 10.

Besides, there's gcc211 in the GCC compile farm, running Solaris 11.3/SPARC.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

On 12/10/20 2:34 PM, Rainer Orth wrote:
> I've just checked: <netinet/icmp6.h> is effectively unchanged since
> Solaris 10.
> 
> Besides, there's gcc211 in the GCC compile farm, running Solaris 11.3/SPARC.

Ah, thanks, I wasn't aware there was a compile farm available to GCC
developers. I've applied for an account, but it sounds like it may take
a while to get approved.

My theory was wrong, by the way. This C snippet, representative of the
Solaris headers, expands just fine:

struct in6_addr {
	union {
		uint8_t		__u6_addr8[16];
		uint16_t	__u6_addr16[8];
		uint32_t	__u6_addr32[4];
	} __u6_addr;			/* 128-bit IP6 address */
};

typedef struct icmp6_hdr {
	uint8_t	 icmp6_type;	/* type field */
	uint8_t	 icmp6_code;	/* code field */
	uint16_t icmp6_cksum;	/* checksum field */
	union {
		uint32_t icmp6_un_data32[1];	/* type-specific field */
		uint16_t icmp6_un_data16[2];	/* type-specific field */
		uint8_t	 icmp6_un_data8[4];	/* type-specific field */
	} icmp6_dataun;
} icmp6_t;

typedef struct mld_hdr {
	struct icmp6_hdr	mld_icmp6_hdr;
	struct in6_addr		mld_addr; /* multicast address */
} mld_hdr_t;

Something else is afoot here, but I'm not sure what.

Nikhil

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

Hi Nikhil,

> On 12/10/20 2:34 PM, Rainer Orth wrote:
>> I've just checked: <netinet/icmp6.h> is effectively unchanged since
>> Solaris 10.
>>
>> Besides, there's gcc211 in the GCC compile farm, running Solaris 11.3/SPARC.
>
> Ah, thanks, I wasn't aware there was a compile farm available to GCC
> developers. I've applied for an account, but it sounds like it may take
> a while to get approved.

it depends: sometimes they're very quick, at others it takes several
reminders.

> My theory was wrong, by the way. This C snippet, representative of the
> Solaris headers, expands just fine:
[...]
> Something else is afoot here, but I'm not sure what.

I'm attaching the -save-temps output, so you can work on the real data
rather than trying to figure things out from the Illumos repos.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University



[-- Attachment #2: sysinfo.i.bz2 --]
[-- Type: application/octet-stream, Size: 38879 bytes --]

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On 12/10/20 4:44 PM, Rainer Orth wrote:
> I'm attaching the -save-temps output, so you can work on the real data
> rather than trying to figure things out from the Illumos repos.

Thanks, that was helpful. I also have successfully acquired access to
gcc211, so I should be self sufficient moving forward. Thanks again for
the pointer to the compile farm.

I believe the attached patch will fix the issue. There are a few parts
to the fix:

   * Typedefs whose name matches the underlying struct tag, as in

         typedef struct FILE {} FILE;
     
     are suppressed, so that we output the interesting "struct FILE {}"
     definition and not the useless "typedef FILE FILE" definition.

   * Rewriting of _in6_addr to [16]byte is applied to all type
     definitions, rather than to an enumerated subset. This follows the
     approach used for timeval/timespec.

     The old approach could not handle cases like:

         type _mld2mar struct { addr _in6_addr /* ... */}
         type _mld2mar_t _mld2mar
     
     The old approach would filter out type _mld2mar but understandably
     was not smart enough to filter out mld2mar_t, resulting in a
     reference to non-existent type _mld2mar.

   * Handling of _timestruc_t when it is just an alias for timespec.

   * Inclusion of stdio.h to avoid an incomplete definition of the type
     __FILE.

There are a few more hurdles before this patch is ready for commit. The
changes to godump.c deserve new test cases. And the changes to the
mk[r]sysinfo.sh scripts will need to go upstream first. Hopefully I'll
get to that soon. I wanted to send along what I had in the meantime.

Nikhil

[-- Attachment #2: solaris-godump.patch --]
[-- Type: text/plain, Size: 8105 bytes --]

diff --git a/gcc/godump.c b/gcc/godump.c
index 033b2c5..c3f4b7b 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -1155,15 +1155,25 @@ go_output_typedef (class godump_container *container, tree decl)
     {
       void **slot;
       const char *type;
+      tree original_type;
 
       type = IDENTIFIER_POINTER (DECL_NAME (decl));
+      original_type = DECL_ORIGINAL_TYPE (decl);
+
+      /* Suppress typedefs where the type name matches the underlying
+	 struct/union/enum tag. This way we'll emit the struct definition
+	 instead of an invalid recursive type.  */
+      if (TYPE_IDENTIFIER (original_type) != NULL
+	  && IDENTIFIER_POINTER (TYPE_IDENTIFIER (original_type)) == type)
+	return;
+
       /* If type defined already, skip.  */
       slot = htab_find_slot (container->type_hash, type, INSERT);
       if (*slot != NULL)
 	return;
       *slot = CONST_CAST (void *, (const void *) type);
 
-      if (!go_format_type (container, DECL_ORIGINAL_TYPE (decl), true, false,
+      if (!go_format_type (container, original_type, true, false,
 			   NULL, false))
 	{
 	  fprintf (go_dump_file, "// ");
@@ -1174,7 +1184,8 @@ go_output_typedef (class godump_container *container, tree decl)
 	       IDENTIFIER_POINTER (DECL_NAME (decl)));
       go_output_type (container);
 
-      if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)))
+      if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl))
+	  || TREE_CODE (TREE_TYPE (decl)) == ENUMERAL_TYPE)
 	{
 	  HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (decl));
 
@@ -1187,7 +1198,9 @@ go_output_typedef (class godump_container *container, tree decl)
 
       container->decls_seen.add (decl);
     }
-  else if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)))
+  else if ((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl))
+	    || TREE_CODE (TREE_TYPE (decl)) == ENUMERAL_TYPE)
+	   && TYPE_NAME (TREE_TYPE (decl)) != NULL)
     {
        void **slot;
        const char *type;
diff --git a/libgo/mkrsysinfo.sh b/libgo/mkrsysinfo.sh
index c28f0e5..29ca1ab 100755
--- a/libgo/mkrsysinfo.sh
+++ b/libgo/mkrsysinfo.sh
@@ -24,11 +24,16 @@ grep -v '^// ' gen-sysinfo.go | \
   grep -v '^type _timespec ' | \
   grep -v '^type _epoll_' | \
   grep -v '^type _*locale[_ ]' | \
-  grep -v 'in6_addr' | \
+  grep -v '^type _in6_addr' | \
   grep -v 'sockaddr_in6' | \
   sed -e 's/\([^a-zA-Z0-9_]\)_timeval\([^a-zA-Z0-9_]\)/\1timeval\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timeval$/\1timeval/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec_t\([^a-zA-Z0-9_]\)/\1timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec_t$/\1timespec_t/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec\([^a-zA-Z0-9_]\)/\1timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec$/\1timespec/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
     >> ${OUT}
 
 # The C long type, needed because that is the type that ptrace returns.
@@ -44,16 +49,6 @@ else
   exit 1
 fi
 
-# On AIX, the _arpcom struct, is filtered by the above grep sequence, as it as
-# a field of type _in6_addr, but other types depend on _arpcom, so we need to
-# put it back.
-grep '^type _arpcom ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
-# Same on Solaris for _mld_hdr_t.
-grep '^type _mld_hdr_t ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
 # The time structures need special handling: we need to name the
 # types, so that we can cast integers to the right types when
 # assigning to the structures.
@@ -174,31 +169,6 @@ if grep "^// type _upad128_t" gen-sysinfo.go > /dev/null 2>&1; then
   echo "type _upad128_t struct { _l [4]uint32; }" >> ${OUT}
 fi
 
-# The Solaris 11 Update 1 _zone_net_addr_t struct.
-grep '^type _zone_net_addr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr/[16]byte/' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_arp_desc_t struct.
-grep '^type _flow_arp_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_l3_desc_t struct.
-grep '^type _flow_l3_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mac_ipaddr_t struct.
-grep '^type _mac_ipaddr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mactun_info_t struct.
-grep '^type _mactun_info_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
 # The Solaris port_event_t struct.
 grep '^type _port_event_t ' gen-sysinfo.go | \
     sed -e s'/_port_event_t/portevent/' \
diff --git a/libgo/mksysinfo.sh b/libgo/mksysinfo.sh
index b32a026..c4df522 100755
--- a/libgo/mksysinfo.sh
+++ b/libgo/mksysinfo.sh
@@ -36,24 +36,20 @@ grep -v '^// ' gen-sysinfo.go | \
   grep -v '^type _timestruc_t ' | \
   grep -v '^type _epoll_' | \
   grep -v '^type _*locale[_ ]' | \
-  grep -v 'in6_addr' | \
+  grep -v '^type _in6_addr' | \
   grep -v 'sockaddr_in6' | \
   sed -e 's/\([^a-zA-Z0-9_]\)_timeval\([^a-zA-Z0-9_]\)/\1Timeval\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timeval$/\1Timeval/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec_t\([^a-zA-Z0-9_]\)/\1Timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec_t$/\1Timespec/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec\([^a-zA-Z0-9_]\)/\1Timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec$/\1Timespec/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timestruc_t\([^a-zA-Z0-9_]\)/\1Timestruc\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timestruc_t$/\1Timestruc/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
     >> ${OUT}
 
-# On AIX, the _arpcom struct, is filtered by the above grep sequence, as it as
-# a field of type _in6_addr, but other types depend on _arpcom, so we need to
-# put it back.
-grep '^type _arpcom ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
-# Same on Solaris for _mld_hdr_t.
-grep '^type _mld_hdr_t ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
 # The errno constants.  These get type Errno.
 egrep '#define E[A-Z0-9_]+ [0-9E]' errno.i | \
   sed -e 's/^#define \(E[A-Z0-9_]*\) .*$/const \1 = Errno(_\1)/' >> ${OUT}
@@ -483,7 +479,9 @@ echo $timespec | \
       -e 's/tv_nsec *[a-zA-Z0-9_]*/Nsec Timespec_nsec_t/' >> ${OUT}
 
 timestruc=`grep '^type _timestruc_t ' gen-sysinfo.go || true`
-if test "$timestruc" != ""; then
+if test "$timestruc" = "type _timestruc_t _timespec"; then
+  echo "type Timestruc Timespec" >> ${OUT}
+elif test "$timestruc" != ""; then
   timestruc_sec=`echo $timestruc | sed -n -e 's/^.*tv_sec \([^ ]*\);.*$/\1/p'`
   timestruc_nsec=`echo $timestruc | sed -n -e 's/^.*tv_nsec \([^ ]*\);.*$/\1/p'`
   echo "type Timestruc_sec_t = $timestruc_sec" >> ${OUT}
@@ -1523,31 +1521,6 @@ if ! grep 'const SizeofICMPv6Filter ' ${OUT} >/dev/null 2>&1; then
     echo 'const SizeofICMPv6Filter = 32' >> ${OUT}
 fi
 
-# The Solaris 11 Update 1 _zone_net_addr_t struct.
-grep '^type _zone_net_addr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr/[16]byte/' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_arp_desc_t struct.
-grep '^type _flow_arp_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_l3_desc_t struct.
-grep '^type _flow_l3_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mac_ipaddr_t struct.
-grep '^type _mac_ipaddr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mactun_info_t struct.
-grep '^type _mactun_info_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
 # Type 'uint128' is needed in a couple of type definitions on arm64,such
 # as _user_fpsimd_struct, _elf_fpregset_t, etc.
 if ! grep '^type uint128' ${OUT} > /dev/null 2>&1; then
diff --git a/libgo/sysinfo.c b/libgo/sysinfo.c
index a060ea8..8ce061e 100644
--- a/libgo/sysinfo.c
+++ b/libgo/sysinfo.c
@@ -11,6 +11,7 @@
 
 #include <stddef.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <sys/types.h>
 #include <dirent.h>
 #include <errno.h>

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On 12/13/20 4:55 PM, Nikhil Benesch wrote:
> There are a few more hurdles before this patch is ready for commit. The
> changes to godump.c deserve new test cases. [...]

Updated patch attached, as promised, and is ready for review.

gcc/:
	* godump.c (go_output_typedef): Suppress typedefs whose name
	matches the tag of the underlying struct, union, or enum.
	Output declarations for enums that do not appear in typedefs.
gcc/testsuite:
	* gcc.misc-tests/godump-1.c: Add test cases.

I don't have write access, so whoever reviews, please commit if approved.

Nikhil

[-- Attachment #2: solaris-godump.patch --]
[-- Type: text/plain, Size: 3820 bytes --]

diff --git a/gcc/godump.c b/gcc/godump.c
index 033b2c59f3c..ff3a4a9c52c 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -1155,15 +1155,25 @@ go_output_typedef (class godump_container *container, tree decl)
     {
       void **slot;
       const char *type;
+      tree original_type;

       type = IDENTIFIER_POINTER (DECL_NAME (decl));
+      original_type = DECL_ORIGINAL_TYPE (decl);
+
+      /* Suppress typedefs where the type name matches the underlying
+	 struct/union/enum tag. This way we'll emit the struct definition
+	 instead of an invalid recursive type.  */
+      if (TYPE_IDENTIFIER (original_type) != NULL
+	  && IDENTIFIER_POINTER (TYPE_IDENTIFIER (original_type)) == type)
+	return;
+
       /* If type defined already, skip.  */
       slot = htab_find_slot (container->type_hash, type, INSERT);
       if (*slot != NULL)
 	return;
       *slot = CONST_CAST (void *, (const void *) type);

-      if (!go_format_type (container, DECL_ORIGINAL_TYPE (decl), true, false,
+      if (!go_format_type (container, original_type, true, false,
 			   NULL, false))
 	{
 	  fprintf (go_dump_file, "// ");
@@ -1187,7 +1197,9 @@ go_output_typedef (class godump_container *container, tree decl)

       container->decls_seen.add (decl);
     }
-  else if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)))
+  else if ((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl))
+	    || TREE_CODE (TREE_TYPE (decl)) == ENUMERAL_TYPE)
+	   && TYPE_NAME (TREE_TYPE (decl)) != NULL)
     {
        void **slot;
        const char *type;
diff --git a/gcc/testsuite/gcc.misc-tests/godump-1.c b/gcc/testsuite/gcc.misc-tests/godump-1.c
index 96c25863374..d37ab0b5af4 100644
--- a/gcc/testsuite/gcc.misc-tests/godump-1.c
+++ b/gcc/testsuite/gcc.misc-tests/godump-1.c
@@ -396,6 +396,15 @@ typedef enum { ET1, ET2 } et_t;
 /* { dg-final { scan-file godump-1.out "(?n)^const _ET1 = 0$" } } */
 /* { dg-final { scan-file godump-1.out "(?n)^const _ET2 = 1$" } } */

+typedef enum e_t_idem_v1 { ETIV1 } e_t_idem_v1;
+/* { dg-final { scan-file godump-1.out "(?n)^type _e_t_idem_v1 u?int\[0-9\]*$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^const _ETIV1 = 0$" } } */
+
+typedef enum e_t_idem_v2 e_t_idem_v2;
+enum e_t_idem_v2 { ETIV2 };
+/* { dg-final { scan-file godump-1.out "(?n)^type _e_t_idem_v2 u?int\[0-9\]*$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^const _ETIV2 = 0$" } } */
+
 enum { ETV1, ETV2 } et_v1;
 /* { dg-final { scan-file godump-1.out "(?n)^var _et_v1 u?int\[0-9\]*$" } } */
 /* { dg-final { scan-file godump-1.out "(?n)^const _ETV1 = 0$" } } */
@@ -477,6 +486,13 @@ struct s_fwd v_fwd;
 struct s_fwd { };
 /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd struct \{ \}$" } } */

+typedef struct s_t_idem_v1 {} s_t_idem_v1;
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_t_idem_v1 struct \{ \}$" } } */
+
+typedef struct s_t_idem_v2 s_t_idem_v2;
+struct s_t_idem_v2 { };
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_t_idem_v2 struct \{ \}$" } } */
+
 /*** nested structs ***/
 typedef struct { struct { uint8_t ca[3]; } s; uint32_t i; } tsn;
 /* { dg-final { scan-file godump-1.out "(?n)^type _tsn struct \{ s struct \{ ca \\\[2\\+1\\\]uint8; \}; i uint32; \}$" } } */
@@ -756,6 +772,13 @@ typedef union { } tue;
 union { } ue;
 /* { dg-final { scan-file godump-1.out "(?n)^var _ue struct \{ \}$" } } */

+typedef union u_t_idem_v1 { } u_t_idem_v1;
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_t_idem_v1 struct \{ \}$" } } */
+
+typedef union u_t_idem_v2 u_t_idem_v2;
+union u_t_idem_v2 { };
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_t_idem_v2 struct \{ \}$" } } */
+
 typedef union { uint8_t c; uint64_t l; } tu1;
 /* { dg-final { scan-file godump-1.out "(?n)^type _tu1 struct \{ c uint8; Godump_0_pad \\\[.\\\]byte; Godump_1_align \\\[0\\\]u?int64; \}$" } } */

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Ian Lance Taylor]

On Sun, Dec 13, 2020 at 3:30 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>
> On 12/13/20 4:55 PM, Nikhil Benesch wrote:
> > There are a few more hurdles before this patch is ready for commit. The
> > changes to godump.c deserve new test cases. [...]
>
> Updated patch attached, as promised, and is ready for review.
>
> gcc/:
>         * godump.c (go_output_typedef): Suppress typedefs whose name
>         matches the tag of the underlying struct, union, or enum.
>         Output declarations for enums that do not appear in typedefs.
> gcc/testsuite:
>         * gcc.misc-tests/godump-1.c: Add test cases.
>
> I don't have write access, so whoever reviews, please commit if approved.

Thanks.  Committed.

Ian

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

Thanks very much, Ian. I'll submit the second half of the patch (the
changes to mk[r]sysinfo.sh) tomorrow.

On Mon, Dec 14, 2020 at 2:39 AM Ian Lance Taylor <iant@golang.org> wrote:
>
> On Sun, Dec 13, 2020 at 3:30 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
> >
> > On 12/13/20 4:55 PM, Nikhil Benesch wrote:
> > > There are a few more hurdles before this patch is ready for commit. The
> > > changes to godump.c deserve new test cases. [...]
> >
> > Updated patch attached, as promised, and is ready for review.
> >
> > gcc/:
> >         * godump.c (go_output_typedef): Suppress typedefs whose name
> >         matches the tag of the underlying struct, union, or enum.
> >         Output declarations for enums that do not appear in typedefs.
> > gcc/testsuite:
> >         * gcc.misc-tests/godump-1.c: Add test cases.
> >
> > I don't have write access, so whoever reviews, please commit if approved.
>
> Thanks.  Committed.
>
> Ian

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Rainer Orth]

Hi Nikhil,

> On 12/10/20 4:44 PM, Rainer Orth wrote:
>> I'm attaching the -save-temps output, so you can work on the real data
>> rather than trying to figure things out from the Illumos repos.
>
> Thanks, that was helpful. I also have successfully acquired access to
> gcc211, so I should be self sufficient moving forward. Thanks again for
> the pointer to the compile farm.
>
> I believe the attached patch will fix the issue. There are a few parts
> to the fix:
>
>   * Typedefs whose name matches the underlying struct tag, as in
>
>         typedef struct FILE {} FILE;
>       are suppressed, so that we output the interesting "struct FILE {}"
>     definition and not the useless "typedef FILE FILE" definition.
>
>   * Rewriting of _in6_addr to [16]byte is applied to all type
>     definitions, rather than to an enumerated subset. This follows the
>     approach used for timeval/timespec.
>
>     The old approach could not handle cases like:
>
>         type _mld2mar struct { addr _in6_addr /* ... */}
>         type _mld2mar_t _mld2mar
>       The old approach would filter out type _mld2mar but understandably
>     was not smart enough to filter out mld2mar_t, resulting in a
>     reference to non-existent type _mld2mar.
>
>   * Handling of _timestruc_t when it is just an alias for timespec.
>
>   * Inclusion of stdio.h to avoid an incomplete definition of the type
>     __FILE.
>
> There are a few more hurdles before this patch is ready for commit. The
> changes to godump.c deserve new test cases. And the changes to the
> mk[r]sysinfo.sh scripts will need to go upstream first. Hopefully I'll
> get to that soon. I wanted to send along what I had in the meantime.

with the revised godump.c patch and this one for mk*sysinfo.sh, I still
get failures on all of Solaris 11.3/x86, 11.4/x86, and 11.4/SPARC
(didn't try 11.3/SPARC):

* Solaris 11.3/x86 and 11.4/x86:

runtime_sysinfo.go:5995:6: error: redefinition of '_upad128_t'

  gen-sysinfo has

// type _upad128_t struct { _q INVALID-float-80; Godump_0_pad [4]byte; }
type _upad128_t struct {}

  and mk*sysinfo.sh adds

type _upad128_t struct { _l [4]uint32; }

* Solaris 11.4/SPARC and x86:

runtime_sysinfo.go:1178:55: error: use of undefined type '_in6_addr_t'

  runtime_sysinfo.go uses _in6_addr_t in _flow_arp_desc_s,
  _flow_l3_desc_s, _mac_ipaddr_s, _mac_resource_props_s, and
  _mactun_info_s, but there's no definition and you've removed the
  section of mk*sysinfo.sh to replace it by [16]byte.

  The issue is here, I believe:

+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \

  Neither line matches _in6_addr_t.  Removing '_' from the second char
  set on the first line fixes this, but I'm unsure what exactly this is
  going to match on different systems.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

On 12/14/20 5:30 AM, Rainer Orth wrote:
> with the revised godump.c patch and this one for mk*sysinfo.sh, I still
> get failures on all of Solaris 11.3/x86, 11.4/x86, and 11.4/SPARC
> (didn't try 11.3/SPARC):
> 
> * Solaris 11.3/x86 and 11.4/x86:
> 
> runtime_sysinfo.go:5995:6: error: redefinition of '_upad128_t'
> 
>    gen-sysinfo has
> 
> // type _upad128_t struct { _q INVALID-float-80; Godump_0_pad [4]byte; }
> type _upad128_t struct {}
> 
>    and mk*sysinfo.sh adds
> 
> type _upad128_t struct { _l [4]uint32; }
> 
> * Solaris 11.4/SPARC and x86:
> 
> runtime_sysinfo.go:1178:55: error: use of undefined type '_in6_addr_t'
> 
>    runtime_sysinfo.go uses _in6_addr_t in _flow_arp_desc_s,
>    _flow_l3_desc_s, _mac_ipaddr_s, _mac_resource_props_s, and
>    _mactun_info_s, but there's no definition and you've removed the
>    section of mk*sysinfo.sh to replace it by [16]byte.
> 
>    The issue is here, I believe:
> 
> +      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
> +      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
> 
>    Neither line matches _in6_addr_t.  Removing '_' from the second char
>    set on the first line fixes this, but I'm unsure what exactly this is
>    going to match on different systems.
> 
> 	Rainer
> 

Sigh. Thanks. Perhaps the attached will work.

(I'm still waiting on my own compilation to complete. Compilation on
gcc211 is glacial. I wonder if I'm doing something wrong.)

The idea is to just rewrite both _in_addr and _in_addr_t. Perhaps
someone smarter than me can figure out how to write a simpler set
of basic REs.

Also godump now emits a dummy `type _u?pad128_t struct {}` entry,
so we just suppress that and conditionally add it back.

If anyone has suggestions for making this less fragile, I'm all ears.

Nikhil

[-- Attachment #2: solaris-mksysinfo.patch --]
[-- Type: text/plain, Size: 7422 bytes --]

diff --git a/libgo/mkrsysinfo.sh b/libgo/mkrsysinfo.sh
index c28f0e5..7aebe3a 100755
--- a/libgo/mkrsysinfo.sh
+++ b/libgo/mkrsysinfo.sh
@@ -24,11 +24,20 @@ grep -v '^// ' gen-sysinfo.go | \
   grep -v '^type _timespec ' | \
   grep -v '^type _epoll_' | \
   grep -v '^type _*locale[_ ]' | \
-  grep -v 'in6_addr' | \
+  grep -v '^type _in6_addr' | \
   grep -v 'sockaddr_in6' | \
+  grep -v '^type _upad128_t' | \
+  grep -v '^type _pad128_t' | \
   sed -e 's/\([^a-zA-Z0-9_]\)_timeval\([^a-zA-Z0-9_]\)/\1timeval\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timeval$/\1timeval/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec_t\([^a-zA-Z0-9_]\)/\1timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec_t$/\1timespec_t/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec\([^a-zA-Z0-9_]\)/\1timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec$/\1timespec/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t$/\1[16]byte/g' \
     >> ${OUT}
 
 # The C long type, needed because that is the type that ptrace returns.
@@ -44,16 +53,6 @@ else
   exit 1
 fi
 
-# On AIX, the _arpcom struct, is filtered by the above grep sequence, as it as
-# a field of type _in6_addr, but other types depend on _arpcom, so we need to
-# put it back.
-grep '^type _arpcom ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
-# Same on Solaris for _mld_hdr_t.
-grep '^type _mld_hdr_t ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
 # The time structures need special handling: we need to name the
 # types, so that we can cast integers to the right types when
 # assigning to the structures.
@@ -169,36 +168,15 @@ grep '^type _sem_t ' gen-sysinfo.go | \
 # double is commented by -fdump-go-spec.
 if grep "^// type _pad128_t" gen-sysinfo.go > /dev/null 2>&1; then
   echo "type _pad128_t struct { _l [4]int32; }" >> ${OUT}
+else
+  grep "^type _pad128_t" gen-sysinfo.go >> ${OUT} || true
 fi
 if grep "^// type _upad128_t" gen-sysinfo.go > /dev/null 2>&1; then
   echo "type _upad128_t struct { _l [4]uint32; }" >> ${OUT}
+else
+  grep "^type _upad128_t" gen-sysinfo.go >> ${OUT} || true
 fi
 
-# The Solaris 11 Update 1 _zone_net_addr_t struct.
-grep '^type _zone_net_addr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr/[16]byte/' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_arp_desc_t struct.
-grep '^type _flow_arp_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_l3_desc_t struct.
-grep '^type _flow_l3_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mac_ipaddr_t struct.
-grep '^type _mac_ipaddr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mactun_info_t struct.
-grep '^type _mactun_info_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
 # The Solaris port_event_t struct.
 grep '^type _port_event_t ' gen-sysinfo.go | \
     sed -e s'/_port_event_t/portevent/' \
diff --git a/libgo/mksysinfo.sh b/libgo/mksysinfo.sh
index b32a026..b2268ac 100755
--- a/libgo/mksysinfo.sh
+++ b/libgo/mksysinfo.sh
@@ -36,24 +36,24 @@ grep -v '^// ' gen-sysinfo.go | \
   grep -v '^type _timestruc_t ' | \
   grep -v '^type _epoll_' | \
   grep -v '^type _*locale[_ ]' | \
-  grep -v 'in6_addr' | \
+  grep -v '^type _in6_addr' | \
+  grep -v '^type _upad128_t' | \
+  grep -v '^type _pad128_t' | \
   grep -v 'sockaddr_in6' | \
   sed -e 's/\([^a-zA-Z0-9_]\)_timeval\([^a-zA-Z0-9_]\)/\1Timeval\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timeval$/\1Timeval/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec_t\([^a-zA-Z0-9_]\)/\1Timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec_t$/\1Timespec/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec\([^a-zA-Z0-9_]\)/\1Timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec$/\1Timespec/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timestruc_t\([^a-zA-Z0-9_]\)/\1Timestruc\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timestruc_t$/\1Timestruc/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t$/\1[16]byte/g' \
     >> ${OUT}
 
-# On AIX, the _arpcom struct, is filtered by the above grep sequence, as it as
-# a field of type _in6_addr, but other types depend on _arpcom, so we need to
-# put it back.
-grep '^type _arpcom ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
-# Same on Solaris for _mld_hdr_t.
-grep '^type _mld_hdr_t ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
 # The errno constants.  These get type Errno.
 egrep '#define E[A-Z0-9_]+ [0-9E]' errno.i | \
   sed -e 's/^#define \(E[A-Z0-9_]*\) .*$/const \1 = Errno(_\1)/' >> ${OUT}
@@ -445,9 +445,13 @@ fi
 # double is commented by -fdump-go-spec.
 if grep "^// type _pad128_t" gen-sysinfo.go > /dev/null 2>&1; then
   echo "type _pad128_t struct { _l [4]int32; }" >> ${OUT}
+else
+  grep "^type _pad128_t" gen-sysinfo.go >> ${OUT} || true
 fi
 if grep "^// type _upad128_t" gen-sysinfo.go > /dev/null 2>&1; then
   echo "type _upad128_t struct { _l [4]uint32; }" >> ${OUT}
+else
+  grep "^type _upad128_t" gen-sysinfo.go >> ${OUT} || true
 fi
 
 # The time_t type.
@@ -483,7 +487,9 @@ echo $timespec | \
       -e 's/tv_nsec *[a-zA-Z0-9_]*/Nsec Timespec_nsec_t/' >> ${OUT}
 
 timestruc=`grep '^type _timestruc_t ' gen-sysinfo.go || true`
-if test "$timestruc" != ""; then
+if test "$timestruc" = "type _timestruc_t _timespec"; then
+  echo "type Timestruc Timespec" >> ${OUT}
+elif test "$timestruc" != ""; then
   timestruc_sec=`echo $timestruc | sed -n -e 's/^.*tv_sec \([^ ]*\);.*$/\1/p'`
   timestruc_nsec=`echo $timestruc | sed -n -e 's/^.*tv_nsec \([^ ]*\);.*$/\1/p'`
   echo "type Timestruc_sec_t = $timestruc_sec" >> ${OUT}
@@ -1523,31 +1529,6 @@ if ! grep 'const SizeofICMPv6Filter ' ${OUT} >/dev/null 2>&1; then
     echo 'const SizeofICMPv6Filter = 32' >> ${OUT}
 fi
 
-# The Solaris 11 Update 1 _zone_net_addr_t struct.
-grep '^type _zone_net_addr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr/[16]byte/' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_arp_desc_t struct.
-grep '^type _flow_arp_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_l3_desc_t struct.
-grep '^type _flow_l3_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mac_ipaddr_t struct.
-grep '^type _mac_ipaddr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mactun_info_t struct.
-grep '^type _mactun_info_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
 # Type 'uint128' is needed in a couple of type definitions on arm64,such
 # as _user_fpsimd_struct, _elf_fpregset_t, etc.
 if ! grep '^type uint128' ${OUT} > /dev/null 2>&1; then
diff --git a/libgo/sysinfo.c b/libgo/sysinfo.c
index a060ea8..8ce061e 100644
--- a/libgo/sysinfo.c
+++ b/libgo/sysinfo.c
@@ -11,6 +11,7 @@
 
 #include <stddef.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <sys/types.h>
 #include <dirent.h>
 #include <errno.h>

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Ian Lance Taylor]

On Mon, Dec 14, 2020 at 7:14 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>
> On 12/14/20 5:30 AM, Rainer Orth wrote:
> > with the revised godump.c patch and this one for mk*sysinfo.sh, I still
> > get failures on all of Solaris 11.3/x86, 11.4/x86, and 11.4/SPARC
> > (didn't try 11.3/SPARC):
> >
> > * Solaris 11.3/x86 and 11.4/x86:
> >
> > runtime_sysinfo.go:5995:6: error: redefinition of '_upad128_t'
> >
> >    gen-sysinfo has
> >
> > // type _upad128_t struct { _q INVALID-float-80; Godump_0_pad [4]byte; }
> > type _upad128_t struct {}
> >
> >    and mk*sysinfo.sh adds
> >
> > type _upad128_t struct { _l [4]uint32; }
> >
> > * Solaris 11.4/SPARC and x86:
> >
> > runtime_sysinfo.go:1178:55: error: use of undefined type '_in6_addr_t'
> >
> >    runtime_sysinfo.go uses _in6_addr_t in _flow_arp_desc_s,
> >    _flow_l3_desc_s, _mac_ipaddr_s, _mac_resource_props_s, and
> >    _mactun_info_s, but there's no definition and you've removed the
> >    section of mk*sysinfo.sh to replace it by [16]byte.
> >
> >    The issue is here, I believe:
> >
> > +      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
> > +      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
> >
> >    Neither line matches _in6_addr_t.  Removing '_' from the second char
> >    set on the first line fixes this, but I'm unsure what exactly this is
> >    going to match on different systems.
> >
> >       Rainer
> >
>
> Sigh. Thanks. Perhaps the attached will work.
>
> (I'm still waiting on my own compilation to complete. Compilation on
> gcc211 is glacial. I wonder if I'm doing something wrong.)
>
> The idea is to just rewrite both _in_addr and _in_addr_t. Perhaps
> someone smarter than me can figure out how to write a simpler set
> of basic REs.
>
> Also godump now emits a dummy `type _u?pad128_t struct {}` entry,
> so we just suppress that and conditionally add it back.

I don't understand this bit.  Why are we seeing an empty struct
definition, and why is this change the right fix?

Is the problem that because we don't know how to emit the definition
of the struct, godump.c winds up emitting an empty definition?  That
doesn't sound like the right thing to do for this case.

Ian

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

On 12/14/20 10:34 PM, Ian Lance Taylor wrote:
> On Mon, Dec 14, 2020 at 7:14 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>> Also godump now emits a dummy `type _u?pad128_t struct {}` entry,
>> so we just suppress that and conditionally add it back.
> 
> I don't understand this bit.  Why are we seeing an empty struct
> definition, and why is this change the right fix?
> 
> Is the problem that because we don't know how to emit the definition
> of the struct, godump.c winds up emitting an empty definition?  That
> doesn't sound like the right thing to do for this case.

That's a good question. u?pad128_t is clearly getting marked as a
potential dummy type. But then you'd think it'd *also* get marked as
an invalid type, and then find_dummy_types would suppress it.

Oh, you know, I bet the change to use DECL_ORIGINAL_TYPE means the
pointers in pot_dummy_types and invalid_types no longer line up. It
might be as simple as this:

diff --git a/gcc/godump.c b/gcc/godump.c
index ff3a4a9..2497037 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -1177,7 +1177,8 @@ go_output_typedef (class godump_container *container, tree decl)
                            NULL, false))
         {
           fprintf (go_dump_file, "// ");
-         slot = htab_find_slot (container->invalid_hash, type, INSERT);
+         slot = htab_find_slot (container->invalid_hash, original_type,
+                                INSERT);
           *slot = CONST_CAST (void *, (const void *) type);
         }
        fprintf (go_dump_file, "type _%s ",

I can't test this theory for a bit though. Probably should couple with a
negative (scan-file-not) test too.

Nikhil

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

On 12/14/20 10:48 PM, Nikhil Benesch wrote:
> On 12/14/20 10:34 PM, Ian Lance Taylor wrote:
>> On Mon, Dec 14, 2020 at 7:14 PM Nikhil Benesch <nikhil.benesch@gmail.com> wrote:
>>> Also godump now emits a dummy `type _u?pad128_t struct {}` entry,
>>> so we just suppress that and conditionally add it back.
>>
>> I don't understand this bit.  Why are we seeing an empty struct
>> definition, and why is this change the right fix?
>>
>> Is the problem that because we don't know how to emit the definition
>> of the struct, godump.c winds up emitting an empty definition?  That
>> doesn't sound like the right thing to do for this case.
> 
> That's a good question. u?pad128_t is clearly getting marked as a
> potential dummy type. But then you'd think it'd *also* get marked as
> an invalid type, and then find_dummy_types would suppress it.

I misunderstood how find_dummy_types worked. Here is a correct
description.

find_dummy_types will output a dummy type for an entry in
pot_dummy_types if:

   (1) we never observed a definition for that type name, or
   (2) we observed an invalid definition for that type name.

I don't actually understand why condition (2) is considered. Since
types that refer to invalid types are themselves marked as invalid,
godump will recursively comment out all types that refer even
transitively to an invalid dummy type. So it doesn't seem necessary to
emit a dummy type in condition (2).

Ian, you added this behavior way back in 2012:

     https://github.com/gcc-mirror/gcc/commit/3eb9e389a6460b6bd9400a3f4acf88fb2e258e07

Perhaps you remember why?

If consensus is this behavior is wrong, I've attached a patch
(invalid-dummy.patch) that removes condition (2) from find_dummy_types
along with some new test cases. I don't have enough context to say
whether this patch is the right solution though.

----

Separately, I'm actually not sure why we're extracting _u?pad128_t from
gen-sysinfo.go at all. Nothing in libgo refers to those types. And no
types in [runtime_]sysinfo.go could refer to them either, because if
they did they'd *also* be marked as invalid (right?). So I suspect we
can just nuke those extractions from mk[r]sysinfo.sh. I've attached a
patch that does so (solaris-godump.patch). I'm still waiting on my
compile to complete, but initial signs look promising.

If this patch looks good, I'll submit it upstream tomorrow.

The nice thing about this approach, if it works, is that it works both
with and without the change to find_dummy_types, so we can consider
that patch independently.

Cheers,
Nikhil

[-- Attachment #2: invalid-dummy.patch --]
[-- Type: text/plain, Size: 2540 bytes --]

diff --git a/gcc/godump.c b/gcc/godump.c
index ff3a4a9c52c..78ff22d5f13 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -1351,11 +1351,9 @@ find_dummy_types (const char *const &ptr, godump_container *adata)
   class godump_container *data = (class godump_container *) adata;
   const char *type = (const char *) ptr;
   void **slot;
-  void **islot;
 
   slot = htab_find_slot (data->type_hash, type, NO_INSERT);
-  islot = htab_find_slot (data->invalid_hash, type, NO_INSERT);
-  if (slot == NULL || islot != NULL)
+  if (slot == NULL)
     fprintf (go_dump_file, "type _%s struct {}\n", type);
   return true;
 }
diff --git a/gcc/testsuite/gcc.misc-tests/godump-1.c b/gcc/testsuite/gcc.misc-tests/godump-1.c
index d37ab0b5af4..e22fb6b8a80 100644
--- a/gcc/testsuite/gcc.misc-tests/godump-1.c
+++ b/gcc/testsuite/gcc.misc-tests/godump-1.c
@@ -304,6 +304,11 @@ long double ld_v1;
 ld_t ld_v2;
 /* { dg-final { scan-file godump-1.out "(?n)^// var _ld_v2 INVALID-float-\[0-9\]*$" } } */
 
+typedef struct ld_s { long double ld_s_f; } ld_s_t;
+/* { dg-final { scan-file godump-1.out "(?n)^// type _ld_s struct \{ ld_s_f INVALID-float-\[0-9\]*; \}$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^// type _ld_s_t _ld_s$" } } *
+/* { dg-final { scan-file-not godump-1.out "(?n)^type _ld_s struct \{\}$" } } */
+
 /*** complex types ***/
 typedef _Complex cx_t;
 /* { dg-final { scan-file godump-1.out "(?n)^type _cx_t complex\[0-9\]*$" } } */
@@ -476,6 +481,8 @@ struct { double d; uint8_t : 0; } sd_not_equiv;
 /* { dg-final { scan-file godump-1.out "(?n)^var _sd_not_equiv struct \{ d float\[0-9\]*; \}$" } } */
 
 typedef struct s_undef_t s_undef_t2;
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_undef_t2 _s_undef_t$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_undef_t struct \{\}$" } } */
 
 typedef struct s_fwd *s_fwd_p;
 /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd_p \\*_s_fwd$" } } */
@@ -816,6 +823,8 @@ union { uint64_t : 1; uint8_t ca[5]; } u2_size;
 /* { dg-final { scan-file godump-1.out "(?n)^var _u2_size struct \{ ca \\\[4\\+1\\\]uint8; \}$" } } */
 
 typedef union u_undef_t u_undef_t2;
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_undef_t2 _u_undef_t$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_undef_t struct \{\}$" } } */
 
 typedef union { uint64_t b : 1; uint8_t ca[5]; } tu3_size;
 /* { dg-final { scan-file godump-1.out "(?n)^type _tu3_size struct \{ ca \\\[4\\+1\\\]uint8; Godump_0_pad \\\[.\\\]byte; Godump_1_align \\\[0\\\]u?int64; \}$" } } */

[-- Attachment #3: solaris-godump.patch --]
[-- Type: text/plain, Size: 7344 bytes --]

diff --git a/libgo/mkrsysinfo.sh b/libgo/mkrsysinfo.sh
index c28f0e5..1bd0956 100755
--- a/libgo/mkrsysinfo.sh
+++ b/libgo/mkrsysinfo.sh
@@ -24,11 +24,18 @@ grep -v '^// ' gen-sysinfo.go | \
   grep -v '^type _timespec ' | \
   grep -v '^type _epoll_' | \
   grep -v '^type _*locale[_ ]' | \
-  grep -v 'in6_addr' | \
+  grep -v '^type _in6_addr' | \
   grep -v 'sockaddr_in6' | \
   sed -e 's/\([^a-zA-Z0-9_]\)_timeval\([^a-zA-Z0-9_]\)/\1timeval\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timeval$/\1timeval/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec_t\([^a-zA-Z0-9_]\)/\1timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec_t$/\1timespec_t/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec\([^a-zA-Z0-9_]\)/\1timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec$/\1timespec/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t$/\1[16]byte/g' \
     >> ${OUT}
 
 # The C long type, needed because that is the type that ptrace returns.
@@ -44,16 +51,6 @@ else
   exit 1
 fi
 
-# On AIX, the _arpcom struct, is filtered by the above grep sequence, as it as
-# a field of type _in6_addr, but other types depend on _arpcom, so we need to
-# put it back.
-grep '^type _arpcom ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
-# Same on Solaris for _mld_hdr_t.
-grep '^type _mld_hdr_t ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
 # The time structures need special handling: we need to name the
 # types, so that we can cast integers to the right types when
 # assigning to the structures.
@@ -165,40 +162,6 @@ fi
 grep '^type _sem_t ' gen-sysinfo.go | \
     sed -e 's/_sem_t/semt/' >> ${OUT}
 
-# Solaris 2 needs _u?pad128_t, but its default definition in terms of long
-# double is commented by -fdump-go-spec.
-if grep "^// type _pad128_t" gen-sysinfo.go > /dev/null 2>&1; then
-  echo "type _pad128_t struct { _l [4]int32; }" >> ${OUT}
-fi
-if grep "^// type _upad128_t" gen-sysinfo.go > /dev/null 2>&1; then
-  echo "type _upad128_t struct { _l [4]uint32; }" >> ${OUT}
-fi
-
-# The Solaris 11 Update 1 _zone_net_addr_t struct.
-grep '^type _zone_net_addr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr/[16]byte/' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_arp_desc_t struct.
-grep '^type _flow_arp_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_l3_desc_t struct.
-grep '^type _flow_l3_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mac_ipaddr_t struct.
-grep '^type _mac_ipaddr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mactun_info_t struct.
-grep '^type _mactun_info_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
 # The Solaris port_event_t struct.
 grep '^type _port_event_t ' gen-sysinfo.go | \
     sed -e s'/_port_event_t/portevent/' \
diff --git a/libgo/mksysinfo.sh b/libgo/mksysinfo.sh
index b32a026..e214d63 100755
--- a/libgo/mksysinfo.sh
+++ b/libgo/mksysinfo.sh
@@ -36,24 +36,22 @@ grep -v '^// ' gen-sysinfo.go | \
   grep -v '^type _timestruc_t ' | \
   grep -v '^type _epoll_' | \
   grep -v '^type _*locale[_ ]' | \
-  grep -v 'in6_addr' | \
+  grep -v '^type _in6_addr' | \
   grep -v 'sockaddr_in6' | \
   sed -e 's/\([^a-zA-Z0-9_]\)_timeval\([^a-zA-Z0-9_]\)/\1Timeval\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timeval$/\1Timeval/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec_t\([^a-zA-Z0-9_]\)/\1Timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec_t$/\1Timespec/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timespec\([^a-zA-Z0-9_]\)/\1Timespec\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timespec$/\1Timespec/g' \
       -e 's/\([^a-zA-Z0-9_]\)_timestruc_t\([^a-zA-Z0-9_]\)/\1Timestruc\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_timestruc_t$/\1Timestruc/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr$/\1[16]byte/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t\([^a-zA-Z0-9_]\)/\1[16]byte\2/g' \
+      -e 's/\([^a-zA-Z0-9_]\)_in6_addr_t$/\1[16]byte/g' \
     >> ${OUT}
 
-# On AIX, the _arpcom struct, is filtered by the above grep sequence, as it as
-# a field of type _in6_addr, but other types depend on _arpcom, so we need to
-# put it back.
-grep '^type _arpcom ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
-# Same on Solaris for _mld_hdr_t.
-grep '^type _mld_hdr_t ' gen-sysinfo.go | \
-  sed -e 's/_in6_addr/[16]byte/' >> ${OUT}
-
 # The errno constants.  These get type Errno.
 egrep '#define E[A-Z0-9_]+ [0-9E]' errno.i | \
   sed -e 's/^#define \(E[A-Z0-9_]*\) .*$/const \1 = Errno(_\1)/' >> ${OUT}
@@ -441,15 +439,6 @@ else
   exit 1
 fi
 
-# Solaris 2 needs _u?pad128_t, but its default definition in terms of long
-# double is commented by -fdump-go-spec.
-if grep "^// type _pad128_t" gen-sysinfo.go > /dev/null 2>&1; then
-  echo "type _pad128_t struct { _l [4]int32; }" >> ${OUT}
-fi
-if grep "^// type _upad128_t" gen-sysinfo.go > /dev/null 2>&1; then
-  echo "type _upad128_t struct { _l [4]uint32; }" >> ${OUT}
-fi
-
 # The time_t type.
 if grep '^type _time_t ' gen-sysinfo.go > /dev/null 2>&1; then
   echo 'type Time_t _time_t' >> ${OUT}
@@ -483,7 +472,9 @@ echo $timespec | \
       -e 's/tv_nsec *[a-zA-Z0-9_]*/Nsec Timespec_nsec_t/' >> ${OUT}
 
 timestruc=`grep '^type _timestruc_t ' gen-sysinfo.go || true`
-if test "$timestruc" != ""; then
+if test "$timestruc" = "type _timestruc_t _timespec"; then
+  echo "type Timestruc Timespec" >> ${OUT}
+elif test "$timestruc" != ""; then
   timestruc_sec=`echo $timestruc | sed -n -e 's/^.*tv_sec \([^ ]*\);.*$/\1/p'`
   timestruc_nsec=`echo $timestruc | sed -n -e 's/^.*tv_nsec \([^ ]*\);.*$/\1/p'`
   echo "type Timestruc_sec_t = $timestruc_sec" >> ${OUT}
@@ -1523,31 +1514,6 @@ if ! grep 'const SizeofICMPv6Filter ' ${OUT} >/dev/null 2>&1; then
     echo 'const SizeofICMPv6Filter = 32' >> ${OUT}
 fi
 
-# The Solaris 11 Update 1 _zone_net_addr_t struct.
-grep '^type _zone_net_addr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr/[16]byte/' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_arp_desc_t struct.
-grep '^type _flow_arp_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.4 _flow_l3_desc_t struct.
-grep '^type _flow_l3_desc_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mac_ipaddr_t struct.
-grep '^type _mac_ipaddr_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
-# The Solaris 11.3 _mactun_info_t struct.
-grep '^type _mactun_info_t ' gen-sysinfo.go | \
-    sed -e 's/_in6_addr_t/[16]byte/g' \
-    >> ${OUT}
-
 # Type 'uint128' is needed in a couple of type definitions on arm64,such
 # as _user_fpsimd_struct, _elf_fpregset_t, etc.
 if ! grep '^type uint128' ${OUT} > /dev/null 2>&1; then
diff --git a/libgo/sysinfo.c b/libgo/sysinfo.c
index a060ea8..8ce061e 100644
--- a/libgo/sysinfo.c
+++ b/libgo/sysinfo.c
@@ -11,6 +11,7 @@
 
 #include <stddef.h>
 #include <stdlib.h>
+#include <stdio.h>
 #include <sys/types.h>
 #include <dirent.h>
 #include <errno.h>

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

On 12/15/20 3:00 AM, Nikhil Benesch wrote:
> If this patch looks good, I'll submit it upstream tomorrow.

Assuming no news is good news, I sent
https://go-review.googlesource.com/c/gofrontend/+/278672.

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Rainer Orth]

Hi Nikhil,

> On 12/15/20 3:00 AM, Nikhil Benesch wrote:
>> If this patch looks good, I'll submit it upstream tomorrow.
>
> Assuming no news is good news, I sent
> https://go-review.googlesource.com/c/gofrontend/+/278672.

sorry for the delay, but unfortunately news is not so good: I get

runtime_sysinfo.go:315:18: error: use of undefined type '_ucontext'
  315 | type _ucontext_t _ucontext
      |                  ^
runtime_sysinfo.go:963:22: error: use of undefined type '_nv_alloc_ops'
  963 | type _nv_alloc_ops_t _nv_alloc_ops
      |                      ^
runtime_sysinfo.go:945:267: error: use of undefined type '_ns_postinit_s'
  945 | type _netstack struct { netstack_u struct { nu_modules [21+1]*byte; }; netstack_m_state [21+1]uint32; netstack_lock _kmutex_t; netstack_next *_netstack; netstack_stackid int32; netstack_numzones int32; netstack_refcnt int32; netstack_flags int32; netstack_postinit *_ns_postinit_s; }
      |                                                                                                                                                                                                                                                                           ^

on both sparc and x86.

gen-sysinfo.go has

type _ucontext_t _ucontext
// type _ucontext struct { uc_flags uint32; uc_link *_ucontext_t; uc_sigmask _sigset_t; uc_stack _stack_t; uc_mcontext _mcontext_t; uc_xrs _xrs_t; uc_filler [2+1]int32; }

type _nv_alloc_ops_t _nv_alloc_ops
// type _nv_alloc_ops struct { nv_ao_init func(*_nv_alloc_t, ___va_list) int32; nv_ao_fini func(*_nv_alloc_t); nv_ao_alloc func(*_nv_alloc_t, uint32) *byte; nv_ao_free func(*_nv_alloc_t, *byte, uint32); nv_ao_reset func(*_nv_alloc_t); }

type _netstack struct { netstack_u struct { nu_modules [21+1]*byte; }; netstack_m_state [21+1]uint32; netstack_lock _kmutex_t; netstack_next *_netstack; netstack_stackid int32; netstack_numzones int32; netstack_refcnt int32; netstack_flags int32; netstack_postinit *_ns_postinit_s; }
// type _ns_postinit_s struct { ns_pi_fn func_ns_applyfn_t; ns_pi_arg uint32; ns_pi_next *_ns_postinit_s; }
// type _ns_postinit_t _ns_postinit_s

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

On 12/16/20 2:20 PM, Rainer Orth wrote:
> Hi Nikhil,
> 
>> On 12/15/20 3:00 AM, Nikhil Benesch wrote:
>>> If this patch looks good, I'll submit it upstream tomorrow.
>>
>> Assuming no news is good news, I sent
>> https://go-review.googlesource.com/c/gofrontend/+/278672.
> 
> sorry for the delay, but unfortunately news is not so good: I get
> 
> runtime_sysinfo.go:315:18: error: use of undefined type '_ucontext'
>    315 | type _ucontext_t _ucontext
>        |                  ^

No problem, Rainer. I figured there would be some hiccups. The somewhat good news
is that this error appears to be independent of the patch I sent upstream.

I suspect what is happening here is that godump sees "typedef ucontext_t struct
ucontext" and outputs the typedef immediately. Only later does it observe that
"struct ucontext" is invalid. At that point it is too late to comment out the
typedef for _ucontext_t.

Nikhil

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

On 12/16/20 3:13 PM, Nikhil Benesch wrote:
> On 12/16/20 2:20 PM, Rainer Orth wrote:
>> Hi Nikhil,
>>
>>> On 12/15/20 3:00 AM, Nikhil Benesch wrote:
>>>> If this patch looks good, I'll submit it upstream tomorrow.
>>>
>>> Assuming no news is good news, I sent
>>> https://go-review.googlesource.com/c/gofrontend/+/278672.
>>
>> sorry for the delay, but unfortunately news is not so good: I get
>>
>> runtime_sysinfo.go:315:18: error: use of undefined type '_ucontext'
>>    315 | type _ucontext_t _ucontext
>>        |                  ^
> 
> No problem, Rainer. I figured there would be some hiccups. The somewhat good news
> is that this error appears to be independent of the patch I sent upstream.
> 
> I suspect what is happening here is that godump sees "typedef ucontext_t struct
> ucontext" and outputs the typedef immediately. Only later does it observe that
> "struct ucontext" is invalid. At that point it is too late to comment out the
> typedef for _ucontext_t.

Oh, wait, Rainer, did you apply *both* solaris-godump.patch and
invalid-dummy.patch? I think if you apply only the former (i.e., only
solaris-godump.patch), which is the only bit I've submitted upstream,
all will be well.

For good measure, I've also fixed the issue in invalid-dummy.patch
and attached a new version. But I'm still not sure whether it is a
worthwhile change, and it's something we can discuss separately from
solaris-godump.patch.

Nikhil

[-- Attachment #2: invalid-dummy-2.patch --]
[-- Type: text/plain, Size: 4413 bytes --]

diff --git a/gcc/godump.c b/gcc/godump.c
index ff3a4a9c52c..b4cbdf275f2 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -670,6 +670,8 @@ go_force_record_alignment (struct obstack *ob, const char *type_string,
   return index;
 }
 
+static void go_output_typedef (class godump_container *container, tree decl);
+
 /* Write the Go version of TYPE to CONTAINER->TYPE_OBSTACK.
    USE_TYPE_NAME is true if we can simply use a type name here without
    needing to define it.  IS_FUNC_OK is true if we can output a func
@@ -705,6 +707,7 @@ go_format_type (class godump_container *container, tree type,
     {
       tree name;
       void **slot;
+      void **islot;
 
       /* References to complex builtin types cannot be translated to
 	Go.  */
@@ -714,10 +717,32 @@ go_format_type (class godump_container *container, tree type,
 
       name = TYPE_IDENTIFIER (type);
 
-      slot = htab_find_slot (container->invalid_hash, IDENTIFIER_POINTER (name),
+
+      slot = htab_find_slot (container->type_hash, IDENTIFIER_POINTER (name),
 			     NO_INSERT);
-      if (slot != NULL)
-	ret = false;
+      islot = htab_find_slot (container->invalid_hash, IDENTIFIER_POINTER (name),
+			     NO_INSERT);
+      if (islot != NULL)
+	{
+	  /* The named type is known to be invalid.  */
+	  ret = false;
+	}
+      else if (slot == NULL)
+	{
+	  size_t pos;
+
+	  /* The named type is not known to be either valid or invalid.
+	     Check to see if the named type is valid. Doing so requires
+	     formatting the type and throwing away the result, which is
+	     a bit silly.  */
+
+	  pos = obstack_object_size (ob);
+
+	  if (!go_format_type (container, type, false, false, NULL, false))
+	    ret = false;
+
+	  ob->next_free = ob->object_base + pos;
+	}
 
       /* References to incomplete structs are permitted in many
 	 contexts, like behind a pointer or inside of a typedef. So
@@ -1351,11 +1376,9 @@ find_dummy_types (const char *const &ptr, godump_container *adata)
   class godump_container *data = (class godump_container *) adata;
   const char *type = (const char *) ptr;
   void **slot;
-  void **islot;
 
   slot = htab_find_slot (data->type_hash, type, NO_INSERT);
-  islot = htab_find_slot (data->invalid_hash, type, NO_INSERT);
-  if (slot == NULL || islot != NULL)
+  if (slot == NULL)
     fprintf (go_dump_file, "type _%s struct {}\n", type);
   return true;
 }
diff --git a/gcc/testsuite/gcc.misc-tests/godump-1.c b/gcc/testsuite/gcc.misc-tests/godump-1.c
index d37ab0b5af4..4492a6e4887 100644
--- a/gcc/testsuite/gcc.misc-tests/godump-1.c
+++ b/gcc/testsuite/gcc.misc-tests/godump-1.c
@@ -304,6 +304,13 @@ long double ld_v1;
 ld_t ld_v2;
 /* { dg-final { scan-file godump-1.out "(?n)^// var _ld_v2 INVALID-float-\[0-9\]*$" } } */
 
+typedef struct ld_s ld_s_t1;
+typedef struct ld_s { long double ld_s_f; } ld_s_t2;
+/* { dg-final { scan-file godump-1.out "(?n)^// type _ld_s_t1 _ld_s$" } } *
+/* { dg-final { scan-file godump-1.out "(?n)^// type _ld_s struct \{ ld_s_f INVALID-float-\[0-9\]*; \}$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^// type _ld_s_t2 _ld_s$" } } *
+/* { dg-final { scan-file-not godump-1.out "(?n)^type _ld_s struct \{\}$" } } */
+
 /*** complex types ***/
 typedef _Complex cx_t;
 /* { dg-final { scan-file godump-1.out "(?n)^type _cx_t complex\[0-9\]*$" } } */
@@ -476,6 +483,8 @@ struct { double d; uint8_t : 0; } sd_not_equiv;
 /* { dg-final { scan-file godump-1.out "(?n)^var _sd_not_equiv struct \{ d float\[0-9\]*; \}$" } } */
 
 typedef struct s_undef_t s_undef_t2;
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_undef_t2 _s_undef_t$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_undef_t struct \{\}$" } } */
 
 typedef struct s_fwd *s_fwd_p;
 /* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd_p \\*_s_fwd$" } } */
@@ -816,6 +825,8 @@ union { uint64_t : 1; uint8_t ca[5]; } u2_size;
 /* { dg-final { scan-file godump-1.out "(?n)^var _u2_size struct \{ ca \\\[4\\+1\\\]uint8; \}$" } } */
 
 typedef union u_undef_t u_undef_t2;
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_undef_t2 _u_undef_t$" } } */
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_undef_t struct \{\}$" } } */
 
 typedef union { uint64_t b : 1; uint8_t ca[5]; } tu3_size;
 /* { dg-final { scan-file godump-1.out "(?n)^type _tu3_size struct \{ ca \\\[4\\+1\\\]uint8; Godump_0_pad \\\[.\\\]byte; Godump_1_align \\\[0\\\]u?int64; \}$" } } */

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Rainer Orth]

Hi Nikhil,

>> I suspect what is happening here is that godump sees "typedef ucontext_t struct
>> ucontext" and outputs the typedef immediately. Only later does it observe that
>> "struct ucontext" is invalid. At that point it is too late to comment out the
>> typedef for _ucontext_t.
>
> Oh, wait, Rainer, did you apply *both* solaris-godump.patch and
> invalid-dummy.patch? I think if you apply only the former (i.e., only

I did indeed.  TBH I sort of lost track which patches were and weren't
required to get this fixed ;-)

> solaris-godump.patch), which is the only bit I've submitted upstream,
> all will be well.
>
> For good measure, I've also fixed the issue in invalid-dummy.patch
> and attached a new version. But I'm still not sure whether it is a
> worthwhile change, and it's something we can discuss separately from
> solaris-godump.patch.

I first tried with the new version included, but that broke badly:

cc1 -fpreprocessed sysinfo.i -quiet -O -std=gnu99 -fdump-go-spec=tmp-gen-sysinfo.go -o sysinfo.s

now SEGVs with either infinite or very deep recursion:

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1 (LWP 1)]
0x08ea285e in go_format_type (container=container@entry=0xfeffd738, type=
    <record_type 0xfa50be40>, use_type_name=true, is_func_ok=true, 
    p_art_i=0x0, is_anon_record_or_union=false)
    at /vol/gcc/src/hg/master/local/gcc/godump.c:688
688	{
(gdb) where
#0  0x08ea285e in go_format_type (container=container@entry=0xfeffd738, 
    type=<record_type 0xfa50be40>, use_type_name=true, is_func_ok=true, 
    p_art_i=0x0, is_anon_record_or_union=false)
    at /vol/gcc/src/hg/master/local/gcc/godump.c:688
#1  0x08ea2cff in go_format_type (container=container@entry=0xfeffd738, 
    type=type@entry=<pointer_type 0xfa50bea0>, 
    use_type_name=use_type_name@entry=true, is_func_ok=false, 
    p_art_i=0xfe6fe174, is_anon_record_or_union=false)
    at /vol/gcc/src/hg/master/local/gcc/tree.h:3453
#2  0x08ea3506 in go_format_type (container=container@entry=0xfeffd738, 
    type=type@entry=<record_type 0xfa50be40>, 
    use_type_name=use_type_name@entry=false, is_func_ok=false, 
    p_art_i=0xfe6fe174, is_anon_record_or_union=false)
    at /vol/gcc/src/hg/master/local/gcc/godump.c:1002
#3  0x08ea3335 in go_format_type (container=container@entry=0xfeffd738, 
    type=<record_type 0xfa50be40>, use_type_name=<optimized out>, 
    is_func_ok=true, p_art_i=0xfe6fe234, is_anon_record_or_union=false)
    at /vol/gcc/src/hg/master/local/gcc/godump.c:741
#4  0x08ea2cff in go_format_type (container=container@entry=0xfeffd738, 
    type=type@entry=<pointer_type 0xfa50bea0>, 
    use_type_name=use_type_name@entry=true, is_func_ok=false, 
    p_art_i=0xfe6fe3b4, is_anon_record_or_union=false)
    at /vol/gcc/src/hg/master/local/gcc/tree.h:3453

I've now reverted that part and the build is into make check, as you
suspected.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

On 12/17/20 7:28 AM, Rainer Orth wrote:
> I first tried with the new version included, but that broke badly:
> 
> cc1 -fpreprocessed sysinfo.i -quiet -O -std=gnu99 -fdump-go-spec=tmp-gen-sysinfo.go -o sysinfo.s
> 
> now SEGVs with either infinite or very deep recursion:
> 
> Thread 2 received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1 (LWP 1)]
> 0x08ea285e in go_format_type (container=container@entry=0xfeffd738, type=
>      <record_type 0xfa50be40>, use_type_name=true, is_func_ok=true,
>      p_art_i=0x0, is_anon_record_or_union=false)
>      at /vol/gcc/src/hg/master/local/gcc/godump.c:688
> 688	{
> (gdb) where
> #0  0x08ea285e in go_format_type (container=container@entry=0xfeffd738,
>      type=<record_type 0xfa50be40>, use_type_name=true, is_func_ok=true,
>      p_art_i=0x0, is_anon_record_or_union=false)
>      at /vol/gcc/src/hg/master/local/gcc/godump.c:688
> #1  0x08ea2cff in go_format_type (container=container@entry=0xfeffd738,
>      type=type@entry=<pointer_type 0xfa50bea0>,
>      use_type_name=use_type_name@entry=true, is_func_ok=false,
>      p_art_i=0xfe6fe174, is_anon_record_or_union=false)
>      at /vol/gcc/src/hg/master/local/gcc/tree.h:3453
> #2  0x08ea3506 in go_format_type (container=container@entry=0xfeffd738,
>      type=type@entry=<record_type 0xfa50be40>,
>      use_type_name=use_type_name@entry=false, is_func_ok=false,
>      p_art_i=0xfe6fe174, is_anon_record_or_union=false)
>      at /vol/gcc/src/hg/master/local/gcc/godump.c:1002
> #3  0x08ea3335 in go_format_type (container=container@entry=0xfeffd738,
>      type=<record_type 0xfa50be40>, use_type_name=<optimized out>,
>      is_func_ok=true, p_art_i=0xfe6fe234, is_anon_record_or_union=false)
>      at /vol/gcc/src/hg/master/local/gcc/godump.c:741
> #4  0x08ea2cff in go_format_type (container=container@entry=0xfeffd738,
>      type=type@entry=<pointer_type 0xfa50bea0>,
>      use_type_name=use_type_name@entry=true, is_func_ok=false,
>      p_art_i=0xfe6fe3b4, is_anon_record_or_union=false)
>      at /vol/gcc/src/hg/master/local/gcc/tree.h:3453
> 
> I've now reverted that part and the build is into make check, as you
> suspected.

Whew, ok, great. Let's leave invalid-godump-2.patch as a curiosity for
Ian, then. The current approach of outputting dummy types for every
invalid type is unsatisfying, but in practice it seems to work alright.

Nikhil

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Ian Lance Taylor]

On Thu, Dec 17, 2020 at 8:31 AM Nikhil Benesch via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 12/17/20 7:28 AM, Rainer Orth wrote:
> > I first tried with the new version included, but that broke badly:
> >
> > cc1 -fpreprocessed sysinfo.i -quiet -O -std=gnu99 -fdump-go-spec=tmp-gen-sysinfo.go -o sysinfo.s
> >
> > now SEGVs with either infinite or very deep recursion:
> >
> > Thread 2 received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 1 (LWP 1)]
> > 0x08ea285e in go_format_type (container=container@entry=0xfeffd738, type=
> >      <record_type 0xfa50be40>, use_type_name=true, is_func_ok=true,
> >      p_art_i=0x0, is_anon_record_or_union=false)
> >      at /vol/gcc/src/hg/master/local/gcc/godump.c:688
> > 688   {
> > (gdb) where
> > #0  0x08ea285e in go_format_type (container=container@entry=0xfeffd738,
> >      type=<record_type 0xfa50be40>, use_type_name=true, is_func_ok=true,
> >      p_art_i=0x0, is_anon_record_or_union=false)
> >      at /vol/gcc/src/hg/master/local/gcc/godump.c:688
> > #1  0x08ea2cff in go_format_type (container=container@entry=0xfeffd738,
> >      type=type@entry=<pointer_type 0xfa50bea0>,
> >      use_type_name=use_type_name@entry=true, is_func_ok=false,
> >      p_art_i=0xfe6fe174, is_anon_record_or_union=false)
> >      at /vol/gcc/src/hg/master/local/gcc/tree.h:3453
> > #2  0x08ea3506 in go_format_type (container=container@entry=0xfeffd738,
> >      type=type@entry=<record_type 0xfa50be40>,
> >      use_type_name=use_type_name@entry=false, is_func_ok=false,
> >      p_art_i=0xfe6fe174, is_anon_record_or_union=false)
> >      at /vol/gcc/src/hg/master/local/gcc/godump.c:1002
> > #3  0x08ea3335 in go_format_type (container=container@entry=0xfeffd738,
> >      type=<record_type 0xfa50be40>, use_type_name=<optimized out>,
> >      is_func_ok=true, p_art_i=0xfe6fe234, is_anon_record_or_union=false)
> >      at /vol/gcc/src/hg/master/local/gcc/godump.c:741
> > #4  0x08ea2cff in go_format_type (container=container@entry=0xfeffd738,
> >      type=type@entry=<pointer_type 0xfa50bea0>,
> >      use_type_name=use_type_name@entry=true, is_func_ok=false,
> >      p_art_i=0xfe6fe3b4, is_anon_record_or_union=false)
> >      at /vol/gcc/src/hg/master/local/gcc/tree.h:3453
> >
> > I've now reverted that part and the build is into make check, as you
> > suspected.
>
> Whew, ok, great. Let's leave invalid-godump-2.patch as a curiosity for
> Ian, then. The current approach of outputting dummy types for every
> invalid type is unsatisfying, but in practice it seems to work alright.

Thanks for looking into this.  I've gotten confused now, though.
Which patch fixes the problem on Solaris?  Thanks.

Ian

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Nikhil Benesch]

On 12/17/20 1:05 PM, Ian Lance Taylor wrote:
> Thanks for looking into this.  I've gotten confused now, though.
> Which patch fixes the problem on Solaris?  Thanks.
> 
> Ian

The patch I submitted upstream is all that is needed to fix the
compilation failures on Solaris:

     https://go-review.googlesource.com/c/gofrontend/+/278672

That is the same patch as "solaris-godump.patch" in this thread.

That patch does not fix the fishiness around dummy types that you
pointed out earlier. That is what the other patch was trying (but
failing) to address. But we don't need to fix that fishiness to fix
Solaris. The code in mk[r]sysinfo.sh that is getting confused by the
u?pad128_t dummy type turns out to be unnecessary. So the patch I sent
upstream just removes that code.

Nikhil

Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types

[Ian Lance Taylor]

On Thu, Dec 17, 2020 at 10:22 AM Nikhil Benesch
<nikhil.benesch@gmail.com> wrote:
>
> On 12/17/20 1:05 PM, Ian Lance Taylor wrote:
> > Thanks for looking into this.  I've gotten confused now, though.
> > Which patch fixes the problem on Solaris?  Thanks.
> >
> > Ian
>
> The patch I submitted upstream is all that is needed to fix the
> compilation failures on Solaris:
>
>      https://go-review.googlesource.com/c/gofrontend/+/278672
>
> That is the same patch as "solaris-godump.patch" in this thread.
>
> That patch does not fix the fishiness around dummy types that you
> pointed out earlier. That is what the other patch was trying (but
> failing) to address. But we don't need to fix that fishiness to fix
> Solaris. The code in mk[r]sysinfo.sh that is getting confused by the
> u?pad128_t dummy type turns out to be unnecessary. So the patch I sent
> upstream just removes that code.

Thanks.  Committed to mainline.

Ian