Fix PR middle-end/78468

Hi,

this is the regression introduced on 32-bit SPARC by this change:

2016-11-18  Dominik Vogt  <vogt@linux.vnet.ibm.com>

        Re-apply after PR bootstrap/77359 is fixed:
        2016-08-23  Dominik Vogt  <vogt@linux.vnet.ibm.com>

        * explow.c (get_dynamic_stack_size): Take known alignment of stack
        pointer + STACK_DYNAMIC_OFFSET into account when calculating the
        size needed.

and which causes the compiler to overwrite contents on the stack after a 
dynamic allocation in some cases.

IMO this patch is backwards and should not have been approved: as explained by 
the comment in get_dynamic_stack_size:

  /* We will need to ensure that the address we return is aligned to
     REQUIRED_ALIGN.  At this point in the compilation, we don't always
     know the final value of the STACK_DYNAMIC_OFFSET used in function.c
     (it might depend on the size of the outgoing parameter lists, for
     example), so we must preventively align the value.  We leave space
     in SIZE for the hole that might result from the alignment operation.  */

the function used to align the value to maintain the expected alignment of 
VIRTUAL_STACK_DYNAMIC_REGNUM.  But the new version does the opposite (without 
changing the comment):

  unsigned known_align = REGNO_POINTER_ALIGN (VIRTUAL_STACK_DYNAMIC_REGNUM);

it assumes that the alignment of VIRTUAL_STACK_DYNAMIC_REGNUM is already 
correct to optimize the dynamic allocation, i.e. to save a few bytes.

This breaks the interface between middle-end and back-end and resulted in the 
breakage of PowerPC/AIX and now of the 32-bit SPARC port.  And the failure 
mode is quite nasty: the entire testsuite of the compiler proper, including 
Ada, is clean on 32-bit SPARC and the issue only shows up in the libgomp 
testsuite, probably because of specific patterns.

IMO it's very likely that other architectures among the ~50 supported ones are 
affected and the issue might show up only in very peculiar circumstances as on 
32-bit SPARC so I think we should repair the breakage for GCC 7.  The attached 
patch is a middle ground between the previously working and currently broken 
situations: if the back-end defines STACK_DYNAMIC_OFFSET, then the middle-end 
assumes that STACK_DYNAMIC_OFFSET maintains the alignment; if it doesn't, 
which means the default value of STACK_DYNAMIC_OFFSET computed in function.c 
is used instead, then the middle-end maintains the alignment itself.

Tested on x86_64-suse-linux and SPARC/Solaris, OK for the mainline?


2017-01-27  Eric Botcazou  <ebotcazou@adacore.com>

	PR middle-end/78468
	* explow.c (get_dynamic_stack_size): Take known alignment of stack
        pointer + STACK_DYNAMIC_OFFSET into account only if the back-end
	defines STACK_DYNAMIC_OFFSET.

On 01/27/2017 01:02 PM, Eric Botcazou wrote:
> The attached
> patch is a middle ground between the previously working and currently broken
> situations: if the back-end defines STACK_DYNAMIC_OFFSET, then the middle-end
> assumes that STACK_DYNAMIC_OFFSET maintains the alignment; if it doesn't,
> which means the default value of STACK_DYNAMIC_OFFSET computed in function.c
> is used instead, then the middle-end maintains the alignment itself.

I'd say let's not have a middle ground - this stuff is sufficiently 
brain-twisting that I'd rather go back to a known working state. If 
there was an error in the previous patch, let's roll it back until we 
fully understand the situation.


Bernd

On 01/27/2017 06:43 AM, Bernd Schmidt wrote:
> On 01/27/2017 01:02 PM, Eric Botcazou wrote:
>> The attached
>> patch is a middle ground between the previously working and currently
>> broken
>> situations: if the back-end defines STACK_DYNAMIC_OFFSET, then the
>> middle-end
>> assumes that STACK_DYNAMIC_OFFSET maintains the alignment; if it doesn't,
>> which means the default value of STACK_DYNAMIC_OFFSET computed in
>> function.c
>> is used instead, then the middle-end maintains the alignment itself.
>
> I'd say let's not have a middle ground - this stuff is sufficiently
> brain-twisting that I'd rather go back to a known working state. If
> there was an error in the previous patch, let's roll it back until we
> fully understand the situation.
I tend to agree.  We've been round and round on this issue.  The code is 
a horrid mess IMHO.  Every time I think I understand it I'm proven wrong.

IIRC what motivated all this in the beginning was to eliminate 
unnecessary stack utilization.  So reverting back (assuming we can find 
all the dependent patches) should be the safe thing to do.


JEff

> I'd say let's not have a middle ground - this stuff is sufficiently
> brain-twisting that I'd rather go back to a known working state. If
> there was an error in the previous patch, let's roll it back until we
> fully understand the situation.

In fact it's not exactly a middle ground; it's already almost a roll back 
since only PowerPC, PA-RISC and s390 define STACK_DYNAMIC_OFFSET.

PowerPC and s390 are OK according to their maintainers, so we're left with 
only PA-RISC.  IIUC the question boils down to whether STACK_DYNAMIC_OFFSET 
garantees that (stack pointer + STACK_DYNAMIC_OFFSET) is always aligned to 
STACK_BOUNDARY, which is 2 * BITS_PER_WORD on PA-RISC.

Dave, can you give the answer here?

[Dave replied privately that PA-RISC is OK too].

> I'd say let's not have a middle ground - this stuff is sufficiently
> brain-twisting that I'd rather go back to a known working state. If
> there was an error in the previous patch, let's roll it back until we
> fully understand the situation.

See the audit trail of the PR, in particular comment #24:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78468#c24

In other words, the issue is more general than just STACK_DYNAMIC_OFFSET.