Message ID | 2127372.YidMXMCpNE@polaris |
---|---|
State | New |
Headers | show |
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.
Index: explow.c =================================================================== --- explow.c (revision 244917) +++ explow.c (working copy) @@ -1231,9 +1231,14 @@ get_dynamic_stack_size (rtx *psize, unsi 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. */ - + in SIZE for the hole that might result from the alignment operation. + However, we assume that, if STACK_DYNAMIC_OFFSET is defined by the + back-end itself as opposed to function.c, it is properly aligned. */ +#ifdef STACK_DYNAMIC_OFFSET unsigned known_align = REGNO_POINTER_ALIGN (VIRTUAL_STACK_DYNAMIC_REGNUM); +#else + unsigned known_align = 0; +#endif if (known_align == 0) known_align = BITS_PER_UNIT; if (required_align > known_align)