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

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

[Jim Wilson]

This set of patches fixes these infcall related failures on a rv64gc
linux system.
FAIL: gdb.base/gnu_vector.exp: call add_many_charvecs
FAIL: gdb.base/gnu_vector.exp: call add_various_floatvecs

The first one fails because we have 2 4-byte vector args passed in the
same 8-byte stack slot.  The code is failing round up the address of a
stack slot when an arg is smaller than the stack slot size.

The second one fails because of two problems.  The 16-byte vector of
floats is given 4 byte alignment when it should have 16 byte
alignment. This is because there is no support for vector types.  And
unnamed args that require twice XLEN alignment are supposed to be
passed in aligned register pairs.  The support for this is also
missing.

This was tested on a rv64gc linux system with the gdb testsuite, and
it fixes 2 failures without causing any regressions.

Jim

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

[Jim Wilson]

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)
 {
   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);
 
       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

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

[Jim Wilson]

Unnamed arguments with 2*XLEN alignment are passed in aligned register pairs.

	gdb/
	* riscv-tdep.c (struct riscv_arg_info): New field is_unnamed.
	(riscv_call_arg_scalar_int): If unnamed arg with twice xlen alignment,
	then increment next_regnum if odd.
	(riscv_arg_location): New arg is_unnamed.  Set ainfo->is_unnamed.
	(riscv_push_dummy_call): New local ftype.  Call check_typedef to set
	function type.  Pass new arg to riscv_arg_location based on function
	type.
	(riscv_return_value): Pass new arg to riscv_arg_location.
---
 gdb/riscv-tdep.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 3d4f7e3dcc..93310c329f 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1737,6 +1737,9 @@ struct riscv_arg_info
        then this offset will be set to 0.  */
     int c_offset;
   } argloc[2];
