The parameters renamed in the function declaration caused shadowing
with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
them also addresses Rule 8.3:
"All declarations of an object or function shall use the same names
and type qualifiers".

The local variable 'mask' is removed because it shadows the homonymous
variable defined in an outer scope, with no change to the semantics.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/include/asm/setup.h | 2 +-
 xen/arch/x86/setup.c             | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 51fce66607..b0e6a39e23 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -33,7 +33,7 @@ static inline void vesa_init(void) {};
 
 int construct_dom0(
     struct domain *d,
-    const module_t *kernel, unsigned long kernel_headroom,
+    const module_t *image, unsigned long image_headroom,
     module_t *initrd,
     const char *cmdline);
 void setup_io_bitmap(struct domain *d);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2dbe9857aa..80ae973d64 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         s = map_s;
         if ( s < map_e )
         {
-            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
-
+            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
             map_s = (s + mask) & ~mask;
             map_e &= ~mask;
             init_boot_pages(map_s, map_e);
-- 
2.34.1

On 04.08.2023 10:03, Nicola Vetrini wrote:
> The parameters renamed in the function declaration caused shadowing
> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
> them also addresses Rule 8.3:
> "All declarations of an object or function shall use the same names
> and type qualifiers".

Can you explain to me how shadowing can happen in a declaration? I
would focus on 8.3 here, and only mention the other name collision.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          s = map_s;
>          if ( s < map_e )
>          {
> -            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
> -
> +            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
>              map_s = (s + mask) & ~mask;
>              map_e &= ~mask;
>              init_boot_pages(map_s, map_e);

Re-using the outer scope variable is a little risky, don't you agree?
It just so happens that below here there's no further use requiring
the earlier value (PAGE_SIZE - 1). This isn't to say I'm opposed, but
it certainly needs evaluating with this in mind (mentioning of which
in the description would have demonstrated that you did consider this
aspect).

Jan

On 07/08/2023 15:05, Jan Beulich wrote:
> On 04.08.2023 10:03, Nicola Vetrini wrote:
>> The parameters renamed in the function declaration caused shadowing
>> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
>> them also addresses Rule 8.3:
>> "All declarations of an object or function shall use the same names
>> and type qualifiers".
> 
> Can you explain to me how shadowing can happen in a declaration? I
> would focus on 8.3 here, and only mention the other name collision.

There's "static struct file __initdata kernel;" in 
xen/common/efi/boot.c, which
is visible when the function is declared. Since renaming these parameter 
names would
have been addressed by Federico for R8.3 anyway, my intention was to 
address them both.

> 
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>          s = map_s;
>>          if ( s < map_e )
>>          {
>> -            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
>> -
>> +            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
>>              map_s = (s + mask) & ~mask;
>>              map_e &= ~mask;
>>              init_boot_pages(map_s, map_e);
> 
> Re-using the outer scope variable is a little risky, don't you agree?
> It just so happens that below here there's no further use requiring
> the earlier value (PAGE_SIZE - 1). This isn't to say I'm opposed, but
> it certainly needs evaluating with this in mind (mentioning of which
> in the description would have demonstrated that you did consider this
> aspect).
> 
> Jan

I guess I should have mentioned it

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

On 07.08.2023 15:18, Nicola Vetrini wrote:
> On 07/08/2023 15:05, Jan Beulich wrote:
>> On 04.08.2023 10:03, Nicola Vetrini wrote:
>>> The parameters renamed in the function declaration caused shadowing
>>> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
>>> them also addresses Rule 8.3:
>>> "All declarations of an object or function shall use the same names
>>> and type qualifiers".
>>
>> Can you explain to me how shadowing can happen in a declaration? I
>> would focus on 8.3 here, and only mention the other name collision.
> 
> There's "static struct file __initdata kernel;" in 
> xen/common/efi/boot.c, which
> is visible when the function is declared. Since renaming these parameter 
> names would
> have been addressed by Federico for R8.3 anyway, my intention was to 
> address them both.

I understand what you say, but your reply doesn't answer my question.
Just to emphasize the important aspect: I could see the shadowing
aspect if the _definition_ of construct_dom0() used "kernel". But I'm
asking about declarations (the one here as well as in general): I
can't see how any shadowing can occur without there being any code in
the position of using any such variable / parameter. IOW if Eclair
spits out 5.3 violations on declarations, I'm inclined to think it's 
wrong. (Because of 8.3 a violation there would then need dealing with
anyway, but _only_ because of 8.3, if the definition is already okay.)

Jan

On 07/08/2023 15:42, Jan Beulich wrote:
> On 07.08.2023 15:18, Nicola Vetrini wrote:
>> On 07/08/2023 15:05, Jan Beulich wrote:
>>> On 04.08.2023 10:03, Nicola Vetrini wrote:
>>>> The parameters renamed in the function declaration caused shadowing
>>>> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
>>>> them also addresses Rule 8.3:
>>>> "All declarations of an object or function shall use the same names
>>>> and type qualifiers".
>>> 
>>> Can you explain to me how shadowing can happen in a declaration? I
>>> would focus on 8.3 here, and only mention the other name collision.
>> 
>> There's "static struct file __initdata kernel;" in
>> xen/common/efi/boot.c, which
>> is visible when the function is declared. Since renaming these 
>> parameter
>> names would
>> have been addressed by Federico for R8.3 anyway, my intention was to
>> address them both.
> 
> I understand what you say, but your reply doesn't answer my question.
> Just to emphasize the important aspect: I could see the shadowing
> aspect if the _definition_ of construct_dom0() used "kernel". But I'm
> asking about declarations (the one here as well as in general): I
> can't see how any shadowing can occur without there being any code in
> the position of using any such variable / parameter. IOW if Eclair
> spits out 5.3 violations on declarations, I'm inclined to think it's
> wrong. (Because of 8.3 a violation there would then need dealing with
> anyway, but _only_ because of 8.3, if the definition is already okay.)
> 
> Jan

The declaration itself is a scope and shadowing can happen, as in:

int x;
void f(int x, int arr[x]);

Now, the example is a bit contrived, but the fact that the rule does not 
list any
exception motivates this behaviour. In any case, I'll try to rephrase 
the commit message
to be less ambiguous.

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

On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> The parameters renamed in the function declaration caused shadowing
> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
> them also addresses Rule 8.3:
> "All declarations of an object or function shall use the same names
> and type qualifiers".
> 
> The local variable 'mask' is removed because it shadows the homonymous
> variable defined in an outer scope, with no change to the semantics.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Same here