* [PATCH] Correct -fdump-go-spec's handling of incomplete types @ 2020-12-08 22:57 Nikhil Benesch 2020-12-10 2:48 ` Ian Lance Taylor 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-08 22:57 UTC (permalink / raw) To: gcc-patches, Ian Lance Taylor 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 \{ \}$" } } */ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-08 22:57 [PATCH] Correct -fdump-go-spec's handling of incomplete types Nikhil Benesch @ 2020-12-10 2:48 ` Ian Lance Taylor 2020-12-10 19:18 ` Rainer Orth 0 siblings, 1 reply; 25+ messages in thread From: Ian Lance Taylor @ 2020-12-10 2:48 UTC (permalink / raw) To: Nikhil Benesch; +Cc: gcc-patches [-- 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 \{ \}$" } } */ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-10 2:48 ` Ian Lance Taylor @ 2020-12-10 19:18 ` Rainer Orth 2020-12-10 19:31 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Rainer Orth @ 2020-12-10 19:18 UTC (permalink / raw) To: Ian Lance Taylor via Gcc-patches Cc: Nikhil Benesch, Ian Lance Taylor, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-10 19:18 ` Rainer Orth @ 2020-12-10 19:31 ` Nikhil Benesch 2020-12-10 19:34 ` Rainer Orth 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-10 19:31 UTC (permalink / raw) To: Rainer Orth Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-10 19:31 ` Nikhil Benesch @ 2020-12-10 19:34 ` Rainer Orth 2020-12-10 19:49 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Rainer Orth @ 2020-12-10 19:34 UTC (permalink / raw) To: Nikhil Benesch Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-10 19:34 ` Rainer Orth @ 2020-12-10 19:49 ` Nikhil Benesch 2020-12-10 21:44 ` Rainer Orth 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-10 19:49 UTC (permalink / raw) To: Rainer Orth Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-10 19:49 ` Nikhil Benesch @ 2020-12-10 21:44 ` Rainer Orth 2020-12-13 21:55 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Rainer Orth @ 2020-12-10 21:44 UTC (permalink / raw) To: Nikhil Benesch Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches [-- 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 --] ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-10 21:44 ` Rainer Orth @ 2020-12-13 21:55 ` Nikhil Benesch 2020-12-13 23:30 ` Nikhil Benesch 2020-12-14 10:30 ` Rainer Orth 0 siblings, 2 replies; 25+ messages in thread From: Nikhil Benesch @ 2020-12-13 21:55 UTC (permalink / raw) To: Rainer Orth Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches [-- 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-13 21:55 ` Nikhil Benesch @ 2020-12-13 23:30 ` Nikhil Benesch 2020-12-14 7:39 ` Ian Lance Taylor 2020-12-14 10:30 ` Rainer Orth 1 sibling, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-13 23:30 UTC (permalink / raw) To: Rainer Orth Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches [-- 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; \}$" } } */ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-13 23:30 ` Nikhil Benesch @ 2020-12-14 7:39 ` Ian Lance Taylor 2020-12-14 8:22 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Ian Lance Taylor @ 2020-12-14 7:39 UTC (permalink / raw) To: Nikhil Benesch; +Cc: Rainer Orth, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-14 7:39 ` Ian Lance Taylor @ 2020-12-14 8:22 ` Nikhil Benesch 0 siblings, 0 replies; 25+ messages in thread From: Nikhil Benesch @ 2020-12-14 8:22 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Rainer Orth, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-13 21:55 ` Nikhil Benesch 2020-12-13 23:30 ` Nikhil Benesch @ 2020-12-14 10:30 ` Rainer Orth 2020-12-15 3:14 ` Nikhil Benesch 1 sibling, 1 reply; 25+ messages in thread From: Rainer Orth @ 2020-12-14 10:30 UTC (permalink / raw) To: Nikhil Benesch Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-14 10:30 ` Rainer Orth @ 2020-12-15 3:14 ` Nikhil Benesch 2020-12-15 3:34 ` Ian Lance Taylor 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-15 3:14 UTC (permalink / raw) To: Rainer Orth Cc: Ian Lance Taylor via Gcc-patches, Ian Lance Taylor, gcc-patches [-- 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-15 3:14 ` Nikhil Benesch @ 2020-12-15 3:34 ` Ian Lance Taylor 2020-12-15 3:48 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Ian Lance Taylor @ 2020-12-15 3:34 UTC (permalink / raw) To: Nikhil Benesch; +Cc: Rainer Orth, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-15 3:34 ` Ian Lance Taylor @ 2020-12-15 3:48 ` Nikhil Benesch 2020-12-15 8:00 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-15 3:48 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Rainer Orth, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-15 3:48 ` Nikhil Benesch @ 2020-12-15 8:00 ` Nikhil Benesch 2020-12-16 14:00 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-15 8:00 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Rainer Orth, gcc-patches, gofrontend-dev [-- 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-15 8:00 ` Nikhil Benesch @ 2020-12-16 14:00 ` Nikhil Benesch 2020-12-16 19:20 ` Rainer Orth 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-16 14:00 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Rainer Orth, gcc-patches, gofrontend-dev 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-16 14:00 ` Nikhil Benesch @ 2020-12-16 19:20 ` Rainer Orth 2020-12-16 20:13 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Rainer Orth @ 2020-12-16 19:20 UTC (permalink / raw) To: Nikhil Benesch; +Cc: Ian Lance Taylor, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-16 19:20 ` Rainer Orth @ 2020-12-16 20:13 ` Nikhil Benesch 2020-12-17 4:59 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-16 20:13 UTC (permalink / raw) To: Rainer Orth; +Cc: Ian Lance Taylor, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-16 20:13 ` Nikhil Benesch @ 2020-12-17 4:59 ` Nikhil Benesch 2020-12-17 12:28 ` Rainer Orth 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-17 4:59 UTC (permalink / raw) To: Rainer Orth; +Cc: Ian Lance Taylor, gcc-patches, gofrontend-dev [-- 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; \}$" } } */ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-17 4:59 ` Nikhil Benesch @ 2020-12-17 12:28 ` Rainer Orth 2020-12-17 16:31 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Rainer Orth @ 2020-12-17 12:28 UTC (permalink / raw) To: Nikhil Benesch; +Cc: Ian Lance Taylor, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-17 12:28 ` Rainer Orth @ 2020-12-17 16:31 ` Nikhil Benesch 2020-12-17 18:05 ` Ian Lance Taylor 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-17 16:31 UTC (permalink / raw) To: Rainer Orth; +Cc: Ian Lance Taylor, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-17 16:31 ` Nikhil Benesch @ 2020-12-17 18:05 ` Ian Lance Taylor 2020-12-17 18:21 ` Nikhil Benesch 0 siblings, 1 reply; 25+ messages in thread From: Ian Lance Taylor @ 2020-12-17 18:05 UTC (permalink / raw) To: Nikhil Benesch; +Cc: Rainer Orth, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-17 18:05 ` Ian Lance Taylor @ 2020-12-17 18:21 ` Nikhil Benesch 2020-12-21 4:16 ` Ian Lance Taylor 0 siblings, 1 reply; 25+ messages in thread From: Nikhil Benesch @ 2020-12-17 18:21 UTC (permalink / raw) To: Ian Lance Taylor; +Cc: Rainer Orth, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types 2020-12-17 18:21 ` Nikhil Benesch @ 2020-12-21 4:16 ` Ian Lance Taylor 0 siblings, 0 replies; 25+ messages in thread From: Ian Lance Taylor @ 2020-12-21 4:16 UTC (permalink / raw) To: Nikhil Benesch; +Cc: Rainer Orth, gcc-patches, gofrontend-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2020-12-21 4:17 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-08 22:57 [PATCH] Correct -fdump-go-spec's handling of incomplete types Nikhil Benesch 2020-12-10 2:48 ` Ian Lance Taylor 2020-12-10 19:18 ` Rainer Orth 2020-12-10 19:31 ` Nikhil Benesch 2020-12-10 19:34 ` Rainer Orth 2020-12-10 19:49 ` Nikhil Benesch 2020-12-10 21:44 ` Rainer Orth 2020-12-13 21:55 ` Nikhil Benesch 2020-12-13 23:30 ` Nikhil Benesch 2020-12-14 7:39 ` Ian Lance Taylor 2020-12-14 8:22 ` Nikhil Benesch 2020-12-14 10:30 ` Rainer Orth 2020-12-15 3:14 ` Nikhil Benesch 2020-12-15 3:34 ` Ian Lance Taylor 2020-12-15 3:48 ` Nikhil Benesch 2020-12-15 8:00 ` Nikhil Benesch 2020-12-16 14:00 ` Nikhil Benesch 2020-12-16 19:20 ` Rainer Orth 2020-12-16 20:13 ` Nikhil Benesch 2020-12-17 4:59 ` Nikhil Benesch 2020-12-17 12:28 ` Rainer Orth 2020-12-17 16:31 ` Nikhil Benesch 2020-12-17 18:05 ` Ian Lance Taylor 2020-12-17 18:21 ` Nikhil Benesch 2020-12-21 4:16 ` Ian Lance Taylor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).