[PATCH,RFC] Do not assign stack slots to ssa names

Hi,
When processing stack vars partitions for alias export patch, I have
noticed that sometimes we get SSA_NAMEs being allocated on stack, e.g.
V2SF vectors. Richard suggested the attached patch to fix it. The
patch is bootstrapped and tested (Ada enabled) on x86-64. The only
failing test is stack protector test ssp-2.c, in which an array and a
loop counter get allocated on stack in a different order and thus
instead of overflowing the array the test overflows the loop counter and
hangs. So I've also fixed the test.
Michael, what do you think of the patch?
Andrey
2009-06-10 Richard Guenther
Andrey Belevantsev
* cfgexpand.c (expand_one_var): Do not call add_stack_var when original
variable is an SSA name.
* testsuite/gcc.dg/ssp-2.c (overflow): Make i global.
Index: gcc/testsuite/gcc.dg/ssp-2.c
===================================================================
*** gcc/testsuite/gcc.dg/ssp-2.c (revision 148346)
--- gcc/testsuite/gcc.dg/ssp-2.c (working copy)
*************** __stack_chk_fail (void)
*** 11,20 ****
exit (0); /* pass */
}
void
overflow()
{
- int i = 0;
char foo[30];
/* Overflow buffer. */
--- 11,21 ----
exit (0); /* pass */
}
+ static int i;
+
void
overflow()
{
char foo[30];
/* Overflow buffer. */
Index: gcc/cfgexpand.c
===================================================================
*** gcc/cfgexpand.c (revision 148346)
--- gcc/cfgexpand.c (working copy)
*************** expand_one_var (tree var, bool toplevel,
*** 1212,1218 ****
if (really_expand)
expand_one_register_var (origvar);
}
! else if (defer_stack_allocation (var, toplevel))
add_stack_var (origvar);
else
{
--- 1212,1219 ----
if (really_expand)
expand_one_register_var (origvar);
}
! else if (defer_stack_allocation (var, toplevel)
! && TREE_CODE (origvar) != SSA_NAME)
add_stack_var (origvar);
else
{

Andrey Belevantsev wrote:
> Hi,
>
> When processing stack vars partitions for alias export patch, I have
> noticed that sometimes we get SSA_NAMEs being allocated on stack, e.g.
> V2SF vectors. Richard suggested the attached patch to fix it. The
> patch is bootstrapped and tested (Ada enabled) on x86-64. The only
> failing test is stack protector test ssp-2.c, in which an array and a
> loop counter get allocated on stack in a different order and thus
> instead of overflowing the array the test overflows the loop counter and
> hangs. So I've also fixed the test.
>
> Michael, what do you think of the patch?
> Andrey
>
>
> 2009-06-10 Richard Guenther
> Andrey Belevantsev
>
> * cfgexpand.c (expand_one_var): Do not call add_stack_var when original
> variable is an SSA name.
This doesn't make any sense. AFAICT, you're still allocating stack
space for the variable, just at a different place.

On Fri, Jun 12, 2009 at 6:09 AM, Richard Henderson wrote:
> Andrey Belevantsev wrote:
>>
>> Hi,
>>
>> When processing stack vars partitions for alias export patch, I have
>> noticed that sometimes we get SSA_NAMEs being allocated on stack, e.g.
>> V2SF vectors. =A0Richard suggested the attached patch to fix it. =A0The
>> patch is bootstrapped and tested (Ada enabled) on x86-64. =A0The only
>> failing test is stack protector test ssp-2.c, in which an array and a
>> loop counter get allocated on stack in a different order and thus
>> instead of overflowing the array the test overflows the loop counter and
>> hangs. =A0So I've also fixed the test.
>>
>> Michael, what do you think of the patch?
>> Andrey
>>
>>
>> 2009-06-10 =A0Richard Guenther =A0
>> =A0 =A0 =A0 =A0 =A0 =A0Andrey Belevantsev =A0
>>
>> =A0 =A0 =A0 =A0* cfgexpand.c (expand_one_var): Do not call add_stack_var=
when
>> original
>> variable is an SSA name.
>
> This doesn't make any sense. =A0AFAICT, you're still allocating stack
> space for the variable, just at a different place.
It does make sense in that we currently put SSA names into the stack
coalescing machinery (which this patch avoids to). Doing stack slot
coalescing for SSA name variables causes us to generate aliases
for them - which we do not expect and I believe cannot reasonably
handle (at least definitely not with the alias machinery we currently
have). Micha told me that we will actually never do any coalescing
here anyway, so simply bypassing the machinery should not hurt.
Another approach would be to re-write variables that we cannot put
in registers out-of-SSA again.
Richard.
>

Hi,
On Wed, 10 Jun 2009, Andrey Belevantsev wrote:
> When processing stack vars partitions for alias export patch, I have
> noticed that sometimes we get SSA_NAMEs being allocated on stack, e.g.
> V2SF vectors. Richard suggested the attached patch to fix it.
Yeah, while discussing this I tried the same. But it actually changes the
underlying assumptions of the stack slot allocation code, in precisely the
way that is tested ...
> patch is bootstrapped and tested (Ada enabled) on x86-64. The only
> failing test is stack protector test ssp-2.c, in which an array and a
> loop counter get allocated on stack in a different order and thus
> instead of overflowing the array the test overflows the loop counter and
> hangs.
... here. _All_ stack slot allocations need to be deferred when stack
protector is on (for the reason the comment therein explains), so that the
reordering gets a chance to do it's job. So ...
> So I've also fixed the test.
>
> Michael, what do you think of the patch?
... I don't think this is going to fly. What was the real problem again?
IIRC it could also be fixed by giving those stack slots their own
partition, right?
Ciao,
Michael.

Hi,
On Fri, 12 Jun 2009, Richard Guenther wrote:
> It does make sense in that we currently put SSA names into the stack
> coalescing machinery (which this patch avoids to). Doing stack slot
> coalescing for SSA name variables causes us to generate aliases for them
> - which we do not expect and I believe cannot reasonably handle (at
> least definitely not with the alias machinery we currently have).
Ah right, _that_ was the problem. Okay then you simply have to not
generate aliases for these.
> Micha told me that we will actually never do any coalescing here anyway,
> so simply bypassing the machinery should not hurt.
That's what I thought until I've seen the stack protector also relying on
the coalescing machinery. Not for coalescing (because those vars aren't)
but for queuing everything to resort later.
And it actually matters. Not queuing means immediately expanding them to
stack slots, meaning top of stack. Queuing in the presence of
"interesting" other variables (strings, arrays) means expanding them after
those, i.e. bottom of stack.
I think this has to be preserved.
> Another approach would be to re-write variables that we cannot put in
> registers out-of-SSA again.
I would simply mark the SSAness in the queue of stackvars and not generate
the above aliases.
Ciao,
Michael.

Michael Matz wrote:
> I would simply mark the SSAness in the queue of stackvars and not generate
> the above aliases.
Yes, exactly.
r~

Michael Matz wrote:
> I would simply mark the SSAness in the queue of stackvars and not generate
> the above aliases.
This is what happens with the current alias-export patch, I just skip
these while reflecting stack partition info in saved alias information.
With this patch, I was investigating the possibility of avoiding this.
As the thread shows this is not desirable, I'm happy enough with what
we have now.
Andrey