Re: [PATCH 2/3] rs6000: Delete PRE_GCC3_DWARF_FRAME_REGISTERS

[Jakub Jelinek <jakub@redhat.com>]

On Sat, Jan 14, 2023 at 07:38:37AM -0600, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Jan 13, 2023 at 07:05:34PM +0100, Jakub Jelinek wrote:
> > On Mon, May 06, 2019 at 09:55:50PM +0000, Segher Boessenkool wrote:
> > > We don't need this.
> > > 
> > > 
> > > Segher
> > > 
> > > 
> > > 2019-05-06  Segher Boessenkool  <segher@kernel.crashing.org>
> > > 
> > > 	* config/rs6000/rs6000.h (PRE_GCC3_DWARF_FRAME_REGISTERS): Delete.
> > 
> > Why do you think so?
> 
> First off, this was three years ago, and no problems have shown up.

It is true that there aren't that many users of GCC 2.x compiled binaries
around.

> > This seems to be a clear ABI break to me in the __frame_state_for
> > API.
> > So, if a __frame_state_for caller calls the function, it will overflow
> > the buffer passed by the caller.
> 
> 77 <= 111 so how can this overflow where it didn't before?

typedef struct frame_state
{
  void *cfa;
  void *eh_ptr;
  long cfa_offset;
  long args_size;
  long reg_or_offset[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
  unsigned short cfa_reg;
  unsigned short retaddr_column;
  char saved[PRE_GCC3_DWARF_FRAME_REGISTERS+1];
} frame_state;

struct frame_state * __frame_state_for (void *, struct frame_state *);

Unlike the new unwinder stuff, __frame_state_for can be called from other
objects, it is exported from both libgcc (on some architectures, including
ppc, ppc64 and ppc64le) and from glibc (on some architectures, including ppc
and i386, but not ppc64 nor ppc64le).
PRE_GCC3_DWARF_FRAME_REGISTERS defaults to __LIBGCC_DWARF_FRAME_REGISTERS__,
which is initialized to DWARF_FRAME_REGISTERS (if not defined, it is
FIRST_PSEUDO_REGISTER).
So, if PRE_GCC3_DWARF_FRAME_REGISTERS was previously defined to 77 and
now is not, the layout of the above structure changed, and if something
calls __frame_state_for with the layout of the structure with 77
registers, then it will not work properly if you get 111 instead.

In the GCC 2.x code, __frame_state_for was defined in gcc/frame.c
(libgcc.a(frame.o), while it was used in libgcc2.c if L_eh was defined
(so libgcc.a(_eh.o)) and the libgcc.a vs. libgcc_eh.a split wasn't present
yet, so parts of the unwinder could be exported from one shared library and
consumed by another one etc.  Depending on what is picked for
__frame_state_for, this can lead to ABI issues (in particular if the
libgcc_s.so.1 version is).
New code doesn't call __frame_state_for, it is there solely for
backwards compatibility.  So, even when GCC 10/11/12 had it broken,
there is no reason not to fix it in 13 and perhaps backport to all release
branches.

glibc sources have:
./sysdeps/sh/gccframe.h:#define DWARF_FRAME_REGISTERS 49
./sysdeps/i386/gccframe.h:#define DWARF_FRAME_REGISTERS 17
./sysdeps/powerpc/gccframe.h:#define DWARF_FRAME_REGISTERS 77
./sysdeps/hppa/gccframe.h:#define DWARF_FRAME_REGISTERS 89
On the GCC side compared to the above:
gcc/config/sh/sh.h:#define DWARF_FRAME_REGISTERS (153)
gcc/config/i386/i386.h:#define DWARF_FRAME_REGISTERS 17
gcc/config/rs6000/rs6000.h:#define FIRST_PSEUDO_REGISTER 111
gcc/config/pa/pa.h:#define DWARF_FRAME_REGISTERS (FIRST_PSEUDO_REGISTER - 1)
gcc/config/pa/pa32-regs.h:#define FIRST_PSEUDO_REGISTER 90  /* 32 general regs + 56 fp regs +
gcc/config/pa/pa64-regs.h:#define FIRST_PSEUDO_REGISTER 62  /* 32 general regs + 28 fp regs +

So, when PRE_GCC3_DWARF_FRAME_REGISTERS was defined on rs6000, it matched
glibc on all arches but sh (I have no idea about sh and don't really care).
I assume for 64-bit pa __frame_state_for isn't exported from glibc.

	Jakub