Re: [PATCH v5] Fix styles in aix-thread.c to follow GNU style code.

[Simon Marchi <simon.marchi@polymtl.ca>]

On 10/5/26 2:55 PM, Aditya Vidyadhar Kamath wrote:
> From: Aditya Vidyadhar Kamath <aditya.kamath1@ibm.com>
> 
> This patch makes changes in aix-thread.c to follow style in
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
> 
> I ran clang-format with GNU style as a starting point:
>   /home/clang+llvm-23.1.2-powerpc64-ibm-aix-7.2/bin/clang-format \
>     -style=GNU -i gdb/aix-thread.c
> 
> and then manually kept only the changes that match GDB's coding
> standards.

Thanks, I have a few comments below, but otherwise this LGTM.  You can
push with those fixed.  Just make sure that you only have whitespace
changes.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> The resulting changes include:
> 
> 1: wrapping lines exceeding 80 columns,
> 2: adding spaces before '(' where required,
> 3: converting one-line switch cases to the preferred GNU style,
> 4: fixing a few indentation issues,
> 5: Remove tab-alignment in #define's
> 6: Add space in sizeof() calls throughout.
> 7: Some double spaces removed.
> 8: Correct genuine misindents like in sync_threadlists (), add_thread_with_info ().
> 
> A few clang-format changes were not kept:
> 
> 1: brace placement changes for struct definitions,

Did you mean turning

struct pd_thread {
  pthdb_pthread_t pdtid;
  pthread_t pthid;
  pthdb_tid_t tid;
};

into

struct pd_thread
{
  pthdb_pthread_t pdtid;
  pthread_t pthid;
  pthdb_tid_t tid;
};

?  If so, it's a valid change, you should include it.

> 2: #include reordering alphabetically,
> 3: removal of blank lines separating logical sections,
> 4: some cast formatting changes.
> @@ -299,16 +331,16 @@ ptrace_check (int req, int id, int ret)
>  			req, id, ret, errno);
>  	  return ret == -1 ? 0 : 1;
>  	}
> -	break;
> -     case PTT_READ_VEC:
> -     case PTT_READ_VSX:
> -	if (debug_aix_thread)
> -	  gdb_printf (gdb_stdlog,
> -		      "ptrace (%d, %d) = %d (errno = %d)\n",
> -		      req, id, ret, errno);
> -	if (ret == -1)
> -	  return -1;
> -	break;
> +      break;
> +    case PTT_READ_VEC:
> +    case PTT_READ_VSX:
> +      if (debug_aix_thread)
> +	gdb_printf (gdb_stdlog,
> +		    "ptrace (%d, %d) = %d (errno = %d)\n",
> +		    req, id, ret, errno);

This can be re-wrapped to take just 2 lines (clang-format does it).

> @@ -473,49 +506,49 @@ pdc_read_regs (pthdb_user_t user_current_pid,
>  	  if (!ptrace64aix (PTT_READ_SPRS, tid,
>  			    (unsigned long) &sprs64, 0, NULL))
>  	    memset (&sprs64, 0, sizeof (sprs64));
> -	  memcpy (&context->msr, &sprs64, sizeof(sprs64));
> +	  memcpy (&context->msr, &sprs64, sizeof (sprs64));
>  	}
>        else
>  	{
>  	  if (!ptrace32 (PTT_READ_SPRS, tid, (uintptr_t) &sprs32, 0, NULL))
>  	    memset (&sprs32, 0, sizeof (sprs32));
> -	  memcpy (&context->msr, &sprs32, sizeof(sprs32));
> +	  memcpy (&context->msr, &sprs32, sizeof (sprs32));
>  	}
>      }
>  
>    /* vector registers.  */
>    __vmx_context_t vmx;
> -  if (__power_vmx() && (flags & PTHDB_FLAG_REGS))
> +  if (__power_vmx () && (flags & PTHDB_FLAG_REGS))
>      {
>        if (data->arch64)
>  	{
>  	  if (!ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0))
>  	    memset (&vmx, 0, sizeof (vmx));
> -	  memcpy (&context->vmx, &vmx, sizeof(__vmx_context_t));
> +	  memcpy (&context->vmx, &vmx, sizeof (__vmx_context_t));
>  	}
>        else
>  	{
>  	  if (!ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0))
>  	    memset (&vmx, 0, sizeof (vmx));
> -	   memcpy (&context->vmx, &vmx, sizeof(__vmx_context_t));
> +	   memcpy (&context->vmx, &vmx, sizeof (__vmx_context_t));

