Re: [PATCH 1/3] RISC-V: gdb.base/gnu_vector fixes.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Jim Wilson <jimw@sifive.com> [2018-11-06 13:44:03 -0800]:

> Ensure that stack slots are always the same size as XLEN by rounding up arg
> sizes when computing the address of the next stack slot.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_assign_stack_location): New arg slot_align.
> 	Use with align_up when setting arg_offset.
> 	(riscv_call_arg_scalar_int): Call riscv_assign_stack_location with
> 	cinfo->xlen as new arg.
> ---
>  gdb/riscv-tdep.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index db372e2163..ac4f2533f4 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1868,13 +1868,13 @@ riscv_assign_reg_location (struct riscv_arg_info::location *loc,
>  static void
>  riscv_assign_stack_location (struct riscv_arg_info::location *loc,
>  			     struct riscv_memory_offsets *memory,
> -			     int length, int align)
> +			     int length, int align, int slot_align)

I struggled to understand what the difference between align and
slot_align is in this patch, it feels like....

>  {
>    loc->loc_type = riscv_arg_info::location::on_stack;
>    memory->arg_offset
>      = align_up (memory->arg_offset, align);
>    loc->loc_data.offset = memory->arg_offset;
> -  memory->arg_offset += length;
> +  memory->arg_offset += align_up (length, slot_align);
>    loc->c_length = length;
>  
>    /* Offset is always 0, either we're the first location part, in which
> @@ -1919,7 +1919,7 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
>  				      cinfo->xlen, 0))
>  	riscv_assign_stack_location (&ainfo->argloc[1],
>  				     &cinfo->memory, cinfo->xlen,
> -				     cinfo->xlen);
> +				     cinfo->xlen, cinfo->xlen);
>      }
>    else
>      {
> @@ -1928,7 +1928,8 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
>        if (!riscv_assign_reg_location (&ainfo->argloc[0],
>  				      &cinfo->int_regs, len, 0))
>  	riscv_assign_stack_location (&ainfo->argloc[0],
> -				     &cinfo->memory, len, ainfo->align);
> +				     &cinfo->memory, len, ainfo->align,
> +				     cinfo->xlen);

.... you might be better off just passing a better value of align
through here.  Testing on gdb.base/gnu_vector.exp shows that doing
this fixes the same problem that your original patch fixes.

My revision is below, but I've only just kicked off a test run, so
there might be problems I'm not seeing.

Thanks,
Andrew


>  
>        if (len < ainfo->length)
>  	{
> @@ -1937,7 +1938,8 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
>  					  &cinfo->int_regs, len,
>  					  cinfo->xlen))
>  	    riscv_assign_stack_location (&ainfo->argloc[1],
> -					 &cinfo->memory, len, cinfo->xlen);
> +					 &cinfo->memory, len, cinfo->xlen,
> +					 cinfo->xlen);
>  	}
>      }
>  }
> -- 
> 2.17.1
> 


---

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index a0b2d1f5d7..243db12999 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1951,12 +1951,13 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
     }
   else
     {
-      int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length;
+      int len = std::min (ainfo->length, cinfo->xlen);
+      int align = std::max (ainfo->align, cinfo->xlen);
 
       if (!riscv_assign_reg_location (&ainfo->argloc[0],
 				      &cinfo->int_regs, len, 0))
 	riscv_assign_stack_location (&ainfo->argloc[0],
-				     &cinfo->memory, len, ainfo->align);
+				     &cinfo->memory, len, align);
 
       if (len < ainfo->length)
 	{