Re: The macro STACK_BOUNDARY may overflow

[Paul Iannetta <piannetta@kalrayinc.com>]

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;
 };