[PATCH] bootstrap: include string/vector before abort redefinition
Similar to 1467a5c and PR 98412. Bootstrapping on FreeBSD 12.2 with the default compiler (Clang 10.0.1) fails if the and headers are included after abort is redefined to fancy_abort. Appears to be missing defines in the new C++ module code. The following patch is sufficient. gcc/cp/ChangeLog: * module.cc: INCLUDE_STRING, INCLUDE_VECTOR. diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 7e38293545f..ed3dbe244a3 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -207,6 +207,8 @@ Classes used: #define _DEFAULT_SOURCE 1 /* To get TZ field of struct tm, if available. */ #include "config.h" +#define INCLUDE_STRING +#define INCLUDE_VECTOR #include "system.h" #include "coretypes.h" #include "cp-tree.h"
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
On 12/17/20 1:05 PM, Ian Lance Taylor wrote: Thanks for looking into this. I've gotten confused now, though. Which patch fixes the problem on Solaris? Thanks. Ian The patch I submitted upstream is all that is needed to fix the compilation failures on Solaris: https://go-review.googlesource.com/c/gofrontend/+/278672 That is the same patch as "solaris-godump.patch" in this thread. That patch does not fix the fishiness around dummy types that you pointed out earlier. That is what the other patch was trying (but failing) to address. But we don't need to fix that fishiness to fix Solaris. The code in mk[r]sysinfo.sh that is getting confused by the u?pad128_t dummy type turns out to be unnecessary. So the patch I sent upstream just removes that code. Nikhil
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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=
, 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=, 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=,
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=,
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=, use_type_name=,
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=,
use_type_name=use_type_name@entry=true, is_func_ok=false,
p_art_i=0xfe6fe3b4, is_anon_record_or_union=false)
at /vol/gcc/src/hg/master/local/gcc/tree.h:3453
I've now reverted that part and the build is into make check, as you
suspected.
Whew, ok, great. Let's leave invalid-godump-2.patch as a curiosity for
Ian, then. The current approach of outputting dummy types for every
invalid type is unsatisfying, but in practice it seems to work alright.
Nikhil
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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
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 ,
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
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
On 12/16/20 2:20 PM, Rainer Orth wrote: Hi Nikhil, On 12/15/20 3:00 AM, Nikhil Benesch wrote: If this patch looks good, I'll submit it upstream tomorrow. Assuming no news is good news, I sent https://go-review.googlesource.com/c/gofrontend/+/278672. sorry for the delay, but unfortunately news is not so good: I get runtime_sysinfo.go:315:18: error: use of undefined type '_ucontext' 315 | type _ucontext_t _ucontext | ^ No problem, Rainer. I figured there would be some hiccups. The somewhat good news is that this error appears to be independent of the patch I sent upstream. I suspect what is happening here is that godump sees "typedef ucontext_t struct ucontext" and outputs the typedef immediately. Only later does it observe that "struct ucontext" is invalid. At that point it is too late to comment out the typedef for _ucontext_t. Nikhil
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
On 12/15/20 3:00 AM, Nikhil Benesch wrote: If this patch looks good, I'll submit it upstream tomorrow. Assuming no news is good news, I sent https://go-review.googlesource.com/c/gofrontend/+/278672.
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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 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
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 ,
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
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
On 12/14/20 10:34 PM, Ian Lance Taylor wrote:
On Mon, Dec 14, 2020 at 7:14 PM Nikhil Benesch wrote:
Also godump now emits a dummy `type _u?pad128_t struct {}` entry,
so we just suppress that and conditionally add it back.
I don't understand this bit. Why are we seeing an empty struct
definition, and why is this change the right fix?
Is the problem that because we don't know how to emit the definition
of the struct, godump.c winds up emitting an empty definition? That
doesn't sound like the right thing to do for this case.
That's a good question. u?pad128_t is clearly getting marked as a
potential dummy type. But then you'd think it'd *also* get marked as
an invalid type, and then find_dummy_types would suppress it.
Oh, you know, I bet the change to use DECL_ORIGINAL_TYPE means the
pointers in pot_dummy_types and invalid_types no longer line up. It
might be as simple as this:
diff --git a/gcc/godump.c b/gcc/godump.c
index ff3a4a9..2497037 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -1177,7 +1177,8 @@ go_output_typedef (class godump_container *container,
tree decl)
NULL, false))
{
fprintf (go_dump_file, "// ");
- slot = htab_find_slot (container->invalid_hash, type, INSERT);
+ slot = htab_find_slot (container->invalid_hash, original_type,
+INSERT);
*slot = CONST_CAST (void *, (const void *) type);
}
fprintf (go_dump_file, "type _%s ",
I can't test this theory for a bit though. Probably should couple with a
negative (scan-file-not) test too.
Nikhil
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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
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
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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 wrote: > > On Sun, Dec 13, 2020 at 3:30 PM Nikhil Benesch > wrote: > > > > On 12/13/20 4:55 PM, Nikhil Benesch wrote: > > > There are a few more hurdles before this patch is ready for commit. The > > > changes to godump.c deserve new test cases. [...] > > > > Updated patch attached, as promised, and is ready for review. > > > > gcc/: > > * godump.c (go_output_typedef): Suppress typedefs whose name > > matches the tag of the underlying struct, union, or enum. > > Output declarations for enums that do not appear in typedefs. > > gcc/testsuite: > > * gcc.misc-tests/godump-1.c: Add test cases. > > > > I don't have write access, so whoever reviews, please commit if approved. > > Thanks. Committed. > > Ian
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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
diff --git a/gcc/godump.c b/gcc/godump.c
index 033b2c59f3c..ff3a4a9c52c 100644
--- a/gcc/godump.c
+++ b/gcc/godump.c
@@ -1155,15 +1155,25 @@ go_output_typedef (class godump_container *container,
tree decl)
{
void **slot;
const char *type;
+ tree original_type;
type = IDENTIFIER_POINTER (DECL_NAME (decl));
+ original_type = DECL_ORIGINAL_TYPE (decl);
+
+ /* Suppress typedefs where the type name matches the underlying
+struct/union/enum tag. This way we'll emit the struct definition
+instead of an invalid recursive type. */
+ if (TYPE_IDENTIFIER (original_type) != NULL
+ && IDENTIFIER_POINTER (TYPE_IDENTIFIER (original_type)) == type)
+ return;
+
/* If type defined already, skip. */
slot = htab_find_slot (container->type_hash, type, INSERT);
if (*slot != NULL)
return;
*slot = CONST_CAST (void *, (const void *) type);
- if (!go_format_type (container, DECL_ORIGINAL_TYPE (decl), true, false,
+ if (!go_format_type (container, original_type, true, false,
NULL, false))
{
fprintf (go_dump_file, "// ");
@@ -1187,7 +1197,9 @@ go_output_typedef (class godump_container *container,
tree decl)
container->decls_seen.add (decl);
}
- else if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl)))
+ else if ((RECORD_OR_UNION_TYPE_P (TREE_TYPE (decl))
+ || TREE_CODE (TREE_TYPE (decl)) == ENUMERAL_TYPE)
+ && TYPE_NAME (TREE_TYPE (decl)) != NULL)
{
void **slot;
const char *type;
diff --git a/gcc/testsuite/gcc.misc-tests/godump-1.c
b/gcc/testsuite/gcc.misc-tests/godump-1.c
index 96c25863374..d37ab0b5af4 100644
--- a/gcc/testsuite/gcc.misc-tests/godump-1.c
+++ b/gcc/testsuite/gcc.misc-tests/godump-1.c
@@ -396,6 +396,15 @@ typedef enum { ET1, ET2 } et_t;
/* { dg-final { scan-file godump-1.out "(?n)^const _ET1 = 0$" } } */
/* { dg-final { scan-file godump-1.out "(?n)^const _ET2 = 1$" } } */
+typedef enum e_t_idem_v1 { ETIV1 } e_t_idem_v1;
+/* { dg-final { scan-file godump-1.out "(?n)^type _e_t_idem_v1 u?int\[0-9\]*$"
} } */
+/* { dg-final { scan-file godump-1.out "(?n)^const _ETIV1 = 0$" } } */
+
+typedef enum e_t_idem_v2 e_t_idem_v2;
+enum e_t_idem_v2 { ETIV2 };
+/* { dg-final { scan-file godump-1.out "(?n)^type _e_t_idem_v2 u?int\[0-9\]*$"
} } */
+/* { dg-final { scan-file godump-1.out "(?n)^const _ETIV2 = 0$" } } */
+
enum { ETV1, ETV2 } et_v1;
/* { dg-final { scan-file godump-1.out "(?n)^var _et_v1 u?int\[0-9\]*$" } } */
/* { dg-final { scan-file godump-1.out "(?n)^const _ETV1 = 0$" } } */
@@ -477,6 +486,13 @@ struct s_fwd v_fwd;
struct s_fwd { };
/* { dg-final { scan-file godump-1.out "(?n)^type _s_fwd struct \{ \}$" } } */
+typedef struct s_t_idem_v1 {} s_t_idem_v1;
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_t_idem_v1 struct \{ \}$"
} } */
+
+typedef struct s_t_idem_v2 s_t_idem_v2;
+struct s_t_idem_v2 { };
+/* { dg-final { scan-file godump-1.out "(?n)^type _s_t_idem_v2 struct \{ \}$"
} } */
+
/*** nested structs ***/
typedef struct { struct { uint8_t ca[3]; } s; uint32_t i; } tsn;
/* { dg-final { scan-file godump-1.out "(?n)^type _tsn struct \{ s struct \{
ca \\\[2\\+1\\\]uint8; \}; i uint32; \}$" } } */
@@ -756,6 +772,13 @@ typedef union { } tue;
union { } ue;
/* { dg-final { scan-file godump-1.out "(?n)^var _ue struct \{ \}$" } } */
+typedef union u_t_idem_v1 { } u_t_idem_v1;
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_t_idem_v1 struct \{ \}$"
} } */
+
+typedef union u_t_idem_v2 u_t_idem_v2;
+union u_t_idem_v2 { };
+/* { dg-final { scan-file godump-1.out "(?n)^type _u_t_idem_v2 struct \{ \}$"
} } */
+
typedef union { uint8_t c; uint64_t l; } tu1;
/* { dg-final { scan-file godump-1.out "(?n)^type _tu1 struct \{ c uint8;
Godump_0_pad \\\[.\\\]byte; Godump_1_align \\\[0\\\]u?int64; \}$" } } */
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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
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
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
On 12/10/20 2:34 PM, Rainer Orth wrote:
I've just checked: 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_hdrmld_icmp6_hdr;
struct in6_addr mld_addr; /* multicast address */
} mld_hdr_t;
Something else is afoot here, but I'm not sure what.
Nikhil
Re: [PATCH] Correct -fdump-go-spec's handling of incomplete types
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
wrote:
>
> Hi Ian,
>
> > On Tue, Dec 8, 2020 at 2:57 PM Nikhil Benesch
> > 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 |
[PATCH] Correct -fdump-go-spec's handling of incomplete types
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 = >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" }
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
On 11/6/20 12:09 PM, Jeff Law wrote:
So I think the best path forward is to let you and Eduard-Mihai make the
technical decisions about what bits are ready for the trunk. When y'all
think something is ready, let's go ahead and get it installed and
iterate on things that aren't quite ready yet.
For bits y'all think are ready, ISTM that Eduard-Mihai should commit the
changes.
I've attached an updated version of the patch that contains some
additional unit tests that eddyb noticed I lost. From my perspective,
this is now ready for commit.
Neither eddyb nor I have write access, so someone else will need to
commit. (But please wait for eddyb to sign off too.)
It's better to get it in sooner, but there is some degree of freedom
depending on the impact of the changes. Changes in the rust demangler
aren't likely to trigger codegen or ABI breakages in the compiler itself
-- so with that in mind I think we should give this code a higher degree
of freedom to land after the stage1 close deadline.
Got it. Thanks. That's very helpful context.
Nikhil
diff --git a/libiberty/rust-demangle.c b/libiberty/rust-demangle.c
index b87365c85fe..08c615f6d8b 100644
--- a/libiberty/rust-demangle.c
+++ b/libiberty/rust-demangle.c
@@ -1,6 +1,7 @@
/* Demangler for the Rust programming language
Copyright (C) 2016-2020 Free Software Foundation, Inc.
Written by David Tolnay (dtol...@gmail.com).
+ Rewritten by Eduard-Mihai Burtescu (ed...@lyken.rs) for v0 support.
This file is part of the libiberty library.
Libiberty is free software; you can redistribute it and/or
@@ -64,11 +65,16 @@ struct rust_demangler
/* Non-zero if any error occurred. */
int errored;
+ /* Non-zero if nothing should be printed. */
+ int skipping_printing;
+
/* Non-zero if printing should be verbose (e.g. include hashes). */
int verbose;
/* Rust mangling version, with legacy mangling being -1. */
int version;
+
+ uint64_t bound_lifetime_depth;
};
/* Parsing functions. */
@@ -81,6 +87,18 @@ peek (const struct rust_demangler *rdm)
return 0;
}
+static int
+eat (struct rust_demangler *rdm, char c)
+{
+ if (peek (rdm) == c)
+{
+ rdm->next++;
+ return 1;
+}
+ else
+return 0;
+}
+
static char
next (struct rust_demangler *rdm)
{
@@ -92,11 +110,87 @@ next (struct rust_demangler *rdm)
return c;
}
+static uint64_t
+parse_integer_62 (struct rust_demangler *rdm)
+{
+ char c;
+ uint64_t x;
+
+ if (eat (rdm, '_'))
+return 0;
+
+ x = 0;
+ while (!eat (rdm, '_'))
+{
+ c = next (rdm);
+ x *= 62;
+ if (ISDIGIT (c))
+x += c - '0';
+ else if (ISLOWER (c))
+x += 10 + (c - 'a');
+ else if (ISUPPER (c))
+x += 10 + 26 + (c - 'A');
+ else
+{
+ rdm->errored = 1;
+ return 0;
+}
+}
+ return x + 1;
+}
+
+static uint64_t
+parse_opt_integer_62 (struct rust_demangler *rdm, char tag)
+{
+ if (!eat (rdm, tag))
+return 0;
+ return 1 + parse_integer_62 (rdm);
+}
+
+static uint64_t
+parse_disambiguator (struct rust_demangler *rdm)
+{
+ return parse_opt_integer_62 (rdm, 's');
+}
+
+static size_t
+parse_hex_nibbles (struct rust_demangler *rdm, uint64_t *value)
+{
+ char c;
+ size_t hex_len;
+
+ hex_len = 0;
+ *value = 0;
+
+ while (!eat (rdm, '_'))
+{
+ *value <<= 4;
+
+ c = next (rdm);
+ if (ISDIGIT (c))
+*value |= c - '0';
+ else if (c >= 'a' && c <= 'f')
+*value |= 10 + (c - 'a');
+ else
+{
+ rdm->errored = 1;
+ return 0;
+}
+ hex_len++;
+}
+
+ return hex_len;
+}
+
struct rust_mangled_ident
{
/* ASCII part of the identifier. */
const char *ascii;
size_t ascii_len;
+
+ /* Punycode insertion codes for Unicode codepoints, if any. */
+ const char *punycode;
+ size_t punycode_len;
};
static struct rust_mangled_ident
@@ -104,10 +198,16 @@ parse_ident (struct rust_demangler *rdm)
{
char c;
size_t start, len;
+ int is_punycode = 0;
struct rust_mangled_ident ident;
ident.ascii = NULL;
ident.ascii_len = 0;
+ ident.punycode = NULL;
+ ident.punycode_len = 0;
+
+ if (rdm->version != -1)
+is_punycode = eat (rdm, 'u');
c = next (rdm);
if (!ISDIGIT (c))
@@ -121,6 +221,10 @@ parse_ident (struct rust_demangler *rdm)
while (ISDIGIT (peek (rdm)))
len = len * 10 + (next (rdm) - '0');
+ /* Skip past the optional `_` separator (v0). */
+ if (rdm->version != -1)
+eat (rdm, '_');
+
start = rdm->next;
rdm->next += len;
/* Check for overflows. */
@@ -133,6 +237,27 @@ parse_ident (struct rust_demangler *rdm)
ident.ascii = rdm->sym + start;
ident.ascii_len = len;
+ if (is_punycode)
+{
+ ident.punycode_len = 0;
+ while (ident.ascii_len > 0)
+{
+ ident.ascii_len--;
+
+ /* The last '_' is a separator between ascii & punycode. */
+ if (ident.ascii[ident.ascii_len] == '_')
+break;
+
+
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
On 11/1/20 6:57 AM, Eduard-Mihai Burtescu wrote:
Reading the diff patch, the v0 changes look great. I wouldn't be too worried
about the "printable character" aspect, there are similar Unicode-related
issues elsewhere, e.g. the "non-control ASCII" comment in decode_legacy_escape
(I suppose we could make it also go through the "print a non-control ASCII
character or some escape sequence" logic you added, if you think that helps).
No, it's entirely fine with me! I just wasn't sure if the small
deviations in output were acceptable. It sounds like they are.
However, I'm not sure about the legacy changes. Or rather, the .llvm. one, it's
not really Rust-specific, it's only in the rustc-demangle crate for convenience,
but C++ code compiled with Clang could run into the same problem - ideally,
stripping that suffix could be done uniformly in cplus-dem.c, but I decided
against making that change myself, for now.
I'm especially not comfortable removing the fast path, since that was the
condition under which I was able to make Rust demangling be attempted first,
before C++, in order to implement the Rust legacy demangling standalone,
separately from C++ demangling, so that it could be together with the v0 one.
It should be possible to keep the fast path if stripping .llvm.* suffixes is
done before either Rust or C++ demangling is attempted, but even if that would
be nice to have, IMO it should be a separate patch and not block v0 demangling.
That makes sense. I've attached updated patches (again generating a diff
against both your original patch and trunk) without the changes to the
legacy code. I did preserve one small hunk regarding the unescaping of a
single '.' character in idents, as I believe that is just a
straightforward bug in the existing code.
I can test the patch and upload the dataset tomorrow, but if you want to get
something committed sooner (is there a deadline for the next release?), feel
free to land the v0 changes (snprintf + const values) without the legacy ones.
My understanding is that the GCC tree closes to new features on November
16 (for "GCC 11 Stage 3"), but I'm not sure whether that applies to
libiberty or whether this patch would be classified as a feature or a
bugfix.
I don't have commit rights (nor am I even a GCC developer). Just wanted
to tee things up for you and Ian this week. I'm very much looking
forward to the new demangling scheme and didn't want to be just another
+1 on the GitHub issue.
So certainly no time pressure from me. But perhaps someone from the GCC
side can confirm whether we are under a bit of time pressure here given
the GCC 11 release.
Cheers,
Nikhil
diff --git a/rust-demangle.c b/rust-demangle.c
index d604b3c..9cd8f99 100644
--- a/rust-demangle.c
+++ b/rust-demangle.c
@@ -143,6 +143,35 @@ parse_disambiguator (struct rust_demangler *rdm)
return parse_opt_integer_62 (rdm, 's');
}
+static size_t
+parse_hex_nibbles (struct rust_demangler *rdm, uint64_t *value)
+{
+ char c;
+ size_t hex_len;
+
+ hex_len = 0;
+ *value = 0;
+
+ while (!eat (rdm, '_'))
+{
+ *value <<= 4;
+
+ c = next (rdm);
+ if (ISDIGIT (c))
+*value |= c - '0';
+ else if (c >= 'a' && c <= 'f')
+*value |= 10 + (c - 'a');
+ else
+{
+ rdm->errored = 1;
+ return 0;
+}
+ hex_len++;
+}
+
+ return hex_len;
+}
+
struct rust_mangled_ident
{
/* ASCII part of the identifier. */
@@ -240,7 +269,7 @@ static void
print_uint64 (struct rust_demangler *rdm, uint64_t x)
{
char s[21];
- sprintf (s, "%" PRIu64, x);
+ snprintf (s, 21, "%" PRIu64, x);
PRINT (s);
}
@@ -248,7 +277,7 @@ static void
print_uint64_hex (struct rust_demangler *rdm, uint64_t x)
{
char s[17];
- sprintf (s, "%" PRIx64, x);
+ snprintf (s, 17, "%" PRIx64, x);
PRINT (s);
}
@@ -380,8 +409,7 @@ print_ident (struct rust_demangler *rdm, struct
rust_mangled_ident ident)
}
else
{
- /* "." becomes "-" */
- PRINT ("-");
+ PRINT (".");
len = 1;
}
}
@@ -591,6 +619,9 @@ static int demangle_path_maybe_open_generics (struct
rust_demangler *rdm);
static void demangle_dyn_trait (struct rust_demangler *rdm);
static void demangle_const (struct rust_demangler *rdm);
static void demangle_const_uint (struct rust_demangler *rdm);
+static void demangle_const_int (struct rust_demangler *rdm);
+static void demangle_const_bool (struct rust_demangler *rdm);
+static void demangle_const_char (struct rust_demangler *rdm);
/* Optionally enter a binder ('G') for late-bound lifetimes,
printing e.g. `for<'a, 'b> `, and make those lifetimes visible
@@ -1089,6 +1120,11 @@ demangle_const (struct rust_demangler *rdm)
ty_tag = next (rdm);
switch (ty_tag)
{
+/* Placeholder. */
+case 'p':
+ PRINT ("_");
+ return;
+
/* Unsigned integer
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
On 10/29/20 12:16 AM, Nikhil Benesch wrote:
On Wed, Oct 28, 2020 at 7:53 PM Eduard-Mihai Burtescu wrote:
I agree that landing the extra changes later on top should be fine anyway,
though when I make the sprintf -> snprintf change, I could provide the extra
changes as well (either as a combined patch, or as a second tiny patch).
Sadly I don't think I can get to either until next week, hope that's okay.
I can make the sprintf -> snprintf change as early as tomorrow.
I suspect I can also make the substantive const generics change, based
on the Rust implementation, though that's less of a promise.
Attached is an updated patch with both of the aforementioned changes.
The demangling of constants matches the demangling in rustc-demangle
library as best as I could manage. The strategy of demangling constant
chars via `format!("{:?}", char)` in Rust makes life hard for us in C,
because there is no easy way to replicate Rust's debug formatting for
chars. (Unless GCC has a Unicode library handy somewhere.)
In the course of this work I noticed several inconsistencies in how
rustc-demangle and libiberty were demangling some legacy Rust symbols. I
fixed those and added test cases, though the fixes required removing
some of the fast checks for whether a given symbol is a valid Rust symbol.
For ease of review, eddyb, I also attached the diff from your last diff
to mine. Since I don't have your collection of symbols I generated my
own by running nm on the materialized binary from
https://github.com/MaterializeInc/materialize.
Let me know how I can help. I'm happy to address review feedback myself,
or I'm happy to hand things back over to you, eddyb.
Nikhil
diff --git a/rust-demangle.c b/rust-demangle.c
index d604b3c..5c0a917 100644
--- a/rust-demangle.c
+++ b/rust-demangle.c
@@ -143,6 +143,35 @@ parse_disambiguator (struct rust_demangler *rdm)
return parse_opt_integer_62 (rdm, 's');
}
+static size_t
+parse_hex_nibbles (struct rust_demangler *rdm, uint64_t *value)
+{
+ char c;
+ size_t hex_len;
+
+ hex_len = 0;
+ *value = 0;
+
+ while (!eat (rdm, '_'))
+{
+ *value <<= 4;
+
+ c = next (rdm);
+ if (ISDIGIT (c))
+*value |= c - '0';
+ else if (c >= 'a' && c <= 'f')
+*value |= 10 + (c - 'a');
+ else
+{
+ rdm->errored = 1;
+ return 0;
+}
+ hex_len++;
+}
+
+ return hex_len;
+}
+
struct rust_mangled_ident
{
/* ASCII part of the identifier. */
@@ -240,7 +269,7 @@ static void
print_uint64 (struct rust_demangler *rdm, uint64_t x)
{
char s[21];
- sprintf (s, "%" PRIu64, x);
+ snprintf (s, 21, "%" PRIu64, x);
PRINT (s);
}
@@ -248,7 +277,7 @@ static void
print_uint64_hex (struct rust_demangler *rdm, uint64_t x)
{
char s[17];
- sprintf (s, "%" PRIx64, x);
+ snprintf (s, 17, "%" PRIx64, x);
PRINT (s);
}
@@ -380,8 +409,7 @@ print_ident (struct rust_demangler *rdm, struct
rust_mangled_ident ident)
}
else
{
- /* "." becomes "-" */
- PRINT ("-");
+ PRINT (".");
len = 1;
}
}
@@ -591,6 +619,9 @@ static int demangle_path_maybe_open_generics (struct
rust_demangler *rdm);
static void demangle_dyn_trait (struct rust_demangler *rdm);
static void demangle_const (struct rust_demangler *rdm);
static void demangle_const_uint (struct rust_demangler *rdm);
+static void demangle_const_int (struct rust_demangler *rdm);
+static void demangle_const_bool (struct rust_demangler *rdm);
+static void demangle_const_char (struct rust_demangler *rdm);
/* Optionally enter a binder ('G') for late-bound lifetimes,
printing e.g. `for<'a, 'b> `, and make those lifetimes visible
@@ -1089,6 +1120,11 @@ demangle_const (struct rust_demangler *rdm)
ty_tag = next (rdm);
switch (ty_tag)
{
+/* Placeholder. */
+case 'p':
+ PRINT ("_");
+ return;
+
/* Unsigned integer types. */
case 'h':
case 't':
@@ -1096,6 +1132,27 @@ demangle_const (struct rust_demangler *rdm)
case 'y':
case 'o':
case 'j':
+ demangle_const_uint (rdm);
+ break;
+
+/* Signed integer types. */
+case 'a':
+case 's':
+case 'l':
+case 'x':
+case 'n':
+case 'i':
+ demangle_const_int (rdm);
+ break;
+
+/* Boolean. */
+case 'b':
+ demangle_const_bool (rdm);
+ break;
+
+/* Character. */
+case 'c':
+ demangle_const_char (rdm);
break;
default:
@@ -1103,10 +1160,8 @@ demangle_const (struct rust_demangler *rdm)
return;
}
- if (eat (rdm, 'p'))
-PRINT ("_");
- else
-demangle_const_uint (rdm);
+ if (rdm->errored)
+return;
if (rdm->verbose)
{
@@ -1118,74 +1173,121 @@ demangle_const (struct rust_demangler *rdm)
static void
demangle_const_uint (struct rust_demangler *rdm)
{
- char c;
size_t hex_len;
uint64_t value;
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
On Wed, Oct 28, 2020 at 7:53 PM Eduard-Mihai Burtescu wrote: > I agree that landing the extra changes later on top should be fine anyway, > though when I make the sprintf -> snprintf change, I could provide the extra > changes as well (either as a combined patch, or as a second tiny patch). > > Sadly I don't think I can get to either until next week, hope that's okay. I can make the sprintf -> snprintf change as early as tomorrow. I suspect I can also make the substantive const generics change, based on the Rust implementation, though that's less of a promise. > All that data is around 1GB but maybe I should try to upload it somewhere > public so that anyone can repeat this procedure, or I could even redo the > collection process (in order to gather even more / newer symbol names). Sharing that file would be very appreciated. I imagine sharing it publicly would be best, but if finding hosting is difficult, you could upload directly to my Dropbox: https://www.dropbox.com/request/PdJDiYQHs9poezwIHFV9 Nikhil
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
I think it is mostly a matter of snagging some of Ian's limited time. I suspect it is still worthwhile to try to get the original patch reviewed and merged, because then any follow-up changes for const generics support will be smaller and easier to review. On Wed, Oct 28, 2020 at 5:48 PM Eduard-Mihai Burtescu wrote: > > FWIW, the patch has become slightly outdated compared to the Rust upstream, > so if someone wants to review it I should prepare a new version. > > The changes would be for the MVP version of "const generics" (Rust's > equivalent to C++ templates with value parameters, enabling e.g. types like > `CustomArray`), which supports a few different kinds of primitive > values, whereas the original patch only handled unsigned integers. > > Thanks, > - Eddy B. > > On Wed, Oct 28, 2020, at 23:25, Nikhil Benesch wrote: > > On 10/28/20 5:22 PM, Nikhil Benesch wrote: > > > Ian, are you able to review this? I saw that you reviewed many of the > > > prior changes to the Rust demangler. > > > > > > If not, can you suggest someone who can? > > > > > > Thanks very much. > > > Nikhil > > > > I seem to have failed to convince my email client to set the appropriate > > reply to headers. This is the patch to which I was referring: > > > > https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542488.html > >
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
On 10/28/20 5:22 PM, Nikhil Benesch wrote: Ian, are you able to review this? I saw that you reviewed many of the prior changes to the Rust demangler. If not, can you suggest someone who can? Thanks very much. Nikhil I seem to have failed to convince my email client to set the appropriate reply to headers. This is the patch to which I was referring: https://gcc.gnu.org/pipermail/gcc-patches/2020-March/542488.html
Re: [PATCH] Support the new ("v0") mangling scheme in rust-demangle.
Ian, are you able to review this? I saw that you reviewed many of the prior changes to the Rust demangler. If not, can you suggest someone who can? Thanks very much. Nikhil
