The macro STACK_BOUNDARY may overflow

The macro STACK_BOUNDARY may overflow

[Paul Iannetta]

Hi,

Currently, the macro STACK_BOUNDARY is defined as

  Macro: STACK_BOUNDARY
     Define this macro to the minimum alignment enforced by hardware for
     the stack pointer on this machine.  The definition is a C
     expression for the desired alignment (measured in bits).  This
     value is used as a default if 'PREFERRED_STACK_BOUNDARY' is not
     defined.  On most machines, this should be the same as
     'PARM_BOUNDARY'.

with no mentions about its type and bounds.  However, at some point, the value
of this macro gets assigned to the field "regno_pointer_align" of "struct
emit_status" which points to an "unsigned char", hence if STACK_BOUNDARY gets
bigger than 255, it will overflow...  Thankfully, the backend which defines the
highest value is microblaze with 128 < 255.

The assignment happens in "emit-rtl.c" through the REGNO_POINTER_ALIGN macro:

in function.h:
  #define REGNO_POINTER_ALIGN(REGNO) (crtl->emit.regno_pointer_align[REGNO])
in emit-rtl.cc:
  REGNO_POINTER_ALIGN (STACK_POINTER_REGNUM) = STACK_BOUNDARY;
  [...]
  REGNO_POINTER_ALIGN (VIRTUAL_OUTGOING_ARGS_REGNUM) = STACK_BOUNDARY;

Would it be possible to, either add an explicit bound to STACK_BOUNDARY in the
manual, and/or use an "unsigned int *" rather than and "unsigned char *" for
the field "regno_pointer_align".

Thanks,
Paul

Re: The macro STACK_BOUNDARY may overflow

[Jeff Law]

On 3/24/23 07:48, Paul Iannetta via Gcc wrote:
> Hi,
> 
> Currently, the macro STACK_BOUNDARY is defined as
> 
>    Macro: STACK_BOUNDARY
>       Define this macro to the minimum alignment enforced by hardware for
>       the stack pointer on this machine.  The definition is a C
>       expression for the desired alignment (measured in bits).  This
>       value is used as a default if 'PREFERRED_STACK_BOUNDARY' is not
>       defined.  On most machines, this should be the same as
>       'PARM_BOUNDARY'.
> 
> with no mentions about its type and bounds.  However, at some point, the value
> of this macro gets assigned to the field "regno_pointer_align" of "struct
> emit_status" which points to an "unsigned char", hence if STACK_BOUNDARY gets
> bigger than 255, it will overflow...  Thankfully, the backend which defines the
> highest value is microblaze with 128 < 255.
> 
> The assignment happens in "emit-rtl.c" through the REGNO_POINTER_ALIGN macro:
> 
> in function.h:
>    #define REGNO_POINTER_ALIGN(REGNO) (crtl->emit.regno_pointer_align[REGNO])
> in emit-rtl.cc:
>    REGNO_POINTER_ALIGN (STACK_POINTER_REGNUM) = STACK_BOUNDARY;
>    [...]
>    REGNO_POINTER_ALIGN (VIRTUAL_OUTGOING_ARGS_REGNUM) = STACK_BOUNDARY;
> 
> Would it be possible to, either add an explicit bound to STACK_BOUNDARY in the
> manual, and/or use an "unsigned int *" rather than and "unsigned char *" for
> the field "regno_pointer_align".
Feel free to send a suitable patch to gcc-patches.  THe alignment data 
isn't held in the RTX structures, so it's not critical that it be kept 
as small as possible.  We don't want to waste space, so an unsigned 
short might be better.  A char was good for 30 years, so we don't need 
to go crazy here.

The alternative would be to change the representation to store the log2 
of the alignment.  That would be a much larger change and would trade 
off runtime for memory consumption.  I would have suggested this 
approach if the data were in the RTX structures amd space at a premium.

While I do see a trend in processor design to reduce/remove the 
misalignment penalties (thus eliminating the driver for increasing data 
alignment), that's been primarily in high end designs.

jeff

Re: The macro STACK_BOUNDARY may overflow

[Paul Iannetta]

