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

  * 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
> 


---
  

On Thu, Nov 8, 2018 at 7:43 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> I struggled to understand what the difference between align and
> slot_align is in this patch, it feels like....

If you have an argument twice the size of xlen, with alignment twice
the size of xlen, then when we pass the first word, align is 2*xlen
and slot_align is 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.

> -      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 you do this, and we keep the patch in riscv_assign_stack_location,
then we end up allocating 2*xlen bytes for the first xlen bytes of a
argument of size 2*len whcih is wrong.

I suppose you want to drop the riscv_assign_stack_location change too.
That could work, but that leaves arg_offset unaligned after allocating
the last argument.  Though it looks like the only problem with that is
some funny riscv_debug_infcall output.  It might say for instance that
we have 12 bytes of stack arguments for a 64-bit target, which doesn't
make any sense.  For a 64-bit target, stack arguments should always be
a multiple of 8 bytes.  Otherwise, this looks harmless, and this is a
problem that we already have, so this isn't anything broken by this
patch set.  I'll rewrite the patch this way.

Jim