https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78468

Dominik Vogt <vogt at linux dot vnet.ibm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vogt at linux dot vnet.ibm.com

--- Comment #22 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
> But these back-ends were not broken before your patch!  It's your patch that
> broke the interface between the middle-end and the back-end without notice.
I disagree with this statement and with marking this bug report as being
middleend related.  The patch has not changed the interface between the two
-ends but simply _uses_ the interface.

--

The facts about the situation, as far as I understand, are:

* The middleend promises that VIRTUAL_STACK_DYNMAIC_REGNUM has at least
STACK_BOUNDARY alignment (emit-rtl.c:init_emit()).
* The 32 bit Aix backend used to break this promise but has already been
changed to honour it.
* The 32 bit Sparc backend breaks this promise.
* Apparently some other backends don't (Power, Aix 64 bit, s390, s390x, x86,
x86_64), yet other backends may or may not break the promise.
* Thus, the middleend and the backends that do not keep this promise (Sparc,
Aix until recently, maybe others) disagree on the interface between backend and
middleend.
* Any user of REGNO_POINTER_ALIGN (VIRTUAL_STACK_DYNMAIC_REGNUM) may generate
bogus results for targets that break the promise.  Grepping through the sources
I didn't find any other user (but there may be still some that cannot be found
using simple "grep"s).
* The interface mismatch is impossible to fix without touching the backends
with non-STACK_BOUNDARY alignment of VIRTUAL_STACK_DYNMAIC_REGNUM.  (Either
these backends need to be changed to keep the promise or at least tell the
middleend that they don't.)
* The interface mismatch is virtually impossible to fix if every change that
tries to fix it is reverted because on some target "it used to work before the
patch".

* The discussed patch introduces a new use of REGNO_POINTER_ALIGN
(VIRTUAL_STACK_DYNMAIC_REGNUM) and therefore leads to bad results on targets
that don't keep the promise.
* The discussed patch _trusts_ REGNO_POINTER_ALIGN
(VIRTUAL_STACK_DYNMAIC_REGNUM) and wouldn't lead to faulty code if that value
were correct (i.e. if the alignment was set to 4 on Sparc instead of 8). 
(Note: Setting the correct pointer alignment on such targets would fix the bad
code generation but not the waste of stack space.)
* As far as it is known, (patched) code generation on targets that keep the
promise is fine.

* Backing out the patch in favour of not having to change the Sparc backend
(and potentially others) not only wastes "some bytes" on Sparc but on every
target.  (Although, Sparc wastes more stack space than other targets, because
of Sparc's peculiar guaranteed "unalignment".)
* Backing out the patch won't fix the disagreement of the middleend and the
Sparc backend about their interface.
* Any other existing and future use of REGNO_POINTER_ALIGN
(VIRTUAL_STACK_DYNMAIC_REGNUM) makes or will also make bogus assumptions, with
unforeseeable consequences, unless the mismatch in the interface is fixed in
some way.

Related to the former over-allocation of dynamic stack memory:

* Off-by-one array accesses to dynamic stack memory in user code often go
unnoticed when Gcc is used as the compiler while other compilers will probably
not have this kind bug and generate failing assembly code right away.
* The patch already helped to identify coding bugs in several Gcc internal
libraries, and Glibc.
* It can be assumed with near certainty that the patch will trigger off-by-one
bugs in applications compiled with Gcc, and that Gcc bug reports will be
created with "Gcc-7 must be broken because it worked with Gcc-6".

--

I'm certainly willing to help fixing any interface mismatch, the Sparc backend,
any other backend with similar problems or identifying library or user code
bugs that are triggered by the fixed middleend.

Reply via email to