Re: [patch] Fix dangling references in thunks at -O0
> This introduces an ICE building the glibc testsuite for alpha (bisected),
> s390 and sparc (symptoms appear the same, not bisected to confirm the
> exact revision). See bug 97078.
Fixed thus, tested on x86_64-suse-linux, applied on mainline as obvious.
PR middle-end/97078
* function.c (use_register_for_decl): Test cfun->tail_call_marked
for a parameter here instead of...
(assign_parm_setup_reg): ...here.
* gcc.dg/pr97078.c: New test.
--
Eric Botcazoudiff --git a/gcc/function.c b/gcc/function.c
index c4c9930d725..c6129593b9b 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -2237,6 +2237,11 @@ use_register_for_decl (const_tree decl)
if (optimize)
return true;
+ /* Thunks force a tail call even at -O0 so we need to avoid creating a
+ dangling reference in case the parameter is passed by reference. */
+ if (TREE_CODE (decl) == PARM_DECL && cfun->tail_call_marked)
+return true;
+
if (!DECL_REGISTER (decl))
return false;
@@ -3328,9 +,8 @@ assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm,
of the parameter instead. */
if (data->arg.pass_by_reference && TYPE_MODE (TREE_TYPE (parm)) != BLKmode)
{
- /* Use a stack slot for debugging purposes, except if a tail call is
- involved because this would create a dangling reference. */
- if (use_register_for_decl (parm) || cfun->tail_call_marked)
+ /* Use a stack slot for debugging purposes if possible. */
+ if (use_register_for_decl (parm))
{
parmreg = gen_reg_rtx (TYPE_MODE (TREE_TYPE (parm)));
mark_user_reg (parmreg);
/* { dg-do compile } */
/* { dg-options "-O2 -ffloat-store" } */
extern void foo (long double);
void bar (long double d)
{
foo (d);
}
Re: [patch] Fix dangling references in thunks at -O0
This introduces an ICE building the glibc testsuite for alpha (bisected), s390 and sparc (symptoms appear the same, not bisected to confirm the exact revision). See bug 97078. -- Joseph S. Myers jos...@codesourcery.com
Re: [patch] Fix dangling references in thunks at -O0
On Mon, Sep 14, 2020 at 1:31 PM Eric Botcazou wrote: > > > In fact I can probably reuse cfun->tail_call_marked for this purpose. > > Like so. Looks reasonable to me. Richard. > > * cgraphunit.c (cgraph_node::expand_thunk): Make sure to set > cfun->tail_call_marked when forcing a tail call. > * function.c (assign_parm_setup_reg): Always use a register to > retrieve a parameter passed by reference if cfun->tail_call_marked. > > -- > Eric Botcazou
Re: [patch] Fix dangling references in thunks at -O0
> In fact I can probably reuse cfun->tail_call_marked for this purpose.
Like so.
* cgraphunit.c (cgraph_node::expand_thunk): Make sure to set
cfun->tail_call_marked when forcing a tail call.
* function.c (assign_parm_setup_reg): Always use a register to
retrieve a parameter passed by reference if cfun->tail_call_marked.
--
Eric Botcazoudiff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 26d3995a0c0..bedb6e2eea1 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -2171,7 +2171,10 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
}
}
else
- gimple_call_set_tail (call, true);
+ {
+ gimple_call_set_tail (call, true);
+ cfun->tail_call_marked = true;
+ }
/* Build return value. */
if (!DECL_BY_REFERENCE (resdecl))
@@ -2184,6 +2187,7 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool force_gimple_thunk)
else
{
gimple_call_set_tail (call, true);
+ cfun->tail_call_marked = true;
remove_edge (single_succ_edge (bb));
}
diff --git a/gcc/function.c b/gcc/function.c
index 2c8fa217f1f..314fd01c195 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -3322,13 +3322,15 @@ assign_parm_setup_reg (struct assign_parm_data_all *all, tree parm,
else
emit_move_insn (parmreg, validated_mem);
- /* If we were passed a pointer but the actual value can safely live
- in a register, retrieve it and use it directly. */
+ /* If we were passed a pointer but the actual value can live in a register,
+ retrieve it and use it directly. Note that we cannot use nominal_mode,
+ because it will have been set to Pmode above, we must use the actual mode
+ of the parameter instead. */
if (data->arg.pass_by_reference && TYPE_MODE (TREE_TYPE (parm)) != BLKmode)
{
- /* We can't use nominal_mode, because it will have been set to
- Pmode above. We must use the actual mode of the parm. */
- if (use_register_for_decl (parm))
+ /* Use a stack slot for debugging purposes, except if a tail call is
+ involved because this would create a dangling reference. */
+ if (use_register_for_decl (parm) || cfun->tail_call_marked)
{
parmreg = gen_reg_rtx (TYPE_MODE (TREE_TYPE (parm)));
mark_user_reg (parmreg);
Re: [patch] Fix dangling references in thunks at -O0
On Mon, Sep 14, 2020 at 12:56 PM Eric Botcazou wrote: > > > No, so you're right - validity analysis is split. Still the cause you > > reference comes from RTL expansion which means RTL expansion should > > possibly do the disqualification here. The question is what makes the case > > you run into at -O0 impossible to trigger with -O1+? > > The key test in use_register_for_decl is: > > if (optimize) > return true; > > I think that all the preceding tests that return false cannot trigger in this > context - a parameter passed by invisible reference - so, by returning true > here, you're pretty much covered I think. > > > Maybe we can also avoid this spilling at -O0? > > The code wants to retrieve the parameter at all optimization levels. The > question is: register or stack slot? It uses use_register_for_decl to decide > as in other circumstances, but so you want to always force a register? > > Note that, since cgraph_node::expand_thunk has expanded the thunk in GIMPLE, > the function is no longer a thunk for the middle-end (cfun->is_thunk false). > > That being said, I can add another bit to cfun, e.g. is_gimple_thunk, and > force the use of a register above only in case the bit is true. /* We don't set DECL_IGNORED_P for the function_result_decl. */ if (optimize) return true; /* We don't set DECL_REGISTER for the function_result_decl. */ return false; or maybe set (and check) DECL_REGISTER? But the above certainly looks like we can pick as we wish. > > Yeah, but the asm thunk tail-calls also at -O0. I guess tail-calling is > > forced here because gimple analysis sometimes would not mark the call. > > The low-level assembly thunks simply support the most simple thunks, see the > condition in expand_thunk. Moreover targets can opt out as they wish. > > -- > Eric Botcazou > >
Re: [patch] Fix dangling references in thunks at -O0
> That being said, I can add another bit to cfun, e.g. is_gimple_thunk, and > force the use of a register above only in case the bit is true. In fact I can probably reuse cfun->tail_call_marked for this purpose. -- Eric Botcazou
Re: [patch] Fix dangling references in thunks at -O0
> No, so you're right - validity analysis is split. Still the cause you > reference comes from RTL expansion which means RTL expansion should > possibly do the disqualification here. The question is what makes the case > you run into at -O0 impossible to trigger with -O1+? The key test in use_register_for_decl is: if (optimize) return true; I think that all the preceding tests that return false cannot trigger in this context - a parameter passed by invisible reference - so, by returning true here, you're pretty much covered I think. > Maybe we can also avoid this spilling at -O0? The code wants to retrieve the parameter at all optimization levels. The question is: register or stack slot? It uses use_register_for_decl to decide as in other circumstances, but so you want to always force a register? Note that, since cgraph_node::expand_thunk has expanded the thunk in GIMPLE, the function is no longer a thunk for the middle-end (cfun->is_thunk false). That being said, I can add another bit to cfun, e.g. is_gimple_thunk, and force the use of a register above only in case the bit is true. > Yeah, but the asm thunk tail-calls also at -O0. I guess tail-calling is > forced here because gimple analysis sometimes would not mark the call. The low-level assembly thunks simply support the most simple thunks, see the condition in expand_thunk. Moreover targets can opt out as they wish. -- Eric Botcazou
Re: [patch] Fix dangling references in thunks at -O0
On Mon, Sep 14, 2020 at 10:36 AM Eric Botcazou wrote:
>
> > ISTR the tailcall flag is only a hint and RTL expansion can decide to
> > not tailcall based on targets. So to me it looks like a missed
> > disqualification on the RTL expansion side.
>
> That's IMO debatable: the GIMPLE tailcall optimization pass e.g. does:
>
> /* Make sure the tail invocation of this function does not indirectly
> refer to local variables. (Passing variables directly by value
> is OK.) */
> FOR_EACH_LOCAL_DECL (cfun, idx, var)
> {
> if (TREE_CODE (var) != PARM_DECL
> && auto_var_in_fn_p (var, cfun->decl)
> && may_be_aliased (var)
> && (ref_maybe_used_by_stmt_p (call, var)
> || call_may_clobber_ref_p (call, var)))
>
> Do you want to replace it with something at the RTL level too? I don't think
> that you can disqualify things at the RTL as easily as at the GIMPLE level.
No, so you're right - validity analysis is split. Still the cause you reference
comes from RTL expansion which means RTL expansion should possibly
do the disqualification here. The question is what makes the case you
run into at -O0 impossible to trigger with -O1+?
Maybe we can also avoid this spilling at -O0?
> > Or do we, besides from this very single spot, simply never tailcall at -O0
> > and thus never hit this latent issue?
>
> Presumably yes, tail calling is an optimization like the others.
Yeah, but the asm thunk tail-calls also at -O0. I guess tail-calling is forced
here because gimple analysis sometimes would not mark the call.
> > How does this change the debug experience at -O0 when GIMPLE thunks
> > are used?
>
> In Ada this doesn't change much since thunks have line debug info.
I see, so as long as you then don't see extra frames it should be fine
with regard to debug info.
I'm not against the patch but let's leave it a bit for others to comment.
Richard.
> --
> Eric Botcazou
>
>
Re: [patch] Fix dangling references in thunks at -O0
> ISTR the tailcall flag is only a hint and RTL expansion can decide to
> not tailcall based on targets. So to me it looks like a missed
> disqualification on the RTL expansion side.
That's IMO debatable: the GIMPLE tailcall optimization pass e.g. does:
/* Make sure the tail invocation of this function does not indirectly
refer to local variables. (Passing variables directly by value
is OK.) */
FOR_EACH_LOCAL_DECL (cfun, idx, var)
{
if (TREE_CODE (var) != PARM_DECL
&& auto_var_in_fn_p (var, cfun->decl)
&& may_be_aliased (var)
&& (ref_maybe_used_by_stmt_p (call, var)
|| call_may_clobber_ref_p (call, var)))
Do you want to replace it with something at the RTL level too? I don't think
that you can disqualify things at the RTL as easily as at the GIMPLE level.
> Or do we, besides from this very single spot, simply never tailcall at -O0
> and thus never hit this latent issue?
Presumably yes, tail calling is an optimization like the others.
> How does this change the debug experience at -O0 when GIMPLE thunks
> are used?
In Ada this doesn't change much since thunks have line debug info.
--
Eric Botcazou
Re: [patch] Fix dangling references in thunks at -O0
On Mon, Sep 14, 2020 at 9:46 AM Eric Botcazou wrote:
>
> Hi,
>
> when a thunk cannot be emitted in assembly directly, cgraph_node::expand_thunk
> generates regular GIMPLE code but unconditionally forces a tail call to the
> target of the thunk. That's theoretically OK because the thunk essentially
> forwards its parameters to the target, but in practice the RTL expander can
> spill parameters passed by reference on the stack, see assign_parm_setup_reg:
>
> /* If we were passed a pointer but the actual value can safely live
> in a register, retrieve it and use it directly. */
> if (data->arg.pass_by_reference && TYPE_MODE (TREE_TYPE (parm)) != BLKmode)
> {
> /* We can't use nominal_mode, because it will have been set to
> Pmode above. We must use the actual mode of the parm. */
> if (use_register_for_decl (parm))
> {
> parmreg = gen_reg_rtx (TYPE_MODE (TREE_TYPE (parm)));
> mark_user_reg (parmreg);
> }
> else
> {
> int align = STACK_SLOT_ALIGNMENT (TREE_TYPE (parm),
> TYPE_MODE (TREE_TYPE (parm)),
> TYPE_ALIGN (TREE_TYPE (parm)));
> parmreg
> = assign_stack_local (TYPE_MODE (TREE_TYPE (parm)),
> GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (parm))),
> align);
> set_mem_attributes (parmreg, parm, 1);
> }
>
> use_register_for_decl always return false at -O0 so, in this case, the thunk
> will pass an address within its frame to its target, so it cannot use a tail
> call to invoke it.
>
> Tested on x86_64-suse-linux, OK for the mainline?
ISTR the tailcall flag is only a hint and RTL expansion can decide to
not tailcall based on targets. So to me it looks like a missed disqualification
on the RTL expansion side. Or do we, besides from this very single spot,
simply never tailcall at -O0 and thus never hit this latent issue?
How does this change the debug experience at -O0 when GIMPLE thunks
are used?
Thanks,
Richard.
>
> 2020-09-14 Eric Botcazou
>
> * cgraphunit.c (cgraph_node::expand_thunk): Force a tail call only
> when optimizing.
>
>
> 2020-09-14 Eric Botcazou
>
> * gnat.dg/thunk1.adb: New test.
> * gnat.dg/thunk1_pkg1.ads: New helper.
> * gnat.dg/thunk1_pkg2.ads: Likewise.
> * gnat.dg/thunk1_pkg2.adb: Likewise.
>
> --
> Eric Botcazou
