Message ID | e9cfc059f15f9d55b672a809b8bd3f249acedf93.1557171132.git.segher@kernel.crashing.org |
---|---|
State | New |
Headers | show |
Series | [1/3] rs6000: rs6000_dbx_register_number for fp/ap/mq | expand |
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? 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. > diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h > index ff9449c..3829e8f 100644 > --- a/gcc/config/rs6000/rs6000.h > +++ b/gcc/config/rs6000/rs6000.h > @@ -817,9 +817,6 @@ enum data_align { align_abi, align_opt, align_both }; > > #define FIRST_PSEUDO_REGISTER 115 > > -/* This must be included for pre gcc 3.0 glibc compatibility. */ > -#define PRE_GCC3_DWARF_FRAME_REGISTERS 77 > - > /* The sfp register and 3 HTM registers > aren't included in DWARF_FRAME_REGISTERS. */ > #define DWARF_FRAME_REGISTERS (FIRST_PSEUDO_REGISTER - 4) > -- > 1.8.3.1 Jakub
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. > 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? > > -/* This must be included for pre gcc 3.0 glibc compatibility. */ > > -#define PRE_GCC3_DWARF_FRAME_REGISTERS 77 === unwind-dw2.c /* Dwarf frame registers used for pre gcc 3.0 compiled glibc. */ #ifndef PRE_GCC3_DWARF_FRAME_REGISTERS #define PRE_GCC3_DWARF_FRAME_REGISTERS __LIBGCC_DWARF_FRAME_REGISTERS__ #endif === c-family/c-cppbuiltin.cc builtin_define_with_int_value ("__LIBGCC_DWARF_FRAME_REGISTERS__", DWARF_FRAME_REGISTERS); (Btw, we used many dwarf reg numbers > 1000 for a long time.) Segher
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
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h index ff9449c..3829e8f 100644 --- a/gcc/config/rs6000/rs6000.h +++ b/gcc/config/rs6000/rs6000.h @@ -817,9 +817,6 @@ enum data_align { align_abi, align_opt, align_both }; #define FIRST_PSEUDO_REGISTER 115 -/* This must be included for pre gcc 3.0 glibc compatibility. */ -#define PRE_GCC3_DWARF_FRAME_REGISTERS 77 - /* The sfp register and 3 HTM registers aren't included in DWARF_FRAME_REGISTERS. */ #define DWARF_FRAME_REGISTERS (FIRST_PSEUDO_REGISTER - 4)