Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

The declaration of local variable 'cpuid_leaf' causes
shadowing with the homonymous function to happen, therefore
the variable is renamed to avoid this.

Local variable 'cr4' that shadows a previous declaration is
removed, as it is unnecessary and doing so does not alter the
semantics.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 2de1be7996..9403beb20f 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1483,7 +1483,7 @@ x86_emulate(
     {
         enum x86_segment seg;
         struct segment_register cs, sreg;
-        struct cpuid_leaf cpuid_leaf;
+        struct cpuid_leaf res;
         uint64_t msr_val;
         unsigned int i, n;
         unsigned long dummy;
@@ -5024,13 +5024,13 @@ x86_emulate(
         generate_exception_if((msr_val & MSR_MISC_FEATURES_CPUID_FAULTING),
                               X86_EXC_GP, 0); /* Faulting active? (Inc. CPL test) */
 
-        rc = ops->cpuid(_regs.eax, _regs.ecx, &cpuid_leaf, ctxt);
+        rc = ops->cpuid(_regs.eax, _regs.ecx, &res, ctxt);
         if ( rc != X86EMUL_OKAY )
             goto done;
-        _regs.r(ax) = cpuid_leaf.a;
-        _regs.r(bx) = cpuid_leaf.b;
-        _regs.r(cx) = cpuid_leaf.c;
-        _regs.r(dx) = cpuid_leaf.d;
+        _regs.r(ax) = res.a;
+        _regs.r(bx) = res.b;
+        _regs.r(cx) = res.c;
+        _regs.r(dx) = res.d;
         break;
 
     case X86EMUL_OPC(0x0f, 0xa3): bt: /* bt */
@@ -8408,8 +8408,6 @@ x86_emulate(
         generate_exception(X86_EXC_MF);
     if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
     {
-        unsigned long cr4;
-
         if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
             cr4 = X86_CR4_OSXMMEXCPT;
         generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);
-- 
2.34.1

On 27.07.2023 12:48, Nicola Vetrini wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1483,7 +1483,7 @@ x86_emulate(
>      {
>          enum x86_segment seg;
>          struct segment_register cs, sreg;
> -        struct cpuid_leaf cpuid_leaf;
> +        struct cpuid_leaf res;

This is too generic a name for a variable with a scope of several
thousand lines. Perhaps just "leaf"?

> @@ -8408,8 +8408,6 @@ x86_emulate(
>          generate_exception(X86_EXC_MF);
>      if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
>      {
> -        unsigned long cr4;
> -
>          if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>              cr4 = X86_CR4_OSXMMEXCPT;
>          generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);

This change looks okay to me, but I'd like to strongly encourage
you to split both changes. They're of different nature, and for
the latter it may even be worthwhile pointing out when exactly
this duplication of variables was introduced (it clearly would
better have been avoided).

Jan

On 27/07/23 17:41, Jan Beulich wrote:
> On 27.07.2023 12:48, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1483,7 +1483,7 @@ x86_emulate(
>>       {
>>           enum x86_segment seg;
>>           struct segment_register cs, sreg;
>> -        struct cpuid_leaf cpuid_leaf;
>> +        struct cpuid_leaf res;
> 
> This is too generic a name for a variable with a scope of several
> thousand lines. Perhaps just "leaf"?

It can also be defined inside the switch clause, since it has no other 
purpose than store a result.

> 
>> @@ -8408,8 +8408,6 @@ x86_emulate(
>>           generate_exception(X86_EXC_MF);
>>       if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
>>       {
>> -        unsigned long cr4;
>> -
>>           if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>>               cr4 = X86_CR4_OSXMMEXCPT;
>>           generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);
> 
> This change looks okay to me, but I'd like to strongly encourage
> you to split both changes. They're of different nature, and for
> the latter it may even be worthwhile pointing out when exactly
> this duplication of variables was introduced (it clearly would
> better have been avoided).
> 

I did it this way because they are the only violations of R5.3 left in 
this file (among those not subject to deviation). By splitting you mean 
two patches in this series or a separate patch just for this change?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

On 27.07.2023 17:58, Nicola Vetrini wrote:
> 
> 
> On 27/07/23 17:41, Jan Beulich wrote:
>> On 27.07.2023 12:48, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -1483,7 +1483,7 @@ x86_emulate(
>>>       {
>>>           enum x86_segment seg;
>>>           struct segment_register cs, sreg;
>>> -        struct cpuid_leaf cpuid_leaf;
>>> +        struct cpuid_leaf res;
>>
>> This is too generic a name for a variable with a scope of several
>> thousand lines. Perhaps just "leaf"?
> 
> It can also be defined inside the switch clause, since it has no other 
> purpose than store a result.

That would be more code churn, though.

>>> @@ -8408,8 +8408,6 @@ x86_emulate(
>>>           generate_exception(X86_EXC_MF);
>>>       if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
>>>       {
>>> -        unsigned long cr4;
>>> -
>>>           if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>>>               cr4 = X86_CR4_OSXMMEXCPT;
>>>           generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);
>>
>> This change looks okay to me, but I'd like to strongly encourage
>> you to split both changes. They're of different nature, and for
>> the latter it may even be worthwhile pointing out when exactly
>> this duplication of variables was introduced (it clearly would
>> better have been avoided).
>>
> 
> I did it this way because they are the only violations of R5.3 left in 
> this file (among those not subject to deviation). By splitting you mean 
> two patches in this series or a separate patch just for this change?

Separate or within a series doesn't matter. Just preferably not in
the same patch. (And btw, if you split larger patches more, some of
your changes may also go in more quickly. Yet of course this shouldn't
get too fine grained.)

Jan