Re: [XEN PATCH] xen/spinlock: mechanically rename parameter name 'debug'

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 21 Jul 2023, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> To avoid any confusion resulting from the parameter 'debug'
> hiding the homonymous function declared at
> 'xen/arch/x86/include/asm/processor.h:428'
> the rename of parameters s/debug/dbg is performed.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> x86 maintainers CC'ed because the violation is caused by a declaration
> in an x86 file, but I reckon it would harm understandability if
> a function was renamed to 'dbg'
> ---
>  xen/common/spinlock.c      | 38 +++++++++++++++++++-------------------
>  xen/include/xen/spinlock.h |  6 +++---
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> index 7f453234a9..d8d2e6ad1a 100644
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -78,7 +78,7 @@ static int __init cf_check lockdebug_init(void)
>  }
>  presmp_initcall(lockdebug_init);
>  
> -void check_lock(union lock_debug *debug, bool try)
> +void check_lock(union lock_debug *dbg, bool try)
>  {
>      bool irq_safe = !local_irq_is_enabled();
>      unsigned int cpu = smp_processor_id();
> @@ -118,12 +118,12 @@ void check_lock(union lock_debug *debug, bool try)
>      if ( try && irq_safe )
>          return;
>  
> -    if ( unlikely(debug->irq_safe != irq_safe) )
> +    if ( unlikely(dbg->irq_safe != irq_safe) )
>      {
>          union lock_debug seen, new = { 0 };
>  
>          new.irq_safe = irq_safe;
> -        seen.val = cmpxchg(&debug->val, LOCK_DEBUG_INITVAL, new.val);
> +        seen.val = cmpxchg(&dbg->val, LOCK_DEBUG_INITVAL, new.val);
>  
>          if ( !seen.unseen && seen.irq_safe == !irq_safe )
>          {
> @@ -137,14 +137,14 @@ void check_lock(union lock_debug *debug, bool try)
>          return;
>  
>      for ( i = 0; i < nr_taken; i++ )
> -        if ( taken[i] == debug )
> +        if ( taken[i] == dbg )
>          {
> -            printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", 
> debug);
> +            printk("CHECKLOCK FAILURE: lock at %p taken recursively\n", dbg);
>              BUG();
>          }
>  }
>  
> -static void check_barrier(union lock_debug *debug)
> +static void check_barrier(union lock_debug *dbg)
>  {
>      if ( unlikely(atomic_read(&spin_debug) <= 0) )
>          return;
> @@ -160,10 +160,10 @@ static void check_barrier(union lock_debug *debug)
>       * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
>       * is clearly wrong, for the same reason outlined in check_lock() above.
>       */
> -    BUG_ON(!local_irq_is_enabled() && !debug->irq_safe);
> +    BUG_ON(!local_irq_is_enabled() && !dbg->irq_safe);
>  }
>  
> -void lock_enter(const union lock_debug *debug)
> +void lock_enter(const union lock_debug *dbg)
>  {
>      unsigned int cpu = smp_processor_id();
>      const union lock_debug **taken = per_cpu(locks_taken, cpu);
> @@ -176,7 +176,7 @@ void lock_enter(const union lock_debug *debug)
>      local_irq_save(flags);
>  
>      if ( *nr_taken < lock_depth_size )
> -        taken[(*nr_taken)++] = debug;
> +        taken[(*nr_taken)++] = dbg;
>      else if ( !max_depth_reached )
>      {
>          max_depth_reached = true;
> @@ -187,7 +187,7 @@ void lock_enter(const union lock_debug *debug)
>      local_irq_restore(flags);
>  }
>  
> -void lock_exit(const union lock_debug *debug)
> +void lock_exit(const union lock_debug *dbg)
>  {
>      unsigned int cpu = smp_processor_id();
>      const union lock_debug **taken = per_cpu(locks_taken, cpu);
> @@ -202,7 +202,7 @@ void lock_exit(const union lock_debug *debug)
>  
>      for ( i = *nr_taken; i > 0; i-- )
>      {
> -        if ( taken[i - 1] == debug )
> +        if ( taken[i - 1] == dbg )
>          {
>              memmove(taken + i - 1, taken + i,
>                      (*nr_taken - i) * sizeof(*taken));
> @@ -217,28 +217,28 @@ void lock_exit(const union lock_debug *debug)
>  
>      if ( !max_depth_reached )
>      {
> -        printk("CHECKLOCK released lock at %p not recorded!\n", debug);
> +        printk("CHECKLOCK released lock at %p not recorded!\n", dbg);
>          WARN();
>      }
>  
>      local_irq_restore(flags);
>  }
>  
> -static void got_lock(union lock_debug *debug)
> +static void got_lock(union lock_debug *dbg)
>  {
> -    debug->cpu = smp_processor_id();
> +    dbg->cpu = smp_processor_id();
>  
> -    lock_enter(debug);
> +    lock_enter(dbg);
>  }
>  
> -static void rel_lock(union lock_debug *debug)
> +static void rel_lock(union lock_debug *dbg)
>  {
>      if ( atomic_read(&spin_debug) > 0 )
> -        BUG_ON(debug->cpu != smp_processor_id());
> +        BUG_ON(dbg->cpu != smp_processor_id());
>  
> -    lock_exit(debug);
> +    lock_exit(dbg);
>  
> -    debug->cpu = SPINLOCK_NO_CPU;
> +    dbg->cpu = SPINLOCK_NO_CPU;
>  }
>  
>  void spin_debug_enable(void)
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index 0a02a527dc..d303c56f8a 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -22,9 +22,9 @@ union lock_debug {
>      };
>  };
>  #define _LOCK_DEBUG { LOCK_DEBUG_INITVAL }
> -void check_lock(union lock_debug *debug, bool try);
> -void lock_enter(const union lock_debug *debug);
> -void lock_exit(const union lock_debug *debug);
> +void check_lock(union lock_debug *dbg, bool try);
> +void lock_enter(const union lock_debug *dbg);
> +void lock_exit(const union lock_debug *dbg);
>  void spin_debug_enable(void);
>  void spin_debug_disable(void);
>  #else
> -- 
> 2.34.1
>