Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
On Tue, Feb 5, 2019 at 10:46 AM Nikhil Benesch wrote: > > Ian—is there anything preventing this from getting merged? (I don't have > write access.) Thanks, committed now. Ian > On Thu, Jan 24, 2019 at 11:31 AM Nikhil Benesch > wrote: > > > On Thu, Jan 24, 2019 at 10:15 AM Richard Biener > > wrote: > > > > > > Ah, I missed that pt is probably a pointer type as well, then the code > > > simply aligns the pointed-to type (note dependent on how pt was built > > > this seems prone to wreck the TYPE_POINTER_TO/TYPE_NEXT_PTR_TO > > > chains which link pointer types to a type. > > > > Right. In fact, set_placeholder_pointer_type asserts that both pt and tt > > are pointer types. > > > > It's true that, after a call to set_placeholder_pointer_type, pt becomes > > a distinct pointer type to T, yet is not part of the > > TYPE_POINTER_TO/TYPE_NEXT_PTR_TO chain for T. As far as I can tell, > > that's fine. The chain for T remains intact, as placeholder pointer > > types are careful to always point to a distinct dummy object (see the > > Gcc_backend::placeholder_pointer_type method) that nothing cares about. > > The only consequence I see is the increased memory usage of having > > multiple distinct but semantically identical pointer types to T. > >
Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
Ian—is there anything preventing this from getting merged? (I don't have write access.) On Thu, Jan 24, 2019 at 11:31 AM Nikhil Benesch wrote: > On Thu, Jan 24, 2019 at 10:15 AM Richard Biener > wrote: > > > > Ah, I missed that pt is probably a pointer type as well, then the code > > simply aligns the pointed-to type (note dependent on how pt was built > > this seems prone to wreck the TYPE_POINTER_TO/TYPE_NEXT_PTR_TO > > chains which link pointer types to a type. > > Right. In fact, set_placeholder_pointer_type asserts that both pt and tt > are pointer types. > > It's true that, after a call to set_placeholder_pointer_type, pt becomes > a distinct pointer type to T, yet is not part of the > TYPE_POINTER_TO/TYPE_NEXT_PTR_TO chain for T. As far as I can tell, > that's fine. The chain for T remains intact, as placeholder pointer > types are careful to always point to a distinct dummy object (see the > Gcc_backend::placeholder_pointer_type method) that nothing cares about. > The only consequence I see is the increased memory usage of having > multiple distinct but semantically identical pointer types to T. >
Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
On Thu, Jan 24, 2019 at 10:15 AM Richard Biener wrote: > > Ah, I missed that pt is probably a pointer type as well, then the code > simply aligns the pointed-to type (note dependent on how pt was built > this seems prone to wreck the TYPE_POINTER_TO/TYPE_NEXT_PTR_TO > chains which link pointer types to a type. Right. In fact, set_placeholder_pointer_type asserts that both pt and tt are pointer types. It's true that, after a call to set_placeholder_pointer_type, pt becomes a distinct pointer type to T, yet is not part of the TYPE_POINTER_TO/TYPE_NEXT_PTR_TO chain for T. As far as I can tell, that's fine. The chain for T remains intact, as placeholder pointer types are careful to always point to a distinct dummy object (see the Gcc_backend::placeholder_pointer_type method) that nothing cares about. The only consequence I see is the increased memory usage of having multiple distinct but semantically identical pointer types to T.
Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
On Thu, Jan 24, 2019 at 3:59 PM Nikhil Benesch wrote: > > On Thu, Jan 24, 2019 at 6:01 AM Richard Biener > wrote: > > > > On Thu, Jan 24, 2019 at 2:18 AM Nikhil Benesch > > wrote: > > > > > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > > > index 7fbdd074119..4e9e0e3026a 100644 > > > --- a/gcc/go/go-gcc.cc > > > +++ b/gcc/go/go-gcc.cc > > > @@ -1049,6 +1049,7 @@ Gcc_backend::set_placeholder_pointer_type(Btype* > > > placeholder, > > > } > > >gcc_assert(TREE_CODE(tt) == POINTER_TYPE); > > >TREE_TYPE(pt) = TREE_TYPE(tt); > > > + TYPE_CANONICAL(pt) = TYPE_CANONICAL(tt); > > > > This doesn't make sense - it says to the middle-end that > > T* is equal to T? > > Not quite. When this function is called, pt is a placeholder of type U*, > where U is some unknown type, and tt has type T*, where T is a complete > type. The goal is to make pt look exactly like tt, i.e., fill in the > placeholder. Ah, I missed that pt is probably a pointer type as well, then the code simply aligns the pointed-to type (note dependent on how pt was built this seems prone to wreck the TYPE_POINTER_TO/TYPE_NEXT_PTR_TO chains which link pointer types to a type. > I realize now that my changelog entry isn't quite precise about this. > Perhaps the second entry should instead read "Propagate the canonical > type from the desired pointer type to the placeholder pointer type".
Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
On Thu, Jan 24, 2019 at 6:01 AM Richard Biener wrote: > > On Thu, Jan 24, 2019 at 2:18 AM Nikhil Benesch > wrote: > > > > diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc > > index 7fbdd074119..4e9e0e3026a 100644 > > --- a/gcc/go/go-gcc.cc > > +++ b/gcc/go/go-gcc.cc > > @@ -1049,6 +1049,7 @@ Gcc_backend::set_placeholder_pointer_type(Btype* > > placeholder, > > } > >gcc_assert(TREE_CODE(tt) == POINTER_TYPE); > >TREE_TYPE(pt) = TREE_TYPE(tt); > > + TYPE_CANONICAL(pt) = TYPE_CANONICAL(tt); > > This doesn't make sense - it says to the middle-end that > T* is equal to T? Not quite. When this function is called, pt is a placeholder of type U*, where U is some unknown type, and tt has type T*, where T is a complete type. The goal is to make pt look exactly like tt, i.e., fill in the placeholder. I realize now that my changelog entry isn't quite precise about this. Perhaps the second entry should instead read "Propagate the canonical type from the desired pointer type to the placeholder pointer type".
Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
On Thu, Jan 24, 2019 at 2:18 AM Nikhil Benesch wrote:
>
> This patch fixes an ICE I reported earlier today as PR go/89019, which
> occurs when compiling sufficiently complicated Go code with link-time
> optimization (i.e., -flto) enabled.
>
> Both of these simple test programs are sufficient to trigger the ICE:
>
> $ cat crash1.go
> package main
>
> type fcanary func()
>
> $ cat crash2.go
> package main
>
> func f() {
> var canary func()
> func() {
> canary()
> }()
> }
>
> The cause appears to be that the propagation of structural equality
> requirements is mishandled during the handoff from the Go frontend to
> the middle-end. Pointers to types that required structural equality
> were not always marked as requiring structural equality themselves.
> Remarkably, the only code that notices the mistake is the free_lang_data
> IPA pass, which is only active when LTO is enabled.
>
> The enclosed patch fixes the issue and provides test coverage by adding
> a subtest to the Go torture test suite that compiles with -flto.
> Bootstrapped and ran Go test suite on x86_64-pc-linux-gnu.
>
> 2018-01-23 Nikhil Benesch
>
> PR go/89019
> * go-gcc.cc (Gcc_backend::placeholder_struct_type): Mark
> placeholder structs as requiring structural equality.
> (Gcc_backend::set_placeholder_pointer_type): Propagate
> the canonical type from the referenced type to the pointer type.
> * lib/go-torture.exp: Test compiling with -flto.
>
> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
> index 7fbdd074119..4e9e0e3026a 100644
> --- a/gcc/go/go-gcc.cc
> +++ b/gcc/go/go-gcc.cc
> @@ -1049,6 +1049,7 @@ Gcc_backend::set_placeholder_pointer_type(Btype*
> placeholder,
> }
>gcc_assert(TREE_CODE(tt) == POINTER_TYPE);
>TREE_TYPE(pt) = TREE_TYPE(tt);
> + TYPE_CANONICAL(pt) = TYPE_CANONICAL(tt);
This doesn't make sense - it says to the middle-end that
T* is equal to T?
>if (TYPE_NAME(pt) != NULL_TREE)
> {
>// Build the data structure gcc wants to see for a typedef.
> @@ -1080,6 +1081,12 @@ Gcc_backend::placeholder_struct_type(const
> std::string& name,
> get_identifier_from_string(name),
> ret);
>TYPE_NAME(ret) = decl;
> +
> + // The struct type that eventually replaces this placeholder will
> require
> + // structural equality. The placeholder must too, so that the
> requirement
> + // for structural equality propagates to references that are
> constructed
> + // before the replacement occurs.
> + SET_TYPE_STRUCTURAL_EQUALITY(ret);
> }
>return this->make_type(ret);
> }
> diff --git a/gcc/testsuite/lib/go-torture.exp
> b/gcc/testsuite/lib/go-torture.exp
> index 213711e41df..a7eca184416 100644
> --- a/gcc/testsuite/lib/go-torture.exp
> +++ b/gcc/testsuite/lib/go-torture.exp
> @@ -34,7 +34,8 @@ if ![info exists TORTURE_OPTIONS] {
> { -O2 -fomit-frame-pointer -finline-functions -funroll-loops } \
> { -O2 -fbounds-check } \
> { -O3 -g } \
> - { -Os }]
> + { -Os } \
> + { -flto }]
> }
>
>
>
Re: [PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
On Wed, Jan 23, 2019 at 5:18 PM Nikhil Benesch wrote: > > 2018-01-23 Nikhil Benesch > > PR go/89019 > * go-gcc.cc (Gcc_backend::placeholder_struct_type): Mark > placeholder structs as requiring structural equality. > (Gcc_backend::set_placeholder_pointer_type): Propagate > the canonical type from the referenced type to the pointer type. > * lib/go-torture.exp: Test compiling with -flto. This is OK. Thanks. Ian
[PATCH] Fix an ICE when compiling Go with LTO (PR go/89019)
This patch fixes an ICE I reported earlier today as PR go/89019, which
occurs when compiling sufficiently complicated Go code with link-time
optimization (i.e., -flto) enabled.
Both of these simple test programs are sufficient to trigger the ICE:
$ cat crash1.go
package main
type fcanary func()
$ cat crash2.go
package main
func f() {
var canary func()
func() {
canary()
}()
}
The cause appears to be that the propagation of structural equality
requirements is mishandled during the handoff from the Go frontend to
the middle-end. Pointers to types that required structural equality
were not always marked as requiring structural equality themselves.
Remarkably, the only code that notices the mistake is the free_lang_data
IPA pass, which is only active when LTO is enabled.
The enclosed patch fixes the issue and provides test coverage by adding
a subtest to the Go torture test suite that compiles with -flto.
Bootstrapped and ran Go test suite on x86_64-pc-linux-gnu.
2018-01-23 Nikhil Benesch
PR go/89019
* go-gcc.cc (Gcc_backend::placeholder_struct_type): Mark
placeholder structs as requiring structural equality.
(Gcc_backend::set_placeholder_pointer_type): Propagate
the canonical type from the referenced type to the pointer type.
* lib/go-torture.exp: Test compiling with -flto.
diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
index 7fbdd074119..4e9e0e3026a 100644
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -1049,6 +1049,7 @@ Gcc_backend::set_placeholder_pointer_type(Btype*
placeholder,
}
gcc_assert(TREE_CODE(tt) == POINTER_TYPE);
TREE_TYPE(pt) = TREE_TYPE(tt);
+ TYPE_CANONICAL(pt) = TYPE_CANONICAL(tt);
if (TYPE_NAME(pt) != NULL_TREE)
{
// Build the data structure gcc wants to see for a typedef.
@@ -1080,6 +1081,12 @@ Gcc_backend::placeholder_struct_type(const std::string&
name,
get_identifier_from_string(name),
ret);
TYPE_NAME(ret) = decl;
+
+ // The struct type that eventually replaces this placeholder will require
+ // structural equality. The placeholder must too, so that the requirement
+ // for structural equality propagates to references that are constructed
+ // before the replacement occurs.
+ SET_TYPE_STRUCTURAL_EQUALITY(ret);
}
return this->make_type(ret);
}
diff --git a/gcc/testsuite/lib/go-torture.exp b/gcc/testsuite/lib/go-torture.exp
index 213711e41df..a7eca184416 100644
--- a/gcc/testsuite/lib/go-torture.exp
+++ b/gcc/testsuite/lib/go-torture.exp
@@ -34,7 +34,8 @@ if ![info exists TORTURE_OPTIONS] {
{ -O2 -fomit-frame-pointer -finline-functions -funroll-loops } \
{ -O2 -fbounds-check } \
{ -O3 -g } \
- { -Os }]
+ { -Os } \
+ { -flto }]
}