On Sat, Mar 25, 2023 at 10:28:02AM -0600, Jeff Law via Gcc wrote:
> On 3/24/23 07:48, Paul Iannetta via Gcc wrote:
> > Hi,
> > 
> > Currently, the macro STACK_BOUNDARY is defined as
> > 
> >    Macro: STACK_BOUNDARY
> >       Define this macro to the minimum alignment enforced by hardware for
> >       the stack pointer on this machine.  The definition is a C
> >       expression for the desired alignment (measured in bits).  This
> >       value is used as a default if 'PREFERRED_STACK_BOUNDARY' is not
> >       defined.  On most machines, this should be the same as
> >       'PARM_BOUNDARY'.
> > 
> > with no mentions about its type and bounds.  However, at some point, the value
> > of this macro gets assigned to the field "regno_pointer_align" of "struct
> > emit_status" which points to an "unsigned char", hence if STACK_BOUNDARY gets
> > bigger than 255, it will overflow...  Thankfully, the backend which defines the
> > highest value is microblaze with 128 < 255.
> > 
> > The assignment happens in "emit-rtl.c" through the REGNO_POINTER_ALIGN macro:
> > 
> > in function.h:
> >    #define REGNO_POINTER_ALIGN(REGNO) (crtl->emit.regno_pointer_align[REGNO])
> > in emit-rtl.cc:
> >    REGNO_POINTER_ALIGN (STACK_POINTER_REGNUM) = STACK_BOUNDARY;
> >    [...]
> >    REGNO_POINTER_ALIGN (VIRTUAL_OUTGOING_ARGS_REGNUM) = STACK_BOUNDARY;
> > 
> > Would it be possible to, either add an explicit bound to STACK_BOUNDARY in the
> > manual, and/or use an "unsigned int *" rather than and "unsigned char *" for
> > the field "regno_pointer_align".
> Feel free to send a suitable patch to gcc-patches.  THe alignment data isn't
> held in the RTX structures, so it's not critical that it be kept as small as
> possible.  We don't want to waste space, so an unsigned short might be
> better.  A char was good for 30 years, so we don't need to go crazy here.
> 
> The alternative would be to change the representation to store the log2 of
> the alignment.  That would be a much larger change and would trade off
> runtime for memory consumption.  I would have suggested this approach if the
> data were in the RTX structures amd space at a premium.
> 
> While I do see a trend in processor design to reduce/remove the misalignment
> penalties (thus eliminating the driver for increasing data alignment),
> that's been primarily in high end designs.
> 
> jeff

Hi,

Here is a patch that changes the type of the variable holding the
alignment to unsigned short for the reasons outlined above.  Should I
also mention the new upper bound for STACK_BOUNDARY in the
documentation or keep it silent?

Thanks,
Paul

---->8-------------------------------------------------------8<----
From: Paul Iannetta <piannetta@kalrayinc.com>
Date: Fri, 12 Jan 2024 10:18:34 +0100
Subject: [PATCH] make regno_pointer_align a unsigned short*

This changes allows to align values greater than 128 to be used for
alignment.

	* emit-rtl.cc (emit_status::ensure_regno_capacity): Change
	  unsigned char* to unsigned short*
	(init_emit): Likewise.
	* function.h (struct GTY): Likewise.
---
 gcc/emit-rtl.cc | 6 +++---
 gcc/function.h  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index 1856fa4884f..f0f0ad193b5 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -1231,9 +1231,9 @@ emit_status::ensure_regno_capacity ()
   while (reg_rtx_no >= new_size)
     new_size *= 2;
 
-  char *tmp = XRESIZEVEC (char, regno_pointer_align, new_size);
+  short *tmp = XRESIZEVEC (short, regno_pointer_align, new_size);
   memset (tmp + old_size, 0, new_size - old_size);
-  regno_pointer_align = (unsigned char *) tmp;
+  regno_pointer_align = (unsigned short *) tmp;
 
   rtx *new1 = GGC_RESIZEVEC (rtx, regno_reg_rtx, new_size);
   memset (new1 + old_size, 0, (new_size - old_size) * sizeof (rtx));
@@ -5972,7 +5972,7 @@ init_emit (void)
   crtl->emit.regno_pointer_align_length = LAST_VIRTUAL_REGISTER + 101;
 
   crtl->emit.regno_pointer_align
-    = XCNEWVEC (unsigned char, crtl->emit.regno_pointer_align_length);
+    = XCNEWVEC (unsigned short, crtl->emit.regno_pointer_align_length);
 
   regno_reg_rtx
     = ggc_cleared_vec_alloc<rtx> (crtl->emit.regno_pointer_align_length);
diff --git a/gcc/function.h b/gcc/function.h
index 2d775b877fc..c4a20060844 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -72,7 +72,7 @@ struct GTY(()) emit_status {
   /* Indexed by pseudo register number, if nonzero gives the known alignment
      for that pseudo (if REG_POINTER is set in x_regno_reg_rtx).
      Allocated in parallel with x_regno_reg_rtx.  */
-  unsigned char * GTY((skip)) regno_pointer_align;
+  unsigned short * GTY((skip)) regno_pointer_align;
 };