* Go PATCH to fix libgo breakage (PR tree-optimization/67284)
@ 2015-08-24 11:39 Marek Polacek
2015-08-24 15:22 ` Ian Lance Taylor
0 siblings, 1 reply; 4+ messages in thread
From: Marek Polacek @ 2015-08-24 11:39 UTC (permalink / raw)
To: GCC Patches, Ian Lance Taylor; +Cc: Jakub Jelinek, Jeff Law, Richard Biener
This is hopefully the last attempt to fix the libgo breakage. This time
around I really think I got to the bottom of the problem. The issue is
that the Go FE defines the __builtin_trap call, but doesn't set the
TREE_THIS_VOLATILE flag on it. That's bad because then this call isn't
gimple_call_noreturn_p and the cgraph cleanup code can't do its job properly.
Bootstrapped/regtested on x86_64-linux. Now, I don't know the Go patch
process here. Should I just wait for you Ian to commit the patch?
(Yeah, I think I don't need the ChangeLog entry.)
2015-08-24 Marek Polacek <polacek@redhat.com>
PR tree-optimization/67284
* go-gcc.cc (Gcc_backend::define_builtin): Add NORETURN_P default
argument. Set TREE_THIS_VOLATILE.
(Gcc_backend::Gcc_backend): Mark __builtin_trap as a noreturn call.
diff --git gcc/go/go-gcc.cc gcc/go/go-gcc.cc
index 6f274fc..aabcd98 100644
--- gcc/go/go-gcc.cc
+++ gcc/go/go-gcc.cc
@@ -498,7 +498,7 @@ class Gcc_backend : public Backend
private:
void
define_builtin(built_in_function bcode, const char* name, const char* libname,
- tree fntype, bool const_p);
+ tree fntype, bool const_p, bool noreturn_p = false);
// A mapping of the GCC built-ins exposed to GCCGo.
std::map<std::string, Bfunction*> builtin_functions_;
@@ -675,7 +675,7 @@ Gcc_backend::Gcc_backend()
// cases.
this->define_builtin(BUILT_IN_TRAP, "__builtin_trap", NULL,
build_function_type(void_type_node, void_list_node),
- false);
+ false, true);
}
// Get an unnamed integer type.
@@ -3095,15 +3095,19 @@ Gcc_backend::write_global_definitions(
// LIBNAME is the name of the corresponding library function, and is
// NULL if there isn't one. FNTYPE is the type of the function.
// CONST_P is true if the function has the const attribute.
+// NORETURN_P is true if the function has the noreturn attribute.
void
Gcc_backend::define_builtin(built_in_function bcode, const char* name,
- const char* libname, tree fntype, bool const_p)
+ const char* libname, tree fntype, bool const_p,
+ bool noreturn_p)
{
tree decl = add_builtin_function(name, fntype, bcode, BUILT_IN_NORMAL,
libname, NULL_TREE);
if (const_p)
TREE_READONLY(decl) = 1;
+ if (noreturn_p)
+ TREE_THIS_VOLATILE(decl) = 1;
set_builtin_decl(bcode, decl, true);
this->builtin_functions_[name] = this->make_function(decl);
if (libname != NULL)
@@ -3112,6 +3116,8 @@ Gcc_backend::define_builtin(built_in_function bcode, const char* name,
NULL, NULL_TREE);
if (const_p)
TREE_READONLY(decl) = 1;
+ if (noreturn_p)
+ TREE_THIS_VOLATILE(decl) = 1;
this->builtin_functions_[libname] = this->make_function(decl);
}
}
Marek
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Go PATCH to fix libgo breakage (PR tree-optimization/67284)
2015-08-24 11:39 Go PATCH to fix libgo breakage (PR tree-optimization/67284) Marek Polacek
@ 2015-08-24 15:22 ` Ian Lance Taylor
2015-08-24 16:07 ` Marek Polacek
0 siblings, 1 reply; 4+ messages in thread
From: Ian Lance Taylor @ 2015-08-24 15:22 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jakub Jelinek, Jeff Law, Richard Biener
On Mon, Aug 24, 2015 at 4:34 AM, Marek Polacek <polacek@redhat.com> wrote:
>
> This is hopefully the last attempt to fix the libgo breakage. This time
> around I really think I got to the bottom of the problem. The issue is
> that the Go FE defines the __builtin_trap call, but doesn't set the
> TREE_THIS_VOLATILE flag on it. That's bad because then this call isn't
> gimple_call_noreturn_p and the cgraph cleanup code can't do its job properly.
>
> Bootstrapped/regtested on x86_64-linux. Now, I don't know the Go patch
> process here. Should I just wait for you Ian to commit the patch?
> (Yeah, I think I don't need the ChangeLog entry.)
For files in gcc/go/gofrontend, you should wait for me to commit the
path. This file is in gcc/go, not gcc/go/gofrontend, so you can
commit the patch directly, with a ChangeLog entry. (I know it's
confusing.)
> 2015-08-24 Marek Polacek <polacek@redhat.com>
>
> PR tree-optimization/67284
> * go-gcc.cc (Gcc_backend::define_builtin): Add NORETURN_P default
> argument. Set TREE_THIS_VOLATILE.
> (Gcc_backend::Gcc_backend): Mark __builtin_trap as a noreturn call.
>
> diff --git gcc/go/go-gcc.cc gcc/go/go-gcc.cc
> index 6f274fc..aabcd98 100644
> --- gcc/go/go-gcc.cc
> +++ gcc/go/go-gcc.cc
> @@ -498,7 +498,7 @@ class Gcc_backend : public Backend
> private:
> void
> define_builtin(built_in_function bcode, const char* name, const char* libname,
> - tree fntype, bool const_p);
> + tree fntype, bool const_p, bool noreturn_p = false);
I would rather not see a default value for this parameter. I would
rather see all the calls change to pass false. As the GCC coding
conventions say, one should normally avoid default arguments
(https://gcc.gnu.org/codingconventions.html#Default).
This patch is OK with that change.
Thanks.
Ian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Go PATCH to fix libgo breakage (PR tree-optimization/67284)
2015-08-24 15:22 ` Ian Lance Taylor
@ 2015-08-24 16:07 ` Marek Polacek
2015-08-24 21:42 ` Ian Lance Taylor
0 siblings, 1 reply; 4+ messages in thread
From: Marek Polacek @ 2015-08-24 16:07 UTC (permalink / raw)
To: Ian Lance Taylor; +Cc: GCC Patches, Jakub Jelinek, Jeff Law, Richard Biener
On Mon, Aug 24, 2015 at 08:18:41AM -0700, Ian Lance Taylor wrote:
> On Mon, Aug 24, 2015 at 4:34 AM, Marek Polacek <polacek@redhat.com> wrote:
> >
> > This is hopefully the last attempt to fix the libgo breakage. This time
> > around I really think I got to the bottom of the problem. The issue is
> > that the Go FE defines the __builtin_trap call, but doesn't set the
> > TREE_THIS_VOLATILE flag on it. That's bad because then this call isn't
> > gimple_call_noreturn_p and the cgraph cleanup code can't do its job properly.
> >
> > Bootstrapped/regtested on x86_64-linux. Now, I don't know the Go patch
> > process here. Should I just wait for you Ian to commit the patch?
> > (Yeah, I think I don't need the ChangeLog entry.)
>
> For files in gcc/go/gofrontend, you should wait for me to commit the
> path. This file is in gcc/go, not gcc/go/gofrontend, so you can
> commit the patch directly, with a ChangeLog entry. (I know it's
> confusing.)
I see, thanks.
> I would rather not see a default value for this parameter. I would
> rather see all the calls change to pass false. As the GCC coding
> conventions say, one should normally avoid default arguments
> (https://gcc.gnu.org/codingconventions.html#Default).
Ok, adjusted.
> This patch is OK with that change.
Thanks and sorry for the breakage.
Bootstrapped/regtested on x86_64-linux, applying to trunk.
2015-08-24 Marek Polacek <polacek@redhat.com>
PR tree-optimization/67284
* go-gcc.cc (Gcc_backend::define_builtin): Add NORETURN_P parameter.
Set TREE_THIS_VOLATILE.
(Gcc_backend::Gcc_backend): Mark __builtin_trap as a noreturn call.
Pass false to the rest of define_builtin calls.
diff --git gcc/go/go-gcc.cc gcc/go/go-gcc.cc
index 6f274fc..cb4c2e5 100644
--- gcc/go/go-gcc.cc
+++ gcc/go/go-gcc.cc
@@ -498,7 +498,7 @@ class Gcc_backend : public Backend
private:
void
define_builtin(built_in_function bcode, const char* name, const char* libname,
- tree fntype, bool const_p);
+ tree fntype, bool const_p, bool noreturn_p);
// A mapping of the GCC built-ins exposed to GCCGo.
std::map<std::string, Bfunction*> builtin_functions_;
@@ -522,25 +522,25 @@ Gcc_backend::Gcc_backend()
tree p = build_pointer_type(build_qualified_type(t, TYPE_QUAL_VOLATILE));
this->define_builtin(BUILT_IN_SYNC_ADD_AND_FETCH_1, "__sync_fetch_and_add_1",
NULL, build_function_type_list(t, p, t, NULL_TREE),
- false);
+ false, false);
t = this->integer_type(BITS_PER_UNIT * 2, 1)->get_tree();
p = build_pointer_type(build_qualified_type(t, TYPE_QUAL_VOLATILE));
this->define_builtin(BUILT_IN_SYNC_ADD_AND_FETCH_2, "__sync_fetch_and_add_2",
NULL, build_function_type_list(t, p, t, NULL_TREE),
- false);
+ false, false);
t = this->integer_type(BITS_PER_UNIT * 4, 1)->get_tree();
p = build_pointer_type(build_qualified_type(t, TYPE_QUAL_VOLATILE));
this->define_builtin(BUILT_IN_SYNC_ADD_AND_FETCH_4, "__sync_fetch_and_add_4",
NULL, build_function_type_list(t, p, t, NULL_TREE),
- false);
+ false, false);
t = this->integer_type(BITS_PER_UNIT * 8, 1)->get_tree();
p = build_pointer_type(build_qualified_type(t, TYPE_QUAL_VOLATILE));
this->define_builtin(BUILT_IN_SYNC_ADD_AND_FETCH_8, "__sync_fetch_and_add_8",
NULL, build_function_type_list(t, p, t, NULL_TREE),
- false);
+ false, false);
// We use __builtin_expect for magic import functions.
this->define_builtin(BUILT_IN_EXPECT, "__builtin_expect", NULL,
@@ -548,7 +548,7 @@ Gcc_backend::Gcc_backend()
long_integer_type_node,
long_integer_type_node,
NULL_TREE),
- true);
+ true, false);
// We use __builtin_memcmp for struct comparisons.
this->define_builtin(BUILT_IN_MEMCMP, "__builtin_memcmp", "memcmp",
@@ -557,7 +557,7 @@ Gcc_backend::Gcc_backend()
const_ptr_type_node,
size_type_node,
NULL_TREE),
- false);
+ false, false);
// We provide some functions for the math library.
tree math_function_type = build_function_type_list(double_type_node,
@@ -574,93 +574,93 @@ Gcc_backend::Gcc_backend()
build_function_type_list(long_double_type_node, long_double_type_node,
long_double_type_node, NULL_TREE);
this->define_builtin(BUILT_IN_ACOS, "__builtin_acos", "acos",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_ACOSL, "__builtin_acosl", "acosl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_ASIN, "__builtin_asin", "asin",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_ASINL, "__builtin_asinl", "asinl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_ATAN, "__builtin_atan", "atan",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_ATANL, "__builtin_atanl", "atanl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_ATAN2, "__builtin_atan2", "atan2",
- math_function_type_two, true);
+ math_function_type_two, true, false);
this->define_builtin(BUILT_IN_ATAN2L, "__builtin_atan2l", "atan2l",
- math_function_type_long_two, true);
+ math_function_type_long_two, true, false);
this->define_builtin(BUILT_IN_CEIL, "__builtin_ceil", "ceil",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_CEILL, "__builtin_ceill", "ceill",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_COS, "__builtin_cos", "cos",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_COSL, "__builtin_cosl", "cosl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_EXP, "__builtin_exp", "exp",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_EXPL, "__builtin_expl", "expl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_EXPM1, "__builtin_expm1", "expm1",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_EXPM1L, "__builtin_expm1l", "expm1l",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_FABS, "__builtin_fabs", "fabs",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_FABSL, "__builtin_fabsl", "fabsl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_FLOOR, "__builtin_floor", "floor",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_FLOORL, "__builtin_floorl", "floorl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_FMOD, "__builtin_fmod", "fmod",
- math_function_type_two, true);
+ math_function_type_two, true, false);
this->define_builtin(BUILT_IN_FMODL, "__builtin_fmodl", "fmodl",
- math_function_type_long_two, true);
+ math_function_type_long_two, true, false);
this->define_builtin(BUILT_IN_LDEXP, "__builtin_ldexp", "ldexp",
build_function_type_list(double_type_node,
double_type_node,
integer_type_node,
NULL_TREE),
- true);
+ true, false);
this->define_builtin(BUILT_IN_LDEXPL, "__builtin_ldexpl", "ldexpl",
build_function_type_list(long_double_type_node,
long_double_type_node,
integer_type_node,
NULL_TREE),
- true);
+ true, false);
this->define_builtin(BUILT_IN_LOG, "__builtin_log", "log",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_LOGL, "__builtin_logl", "logl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_LOG1P, "__builtin_log1p", "log1p",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_LOG1PL, "__builtin_log1pl", "log1pl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_LOG10, "__builtin_log10", "log10",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_LOG10L, "__builtin_log10l", "log10l",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_LOG2, "__builtin_log2", "log2",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_LOG2L, "__builtin_log2l", "log2l",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_SIN, "__builtin_sin", "sin",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_SINL, "__builtin_sinl", "sinl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_SQRT, "__builtin_sqrt", "sqrt",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_SQRTL, "__builtin_sqrtl", "sqrtl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_TAN, "__builtin_tan", "tan",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_TANL, "__builtin_tanl", "tanl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
this->define_builtin(BUILT_IN_TRUNC, "__builtin_trunc", "trunc",
- math_function_type, true);
+ math_function_type, true, false);
this->define_builtin(BUILT_IN_TRUNCL, "__builtin_truncl", "truncl",
- math_function_type_long, true);
+ math_function_type_long, true, false);
// We use __builtin_return_address in the thunk we build for
// functions which call recover.
@@ -669,13 +669,13 @@ Gcc_backend::Gcc_backend()
build_function_type_list(ptr_type_node,
unsigned_type_node,
NULL_TREE),
- false);
+ false, false);
// The compiler uses __builtin_trap for some exception handling
// cases.
this->define_builtin(BUILT_IN_TRAP, "__builtin_trap", NULL,
build_function_type(void_type_node, void_list_node),
- false);
+ false, true);
}
// Get an unnamed integer type.
@@ -3095,15 +3095,19 @@ Gcc_backend::write_global_definitions(
// LIBNAME is the name of the corresponding library function, and is
// NULL if there isn't one. FNTYPE is the type of the function.
// CONST_P is true if the function has the const attribute.
+// NORETURN_P is true if the function has the noreturn attribute.
void
Gcc_backend::define_builtin(built_in_function bcode, const char* name,
- const char* libname, tree fntype, bool const_p)
+ const char* libname, tree fntype, bool const_p,
+ bool noreturn_p)
{
tree decl = add_builtin_function(name, fntype, bcode, BUILT_IN_NORMAL,
libname, NULL_TREE);
if (const_p)
TREE_READONLY(decl) = 1;
+ if (noreturn_p)
+ TREE_THIS_VOLATILE(decl) = 1;
set_builtin_decl(bcode, decl, true);
this->builtin_functions_[name] = this->make_function(decl);
if (libname != NULL)
@@ -3112,6 +3116,8 @@ Gcc_backend::define_builtin(built_in_function bcode, const char* name,
NULL, NULL_TREE);
if (const_p)
TREE_READONLY(decl) = 1;
+ if (noreturn_p)
+ TREE_THIS_VOLATILE(decl) = 1;
this->builtin_functions_[libname] = this->make_function(decl);
}
}
Marek
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Go PATCH to fix libgo breakage (PR tree-optimization/67284)
2015-08-24 16:07 ` Marek Polacek
@ 2015-08-24 21:42 ` Ian Lance Taylor
0 siblings, 0 replies; 4+ messages in thread
From: Ian Lance Taylor @ 2015-08-24 21:42 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jakub Jelinek, Jeff Law, Richard Biener
On Mon, Aug 24, 2015 at 9:04 AM, Marek Polacek <polacek@redhat.com> wrote:
> On Mon, Aug 24, 2015 at 08:18:41AM -0700, Ian Lance Taylor wrote:
>> On Mon, Aug 24, 2015 at 4:34 AM, Marek Polacek <polacek@redhat.com> wrote:
>> >
>> > This is hopefully the last attempt to fix the libgo breakage. This time
>> > around I really think I got to the bottom of the problem. The issue is
>> > that the Go FE defines the __builtin_trap call, but doesn't set the
>> > TREE_THIS_VOLATILE flag on it. That's bad because then this call isn't
>> > gimple_call_noreturn_p and the cgraph cleanup code can't do its job properly.
>> >
>> > Bootstrapped/regtested on x86_64-linux. Now, I don't know the Go patch
>> > process here. Should I just wait for you Ian to commit the patch?
>> > (Yeah, I think I don't need the ChangeLog entry.)
>>
>> For files in gcc/go/gofrontend, you should wait for me to commit the
>> path. This file is in gcc/go, not gcc/go/gofrontend, so you can
>> commit the patch directly, with a ChangeLog entry. (I know it's
>> confusing.)
>
> I see, thanks.
>
>> I would rather not see a default value for this parameter. I would
>> rather see all the calls change to pass false. As the GCC coding
>> conventions say, one should normally avoid default arguments
>> (https://gcc.gnu.org/codingconventions.html#Default).
>
> Ok, adjusted.
>
>> This patch is OK with that change.
>
> Thanks and sorry for the breakage.
No need to apologize, it was my bug. Thanks for finding and fixing it.
Ian
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-24 21:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-24 11:39 Go PATCH to fix libgo breakage (PR tree-optimization/67284) Marek Polacek
2015-08-24 15:22 ` Ian Lance Taylor
2015-08-24 16:07 ` Marek Polacek
2015-08-24 21:42 ` 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).