Re: [PATCH] Fix complex argument handling in ppc64 dummy function call

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, February 28 2013, Tiago StÃrmer Daitx wrote:

> Handle complex arguments for dummy function call on PPC64. I refactored the
> code to extract the float logic into a function to reuse it for complex 
> arguments as well - ABI defines that complex arguments are to be handled as 
> if 2 float arguments were given.

Thanks for the patch, Tiago.  I have only formatting comments.

> gdb/ChangeLog
> 2013-02-05  Tiago Sturmer Daitx  <tdaitx@linux.vnet.ibm.com>

Don't forget to update the date if your patch is accepted :-).

> 	* ppc-sysv-tdep.c (ppc64_sysv_abi_push_dummy_call): Handle complex 
> 	arguments.
> 	* ppc-sysv-tdep.c (ppc64_sysv_abi_push_float): New functions to handle 
> 	float arguments.

You don't need multiple entries for the same file.  The order of the
functions is reverse (it's not a big issue, but I try to write my
ChangeLog entries following the order of the modifications in the
patch).  You can also just write "New function." for the
`ppc64_sysv_abi_push_float'.  So your ChangeLog would be something like:

	* ppc-sysv-tdep.c (ppc64_sysv_abi_push_float): New function.
	(ppc64_sysv_abi_push_dummy_call): Handle complex arguments.

> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 0ffeab9..690d65d 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -1101,6 +1101,76 @@ convert_code_addr_to_desc_addr (CORE_ADDR code_addr, CORE_ADDR *desc_addr)
>    return 1;
>  }
>  
> +static void
> +ppc64_sysv_abi_push_float (struct gdbarch *gdbarch, struct regcache *regcache,
> +			   struct gdbarch_tdep *tdep, struct type *type, 
> +			   const bfd_byte *val, int freg, int greg,
> +			   CORE_ADDR gparam)
> +{

Since this is a new function, it needs a comment describing it.  Oh, and
there should be a blank line between the comment and the function
declaration :-).

> +      /* Floats and Doubles go in f1 .. f13.  They also consume a left aligned 
> +	 GREG, and can end up in memory.  */
> +      if (freg <= 13)
> +	{
> +	  struct type *regtype;
> +	  regtype = register_type (gdbarch, tdep->ppc_fp0_regnum + freg);

I know you've just made a copy-and-paste here, but it's a standard to
put a blank line between the declaration of the variable(s) and the
actual code.

> @@ -1276,36 +1302,55 @@ ppc64_sysv_abi_push_dummy_call (struct gdbarch *gdbarch,
>  		   && (gdbarch_long_double_format (gdbarch)
>  		       == floatformats_ibm_long_double))
>  	    {
> -	      /* IBM long double stored in two doublewords of the
> -		 parameter save area and corresponding registers.  */
>  	      if (write_pass)
> -		{
> -		  if (!tdep->soft_float && freg <= 13)
> -		    {
> -		      regcache_cooked_write (regcache,
> -                                             tdep->ppc_fp0_regnum + freg,
> -					     val);
> -		      if (freg <= 12)
> -			regcache_cooked_write (regcache,
> -					       tdep->ppc_fp0_regnum + freg + 1,
> -					       val + 8);
> -		    }
> -		  if (greg <= 10)
> -		    {
> -		      regcache_cooked_write (regcache,
> -					     tdep->ppc_gp0_regnum + greg,
> -					     val);
> -		      if (greg <= 9)
> -			regcache_cooked_write (regcache,
> -					       tdep->ppc_gp0_regnum + greg + 1,
> -					       val + 8);
> -		    }
> -		  write_memory (gparam, val, TYPE_LENGTH (type));
> -		}
> +		ppc64_sysv_abi_push_float (gdbarch, regcache, tdep, type, 

Spurious space at the end of the line.

> +					   val, freg, greg, gparam);
[...]
> +          else if (TYPE_CODE (type) == TYPE_CODE_COMPLEX
> +              && (TYPE_LENGTH (type) == 8 || TYPE_LENGTH (type) == 16))
> +            {
> +              int i;

Same comment as above: blank line between variable declaration and code.

> +              for (i = 0; i < 2; i++)
> +                {
> +                  if (write_pass)
> +		    {
> +		      struct type *target_type;

Likewise.

> +		      target_type = check_typedef (TYPE_TARGET_TYPE (type));
> +		      ppc64_sysv_abi_push_float (gdbarch, regcache, tdep, 

Spurious space at the end of the line.

> +						 target_type, val + i * 

Likewise.

> +						 TYPE_LENGTH (target_type),
> +						 freg, greg, gparam);
> +		    }
> +                  freg++;
> +                  greg++;
> +                  /* Always consume parameter stack space.  */
> +                  gparam = align_up(gparam + 8, tdep->wordsize);

Space between function name and open parenthesis.

> +                }
> +            }
> +          else if (TYPE_CODE (type) == TYPE_CODE_COMPLEX
> +                   && TYPE_LENGTH (type) == 32
> +                   && (gdbarch_long_double_format (gdbarch)
> +                       == floatformats_ibm_long_double))
> +            {
> +              int i;
> +              for (i = 0; i < 2; i++)
> +                {
> +		  struct type *target_type;
> +		  target_type = check_typedef (TYPE_TARGET_TYPE (type));
> +                  if (write_pass)

This "if" is indented only with spaces (not using GNU style).

> +		    ppc64_sysv_abi_push_float (gdbarch, regcache, tdep, 
> +					       target_type, val + i * 

Spurious space at the end of both lines.

> +					       TYPE_LENGTH (target_type),
> +					       freg, greg, gparam);
> +                  freg += 2;
> +                  greg += 2;
> +                  gparam = align_up (gparam + TYPE_LENGTH (target_type),
> +                                     tdep->wordsize);
> +                }
> +            }
>  	  else if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT
>  		   && TYPE_LENGTH (type) <= 8)
>  	    {

Well, I guess that's it.  Sorry for the nitpicking, and thanks a lot for
the code refactoring :-).

-- 
Sergio