+
+  /* TRUE if this is an unnamed argument.  */
+  bool is_unnamed;
 };
 
 /* Information about a set of registers being used for passing arguments as
@@ -1932,6 +1935,12 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
     {
       int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length;
 
+      /* Unnamed arguments in registers that require 2*XLEN alignment are
+	 passed in an aligned register pair.  */
+      if (ainfo->is_unnamed && (ainfo->align == cinfo->xlen * 2)
+	  && cinfo->int_regs.next_regnum & 1)
+	cinfo->int_regs.next_regnum++;
+
       if (!riscv_assign_reg_location (&ainfo->argloc[0],
 				      &cinfo->int_regs, len, 0))
 	riscv_assign_stack_location (&ainfo->argloc[0],
@@ -2222,11 +2231,12 @@ static void
 riscv_arg_location (struct gdbarch *gdbarch,
 		    struct riscv_arg_info *ainfo,
 		    struct riscv_call_info *cinfo,
-		    struct type *type)
+		    struct type *type, bool is_unnamed)
 {
   ainfo->type = type;
   ainfo->length = TYPE_LENGTH (ainfo->type);
   ainfo->align = riscv_type_alignment (ainfo->type);
+  ainfo->is_unnamed = is_unnamed;
   ainfo->contents = nullptr;
 
   switch (TYPE_CODE (ainfo->type))
@@ -2375,6 +2385,11 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
 
   CORE_ADDR osp = sp;
 
+  struct type *ftype = check_typedef (value_type (function));
+
+  if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
+    ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
+
   /* We'll use register $a0 if we're returning a struct.  */
   if (struct_return)
     ++call_info.int_regs.next_regnum;
@@ -2388,7 +2403,8 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
       arg_value = args[i];
       arg_type = check_typedef (value_type (arg_value));
 
-      riscv_arg_location (gdbarch, info, &call_info, arg_type);
+      riscv_arg_location (gdbarch, info, &call_info, arg_type,
+			  TYPE_VARARGS (ftype) && i >= TYPE_NFIELDS (ftype));
 
       if (info->type != arg_type)
 	arg_value = value_cast (info->type, arg_value);
@@ -2565,7 +2581,7 @@ riscv_return_value (struct gdbarch  *gdbarch,
   struct type *arg_type;
 
   arg_type = check_typedef (type);
-  riscv_arg_location (gdbarch, &info, &call_info, arg_type);
+  riscv_arg_location (gdbarch, &info, &call_info, arg_type, false);
 
   if (riscv_debug_infcall > 0)
     {
-- 
2.17.1

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

[Jim Wilson]

GCC gives vectors natural aligment based on total size, not element size,
bounded by the maximum supported type alignment.

	gdb/
	* riscv-tdep.c (BIGGEST_ALIGNMENT): New.
	(riscv_type_alignment) <TYPE_CODE_ARRAY>: If TYPE_VECTOR, return min
	of TYPE_LENGTH and BIGGEST_ALIGNMENT.
---
 gdb/riscv-tdep.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index ac4f2533f4..3d4f7e3dcc 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -59,6 +59,9 @@
 /* The stack must be 16-byte aligned.  */
 #define SP_ALIGNMENT 16
 
+/* The biggest alignment that the target supports.  */
+#define BIGGEST_ALIGNMENT 16
+
 /* Forward declarations.  */
 static bool riscv_has_feature (struct gdbarch *gdbarch, char feature);
 
@@ -1640,6 +1643,10 @@ riscv_type_alignment (struct type *t)
       return TYPE_LENGTH (t);
 
     case TYPE_CODE_ARRAY:
+      if (TYPE_VECTOR (t))
+	return std::min (TYPE_LENGTH (t), (unsigned)BIGGEST_ALIGNMENT);
+      /* FALLTHROUGH */
+
     case TYPE_CODE_COMPLEX:
       return riscv_type_alignment (TYPE_TARGET_TYPE (t));
 
-- 
2.17.1

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

[Andrew Burgess]

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

> Unnamed arguments with 2*XLEN alignment are passed in aligned register pairs.
> 
> 	gdb/
> 	* riscv-tdep.c (struct riscv_arg_info): New field is_unnamed.
> 	(riscv_call_arg_scalar_int): If unnamed arg with twice xlen alignment,
> 	then increment next_regnum if odd.
> 	(riscv_arg_location): New arg is_unnamed.  Set ainfo->is_unnamed.
> 	(riscv_push_dummy_call): New local ftype.  Call check_typedef to set
> 	function type.  Pass new arg to riscv_arg_location based on function
> 	type.
> 	(riscv_return_value): Pass new arg to riscv_arg_location.

Could you change the titles for the 3 commits in this series please to
make them more descriptive of the fix in each commit.  This also
avoids having 3 commits in a row with the same title line, which can
look like a mistake in 'git log --format=oneline'.





> ---
>  gdb/riscv-tdep.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 3d4f7e3dcc..93310c329f 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1737,6 +1737,9 @@ struct riscv_arg_info
>         then this offset will be set to 0.  */
>      int c_offset;
>    } argloc[2];
> +
> +  /* TRUE if this is an unnamed argument.  */
> +  bool is_unnamed;
>  };
>  
>  /* Information about a set of registers being used for passing arguments as
> @@ -1932,6 +1935,12 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
>      {
>        int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length;
>  
> +      /* Unnamed arguments in registers that require 2*XLEN alignment are
> +	 passed in an aligned register pair.  */
> +      if (ainfo->is_unnamed && (ainfo->align == cinfo->xlen * 2)
> +	  && cinfo->int_regs.next_regnum & 1)
> +	cinfo->int_regs.next_regnum++;
> +
>        if (!riscv_assign_reg_location (&ainfo->argloc[0],
>  				      &cinfo->int_regs, len, 0))
>  	riscv_assign_stack_location (&ainfo->argloc[0],
> @@ -2222,11 +2231,12 @@ static void
>  riscv_arg_location (struct gdbarch *gdbarch,
>  		    struct riscv_arg_info *ainfo,
>  		    struct riscv_call_info *cinfo,
> -		    struct type *type)
> +		    struct type *type, bool is_unnamed)

Could you update the functions header comment to explain what impact
IS_UNNAMED will have on the behaviour of the function.

>  {
>    ainfo->type = type;
>    ainfo->length = TYPE_LENGTH (ainfo->type);
>    ainfo->align = riscv_type_alignment (ainfo->type);
> +  ainfo->is_unnamed = is_unnamed;
>    ainfo->contents = nullptr;
>  
>    switch (TYPE_CODE (ainfo->type))
> @@ -2375,6 +2385,11 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
>  
>    CORE_ADDR osp = sp;
>  
> +  struct type *ftype = check_typedef (value_type (function));
> +
> +  if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
> +    ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
> +
>    /* We'll use register $a0 if we're returning a struct.  */
>    if (struct_return)
>      ++call_info.int_regs.next_regnum;
> @@ -2388,7 +2403,8 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
>        arg_value = args[i];
>        arg_type = check_typedef (value_type (arg_value));
>  
> -      riscv_arg_location (gdbarch, info, &call_info, arg_type);
> +      riscv_arg_location (gdbarch, info, &call_info, arg_type,
> +			  TYPE_VARARGS (ftype) && i >= TYPE_NFIELDS (ftype));
>  
>        if (info->type != arg_type)
>  	arg_value = value_cast (info->type, arg_value);
> @@ -2565,7 +2581,7 @@ riscv_return_value (struct gdbarch  *gdbarch,
>    struct type *arg_type;
>  
>    arg_type = check_typedef (type);
> -  riscv_arg_location (gdbarch, &info, &call_info, arg_type);
> +  riscv_arg_location (gdbarch, &info, &call_info, arg_type, false);
>  
>    if (riscv_debug_infcall > 0)
>      {
> -- 
> 2.17.1
> 

Otherwise looks fine.

Thanks,
Andrew

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

[Andrew Burgess]

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

> GCC gives vectors natural aligment based on total size, not element size,
> bounded by the maximum supported type alignment.
> 
> 	gdb/
> 	* riscv-tdep.c (BIGGEST_ALIGNMENT): New.
> 	(riscv_type_alignment) <TYPE_CODE_ARRAY>: If TYPE_VECTOR, return min
> 	of TYPE_LENGTH and BIGGEST_ALIGNMENT.

As with patch #1, please update the patch title line.

> ---
>  gdb/riscv-tdep.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index ac4f2533f4..3d4f7e3dcc 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -59,6 +59,9 @@
>  /* The stack must be 16-byte aligned.  */
>  #define SP_ALIGNMENT 16
>  
> +/* The biggest alignment that the target supports.  */
> +#define BIGGEST_ALIGNMENT 16
> +
>  /* Forward declarations.  */
>  static bool riscv_has_feature (struct gdbarch *gdbarch, char feature);
>  
> @@ -1640,6 +1643,10 @@ riscv_type_alignment (struct type *t)
>        return TYPE_LENGTH (t);
>  
>      case TYPE_CODE_ARRAY:
> +      if (TYPE_VECTOR (t))
> +	return std::min (TYPE_LENGTH (t), (unsigned)BIGGEST_ALIGNMENT);

I think there's supposed to be a space after the cast, '(unsigned) BIGGEST_ALIGNMENT'.

> +      /* FALLTHROUGH */
> +
>      case TYPE_CODE_COMPLEX:
>        return riscv_type_alignment (TYPE_TARGET_TYPE (t));
>  
> -- 
> 2.17.1
> 

Otherwise, look fine.

Thanks,
Andrew

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

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2018-11-08 14:34:11 +0000]:

> * Jim Wilson <jimw@sifive.com> [2018-11-06 13:44:46 -0800]:
> 
> > Unnamed arguments with 2*XLEN alignment are passed in aligned register pairs.
> > 
> > 	gdb/
> > 	* riscv-tdep.c (struct riscv_arg_info): New field is_unnamed.
> > 	(riscv_call_arg_scalar_int): If unnamed arg with twice xlen alignment,
> > 	then increment next_regnum if odd.
> > 	(riscv_arg_location): New arg is_unnamed.  Set ainfo->is_unnamed.
> > 	(riscv_push_dummy_call): New local ftype.  Call check_typedef to set
> > 	function type.  Pass new arg to riscv_arg_location based on function
> > 	type.
> > 	(riscv_return_value): Pass new arg to riscv_arg_location.
> 
> Could you change the titles for the 3 commits in this series please to
> make them more descriptive of the fix in each commit.  This also
> avoids having 3 commits in a row with the same title line, which can
> look like a mistake in 'git log --format=oneline'.

Also can you make sure the commit message for each patch includes
details about which tests you expect to go from fail to pass, and on
which RISC-V variants you expect to see improvements.

Thanks,
Andrew


> 
> 
> 
> 
> 
> > ---
> >  gdb/riscv-tdep.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> > index 3d4f7e3dcc..93310c329f 100644
> > --- a/gdb/riscv-tdep.c
> > +++ b/gdb/riscv-tdep.c
> > @@ -1737,6 +1737,9 @@ struct riscv_arg_info
> >         then this offset will be set to 0.  */
> >      int c_offset;
> >    } argloc[2];
> > +
> > +  /* TRUE if this is an unnamed argument.  */
> > +  bool is_unnamed;
> >  };
> >  
> >  /* Information about a set of registers being used for passing arguments as
> > @@ -1932,6 +1935,12 @@ riscv_call_arg_scalar_int (struct riscv_arg_info *ainfo,
> >      {
> >        int len = (ainfo->length > cinfo->xlen) ? cinfo->xlen : ainfo->length;
> >  
> > +      /* Unnamed arguments in registers that require 2*XLEN alignment are
> > +	 passed in an aligned register pair.  */
> > +      if (ainfo->is_unnamed && (ainfo->align == cinfo->xlen * 2)
> > +	  && cinfo->int_regs.next_regnum & 1)
> > +	cinfo->int_regs.next_regnum++;
> > +
> >        if (!riscv_assign_reg_location (&ainfo->argloc[0],
> >  				      &cinfo->int_regs, len, 0))
> >  	riscv_assign_stack_location (&ainfo->argloc[0],
> > @@ -2222,11 +2231,12 @@ static void
> >  riscv_arg_location (struct gdbarch *gdbarch,
> >  		    struct riscv_arg_info *ainfo,
> >  		    struct riscv_call_info *cinfo,
> > -		    struct type *type)
> > +		    struct type *type, bool is_unnamed)
> 
> Could you update the functions header comment to explain what impact
> IS_UNNAMED will have on the behaviour of the function.
> 
> >  {
> >    ainfo->type = type;
> >    ainfo->length = TYPE_LENGTH (ainfo->type);
> >    ainfo->align = riscv_type_alignment (ainfo->type);
> > +  ainfo->is_unnamed = is_unnamed;
> >    ainfo->contents = nullptr;
> >  
> >    switch (TYPE_CODE (ainfo->type))
> > @@ -2375,6 +2385,11 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
> >  
> >    CORE_ADDR osp = sp;
> >  
> > +  struct type *ftype = check_typedef (value_type (function));
> > +
> > +  if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
> > +    ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
> > +
> >    /* We'll use register $a0 if we're returning a struct.  */
> >    if (struct_return)
> >      ++call_info.int_regs.next_regnum;
> > @@ -2388,7 +2403,8 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
> >        arg_value = args[i];
> >        arg_type = check_typedef (value_type (arg_value));
> >  
> > -      riscv_arg_location (gdbarch, info, &call_info, arg_type);
> > +      riscv_arg_location (gdbarch, info, &call_info, arg_type,
> > +			  TYPE_VARARGS (ftype) && i >= TYPE_NFIELDS (ftype));
> >  
> >        if (info->type != arg_type)
> >  	arg_value = value_cast (info->type, arg_value);
> > @@ -2565,7 +2581,7 @@ riscv_return_value (struct gdbarch  *gdbarch,
> >    struct type *arg_type;
> >  
> >    arg_type = check_typedef (type);
> > -  riscv_arg_location (gdbarch, &info, &call_info, arg_type);
> > +  riscv_arg_location (gdbarch, &info, &call_info, arg_type, false);
> >  
> >    if (riscv_debug_infcall > 0)
> >      {
> > -- 
> > 2.17.1
> > 
> 
> Otherwise looks fine.
> 
> Thanks,
> Andrew

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

[Andrew Burgess]

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

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

[Jim Wilson]

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