Re: [patch] Fix segfault during inlining of thunk
On Wed, Jan 23, 2019 at 10:48 AM Eric Botcazou wrote: > > Hi, > > this is a regression present in Ada on the mainline since we switched to using > the internal support for thunks: expand_thunk segfaults during inlining when > trying to access the DECL_RESULT of the special alias built to make the call > from the thunk local, if the DECL_RESULT of the thunk is DECL_BY_REFERENCE. > > It turns out that neither the C++ nor the Ada front-end builds the DECL_RESULT > of this special alias (the C++ front-end apparently doesn't even build that of > the thunk itself so expand_thunk has code to patch this up). Moreover it's a > bit strange to first try: > >restmp = gimple_fold_indirect_ref (resdecl); > > i.e. build the dereference using the type of resdecl and then use the type of > the DECL_RESULT of the alias if this failed. So the attached fix changes this > to using the type of resdecl in the latter case too. > > Bootstrapped/regtested on x86_64-suse-linux, OK for the mainline? OK. I guess the original (cut?) error was to look at DECL_RESULT (alias) instead of DECL_RESULT (thunk_fndecl) which would have made this consistent as well. Richard. > > 2019-01-22 Eric Botcazou > > * cgraphunit.c (cgraph_node::expand_thunk): When expanding a GIMPLE > thunk that returns by reference, use the type of the return object > of the thunk instead of that of the alias to build the dereference. > > > 2019-01-23 Eric Botcazou > > * gnat.dg/specs/opt4.ads: New test. > > -- > Eric Botcazou
[patch] Fix segfault during inlining of thunk
Hi,
this is a regression present in Ada on the mainline since we switched to using
the internal support for thunks: expand_thunk segfaults during inlining when
trying to access the DECL_RESULT of the special alias built to make the call
from the thunk local, if the DECL_RESULT of the thunk is DECL_BY_REFERENCE.
It turns out that neither the C++ nor the Ada front-end builds the DECL_RESULT
of this special alias (the C++ front-end apparently doesn't even build that of
the thunk itself so expand_thunk has code to patch this up). Moreover it's a
bit strange to first try:
restmp = gimple_fold_indirect_ref (resdecl);
i.e. build the dereference using the type of resdecl and then use the type of
the DECL_RESULT of the alias if this failed. So the attached fix changes this
to using the type of resdecl in the latter case too.
Bootstrapped/regtested on x86_64-suse-linux, OK for the mainline?
2019-01-22 Eric Botcazou
* cgraphunit.c (cgraph_node::expand_thunk): When expanding a GIMPLE
thunk that returns by reference, use the type of the return object
of the thunk instead of that of the alias to build the dereference.
2019-01-23 Eric Botcazou
* gnat.dg/specs/opt4.ads: New test.
--
Eric BotcazouIndex: cgraphunit.c
===
--- cgraphunit.c (revision 268071)
+++ cgraphunit.c (working copy)
@@ -1916,10 +1916,9 @@ cgraph_node::expand_thunk (bool output_a
restmp = gimple_fold_indirect_ref (resdecl);
if (!restmp)
restmp = build2 (MEM_REF,
- TREE_TYPE (TREE_TYPE (DECL_RESULT (alias))),
+ TREE_TYPE (TREE_TYPE (resdecl)),
resdecl,
- build_int_cst (TREE_TYPE
- (DECL_RESULT (alias)), 0));
+ build_int_cst (TREE_TYPE (resdecl), 0));
}
else if (!is_gimple_reg_type (restype))
{
-- { dg-do compile }
-- { dg-options "-O" }
package Opt4 is
type Rec (D : Boolean := False) is record
case D is
when False => null;
when True => I : Integer;
end case;
end record;
Null_Rec : constant Rec := (D => False);
type I1 is limited interface;
type I2 is limited interface;
function Func (Data : I2) return Rec is abstract;
type Ext is limited new I1 and I2 with null record;
overriding function Func (Data : Ext) return Rec is (Null_Rec);
end Opt4;
Re: [PATCH] Fix segfault with inlining
I've looked at the C++ testcase
int foo (int x)
{
try {
return x;
}
catch (...)
{
return 0;
}
}
which exhibits exactly the behavior you quote - return x is considered
throwing an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to
be set here (maybe due to this issue you quote?).
I presume that you compiled with -fnon-call-exceptions? Otherwise, I don't
see how something that isn't a call can throw an exception in C++, it should
be seen at most as possibly trapping, which is less blocking.
Other than that the patch looks reasonable (I suppose you need
is_parameter_of only because as we recursively handle the trees
PARM_DECLs from the destination could already have leaked into
the tree we recurse into?)
Do you mean that the test on DECL_CONTEXT is superfluous? Possibly indeed,
but with nested functions you can have PARM_DECLs of different origins in a
given function body, although this may be irrelevant for tree-inline.c.
--
Eric Botcazou
Re: [PATCH] Fix segfault with inlining
On Tue, Sep 17, 2013 at 9:03 AM, Eric Botcazou ebotca...@adacore.com wrote:
I've looked at the C++ testcase
int foo (int x)
{
try {
return x;
}
catch (...)
{
return 0;
}
}
which exhibits exactly the behavior you quote - return x is considered
throwing an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to
be set here (maybe due to this issue you quote?).
I presume that you compiled with -fnon-call-exceptions? Otherwise, I don't
see how something that isn't a call can throw an exception in C++, it should
be seen at most as possibly trapping, which is less blocking.
Yes, with -fnon-call-exceptions.
Other than that the patch looks reasonable (I suppose you need
is_parameter_of only because as we recursively handle the trees
PARM_DECLs from the destination could already have leaked into
the tree we recurse into?)
Do you mean that the test on DECL_CONTEXT is superfluous? Possibly indeed,
but with nested functions you can have PARM_DECLs of different origins in a
given function body, although this may be irrelevant for tree-inline.c.
Yeah, I thought testing for a PARM_DECL should be sufficient? For
nested functions
references to outer parms should have been lowered via the static
chain at the point
tree-inline.c sees them.
So, if you agree that the DECL_CONTEXT test is superfluous the patch is ok
with the is_parameter_of function removed.
Thanks,
Richard.
--
Eric Botcazou
Re: [PATCH] Fix segfault with inlining
Yeah, I thought testing for a PARM_DECL should be sufficient? For nested functions references to outer parms should have been lowered via the static chain at the point tree-inline.c sees them. OK for the latter point, but are you sure for the former? My understanding is that we're already in SSA form, so parameters can be represented by SSA_NAMEs without defining statements. -- Eric Botcazou
Re: [PATCH] Fix segfault with inlining
On Fri, Sep 13, 2013 at 04:29:48PM +0200, Eric Botcazou wrote: @@ -4748,6 +4774,8 @@ copy_gimple_seq_and_replace_locals (gimp id.transform_call_graph_edges = CB_CGE_DUPLICATE; id.transform_new_cfg = false; id.transform_return_to_modify = false; + id.transform_parameter = false; + id.transform_parameter = false; id.transform_lang_insert_block = NULL; /* Walk the tree once to find local labels. */ Why are you storing the same thing twice? Jakub
Re: [PATCH] Fix segfault with inlining
On Tue, Sep 17, 2013 at 10:42 AM, Eric Botcazou ebotca...@adacore.com wrote: Yeah, I thought testing for a PARM_DECL should be sufficient? For nested functions references to outer parms should have been lowered via the static chain at the point tree-inline.c sees them. OK for the latter point, but are you sure for the former? My understanding is that we're already in SSA form, so parameters can be represented by SSA_NAMEs without defining statements. That's true... so you can only simplify is_parameter_of by dropping the context check. Richard. -- Eric Botcazou
Re: [PATCH] Fix segfault with inlining
That's true... so you can only simplify is_parameter_of by dropping the context check. OK, thanks, installed with this modification and the fix for the oversight spotted by Jakub, after retesting on x86-64/Linux. -- Eric Botcazou
Re: [PATCH] Fix segfault with inlining
On Fri, Sep 13, 2013 at 4:29 PM, Eric Botcazou ebotca...@adacore.com wrote:
Hi,
in Ada parameters can be passed by reference: in this case, the address of the
argument is directly passed to the callee, which dereferences it to access the
argument; now Ada also enables -fexceptions -fnon-call-exceptions, which means
that any pointer dereference is seen as the potential source of an exception,
which can quickly block the optimizer. That's why we put TREE_THIS_NOTRAP on
the dereferences associated with a parameter.
This works fine as long as the function isn't inlined, because it may happen
that the argument is itself the dereference of a pointer, properly guarded by
a null check on the pointer. When the function is inlined, the dereference in
the caller is replaced with that of the callee, which is TREE_THIS_NOTRAP and
thus can be moved ahead of the null pointer check, for example by LIM.
The patch ensures that this cannot happen by clearing TREE_THIS_NOTRAP in the
inliner. I think that this affects only the Ada compiler in practice.
Tested on x86_64-suse-linux, OK for the mainline?
I've looked at the C++ testcase
int foo (int x)
{
try {
return x;
}
catch (...)
{
return 0;
}
}
which exhibits exactly the behavior you quote - return x is considered throwing
an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to be
set here (maybe due to this issue you quote?).
Other than that the patch looks reasonable (I suppose you need
is_parameter_of only because as we recursively handle the trees
PARM_DECLs from the destination could already have leaked into
the tree we recurse into?)
Thanks,
Richard.
2013-09-13 Eric Botcazou ebotca...@adacore.com
* tree-inline.h (struct copy_body_data): Add transform_parameter.
* tree-inline.c (is_parameter_of): New predicate.
(remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if
a parameter has been remapped.
(copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF.
(optimize_inline_calls): Initialize transform_parameter.
(unsave_expr_now): Likewise.
(copy_gimple_seq_and_replace_locals): Likewise.
(tree_function_versioning): Likewise.
(maybe_inline_call_in_expr): Likewise.
2013-09-13 Eric Botcazou ebotca...@adacore.com
* gnat.dg/opt27.adb: New test.
* gnat.dg/opt27_pkg.ad[sb]: New helper.
--
Eric Botcazou
Re: [PATCH] Fix segfault with inlining
On 09/16/2013 05:28 AM, Richard Biener wrote: which exhibits exactly the behavior you quote - return x is considered throwing an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to be set here (maybe due to this issue you quote?). I haven't been aware of TREE_THIS_NOTRAP, but we could certainly start setting it. Jason
[PATCH] Fix segfault with inlining
Hi,
in Ada parameters can be passed by reference: in this case, the address of the
argument is directly passed to the callee, which dereferences it to access the
argument; now Ada also enables -fexceptions -fnon-call-exceptions, which means
that any pointer dereference is seen as the potential source of an exception,
which can quickly block the optimizer. That's why we put TREE_THIS_NOTRAP on
the dereferences associated with a parameter.
This works fine as long as the function isn't inlined, because it may happen
that the argument is itself the dereference of a pointer, properly guarded by
a null check on the pointer. When the function is inlined, the dereference in
the caller is replaced with that of the callee, which is TREE_THIS_NOTRAP and
thus can be moved ahead of the null pointer check, for example by LIM.
The patch ensures that this cannot happen by clearing TREE_THIS_NOTRAP in the
inliner. I think that this affects only the Ada compiler in practice.
Tested on x86_64-suse-linux, OK for the mainline?
2013-09-13 Eric Botcazou ebotca...@adacore.com
* tree-inline.h (struct copy_body_data): Add transform_parameter.
* tree-inline.c (is_parameter_of): New predicate.
(remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if
a parameter has been remapped.
(copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF.
(optimize_inline_calls): Initialize transform_parameter.
(unsave_expr_now): Likewise.
(copy_gimple_seq_and_replace_locals): Likewise.
(tree_function_versioning): Likewise.
(maybe_inline_call_in_expr): Likewise.
2013-09-13 Eric Botcazou ebotca...@adacore.com
* gnat.dg/opt27.adb: New test.
* gnat.dg/opt27_pkg.ad[sb]: New helper.
--
Eric BotcazouIndex: tree-inline.c
===
--- tree-inline.c (revision 202431)
+++ tree-inline.c (working copy)
@@ -751,6 +751,21 @@ copy_gimple_bind (gimple stmt, copy_body
return new_bind;
}
+/* Return true if DECL is a parameter or a SSA_NAME for a parameter of
+ function FNDECL. */
+
+static bool
+is_parameter_of (tree decl, tree fndecl)
+{
+ if (TREE_CODE (decl) == SSA_NAME)
+{
+ decl = SSA_NAME_VAR (decl);
+ if (!decl)
+ return false;
+}
+
+ return (TREE_CODE (decl) == PARM_DECL DECL_CONTEXT (decl) == fndecl);
+}
/* Remap the GIMPLE operand pointed to by *TP. DATA is really a
'struct walk_stmt_info *'. DATA-INFO is a 'copy_body_data *'.
@@ -840,20 +855,25 @@ remap_gimple_op_r (tree *tp, int *walk_s
if (TREE_CODE (*tp) == MEM_REF)
{
- tree ptr = TREE_OPERAND (*tp, 0);
- tree type = remap_type (TREE_TYPE (*tp), id);
- tree old = *tp;
-
/* We need to re-canonicalize MEM_REFs from inline substitutions
that can happen when a pointer argument is an ADDR_EXPR.
Recurse here manually to allow that. */
+ tree ptr = TREE_OPERAND (*tp, 0);
+ tree type = remap_type (TREE_TYPE (*tp), id);
+ tree old = *tp;
walk_tree (ptr, remap_gimple_op_r, data, NULL);
- *tp = fold_build2 (MEM_REF, type,
- ptr, TREE_OPERAND (*tp, 1));
- TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old);
+ *tp = fold_build2 (MEM_REF, type, ptr, TREE_OPERAND (*tp, 1));
TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old);
TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old);
TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old);
+ /* We cannot propagate the TREE_THIS_NOTRAP flag if we have
+ remapped a parameter as the property might be valid only
+ for the parameter itself. */
+ if (TREE_THIS_NOTRAP (old)
+ (!is_parameter_of (TREE_OPERAND (old, 0), id-src_fn)
+ || (!id-transform_parameter
+ is_parameter_of (ptr, id-dst_fn
+ TREE_THIS_NOTRAP (*tp) = 1;
*walk_subtrees = 0;
return NULL;
}
@@ -1043,45 +1063,45 @@ copy_tree_body_r (tree *tp, int *walk_su
/* Get rid of * from inline substitutions that can happen when a
pointer argument is an ADDR_EXPR. */
tree decl = TREE_OPERAND (*tp, 0);
- tree *n;
-
- n = (tree *) pointer_map_contains (id-decl_map, decl);
+ tree *n = (tree *) pointer_map_contains (id-decl_map, decl);
if (n)
{
- tree new_tree;
- tree old;
/* If we happen to get an ADDR_EXPR in n-value, strip
it manually here as we'll eventually get ADDR_EXPRs
which lie about their types pointed to. In this case
build_fold_indirect_ref wouldn't strip the INDIRECT_REF,
but we absolutely rely on that. As fold_indirect_ref
does other useful transformations, try that first, though. */
- tree type = TREE_TYPE (TREE_TYPE (*n));
- if (id-do_not_unshare)
- new_tree = *n;
- else
- new_tree = unshare_expr (*n);
- old = *tp;
- *tp = gimple_fold_indirect_ref (new_tree);
+ tree type = TREE_TYPE (*tp);
+ tree ptr = id-do_not_unshare ? *n : unshare_expr (*n);
+