The indent on that line is wrong, clang-format should have fixed it... ?

> @@ -1480,9 +1515,10 @@ fill_altivec (const struct regcache *regcache, __vmx_context_t *vmx)
>    int regno;
>  
>    for (regno = 0; regno < ppc_num_vrs; regno++)
> -    if (REG_VALID == regcache->get_register_status (tdep->ppc_vr0_regnum + regno))
> +    if (REG_VALID == regcache->get_register_status
> +		       (tdep->ppc_vr0_regnum + regno))
>        regcache->raw_collect (tdep->ppc_vr0_regnum + regno,
> -				     &(vmx->__vr[regno]));
> +			       &(vmx->__vr[regno]));

The indent on this line seems still off.

> @@ -1860,55 +1897,55 @@ store_regs_kernel_thread (const struct regcache *regcache, int regno,
>  	}
>      }
>  
> -    /* Vector registers.  */
> -    if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1
> -	&& (regno == -1 || (regno >= tdep->ppc_vr0_regnum
> -	&& regno <= tdep->ppc_vrsave_regnum)))
> -      {
> -	__vmx_context_t vmx;
> -	if (__power_vmx())
> -	  {
> -	    if (data->arch64)
> -	      ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
> -	    else
> -	      ret = ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
> -	    if (ret > 0)
> -	      {
> -		fill_altivec(regcache, &vmx);
> -		if (data->arch64)
> -		  ret = ptrace64aix (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
> -		else
> -		  ret = ptrace32 (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
> -		if (ret < 0)
> -		  perror_with_name (_("Unable to store AltiVec register after read"));
> -	      }
> -	  }
> -      }
> +  /* Vector registers.  */
> +  if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1
> +      && (regno == -1 || (regno >= tdep->ppc_vr0_regnum
> +			 && regno <= tdep->ppc_vrsave_regnum)))

Indent of the last line seems off.

> +    {
> +      __vmx_context_t vmx;
> +      if (__power_vmx ())
> +	{
> +	  if (data->arch64)
> +	    ret = ptrace64aix (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
> +	  else
> +	    ret = ptrace32 (PTT_READ_VEC, tid, (long long) &vmx, 0, 0);
> +	  if (ret > 0)
> +	    {
> +	      fill_altivec (regcache, &vmx);
> +	      if (data->arch64)
> +		ret = ptrace64aix (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
> +	      else
> +		ret = ptrace32 (PTT_WRITE_VEC, tid, (long long) &vmx, 0, 0);
> +	      if (ret < 0)
> +		perror_with_name (_("Unable to store AltiVec register after read"));
> +	    }
> +	}
> +    }
>  
> -    /* VSX registers.  */
> -    if (tdep->ppc_vsr0_upper_regnum != -1 && (regno == -1
> -	|| (regno >=tdep->ppc_vsr0_upper_regnum
> -	&& regno < tdep->ppc_vsr0_upper_regnum + ppc_num_vshrs)))
> -      {
> -	__vsx_context_t vsx;
> -	if (__power_vsx())
> -	  {
> -	    if (data->arch64)
> -	      ret =  ptrace64aix (PTT_READ_VSX, tid, (long long) &vsx, 0, 0);
> -	    else
> -	      ret =  ptrace32 (PTT_READ_VSX, tid, (long long) &vsx, 0, 0);
> -	    if (ret > 0)
> -	      {
> -		fill_vsx (regcache, &vsx);
> -		if (data->arch64)
> -		  ret = ptrace64aix (PTT_WRITE_VSX, tid, (long long) &vsx, 0, 0);
> -		else
> -		  ret = ptrace32 (PTT_WRITE_VSX, tid, (long long) &vsx, 0, 0);
> -		if (ret < 0)
> -		  perror_with_name (_("Unable to store VSX register after read"));
> -	      }
> -	  }
> -      }
> +  /* VSX registers.  */
> +  if (tdep->ppc_vsr0_upper_regnum != -1
> +      && (regno == -1 || (regno >= tdep->ppc_vsr0_upper_regnum
> +			  && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_vshrs)))

This last line ends up too long, clang-format rewrites it like this,
which I like:

  if (tdep->ppc_vsr0_upper_regnum != -1
      && (regno == -1
	  || (regno >= tdep->ppc_vsr0_upper_regnum
	      && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_vshrs)))

It also indents an if a few lines above in a similar way, if you want to
have a consistent formatting.

Simon