[PATCH] x86/mm: drop superfluous initialization

[PATCH] x86/mm: drop superfluous initialization

[Lukas Bulwahn]

It is not required to initialize the local variable start in
memory_map_top_down(), as the variable will be initialized in any path
before it is used.

make clang-analyzer on x86_64 tinyconfig reports:

  arch/x86/mm/init.c:612:15: warning: Although the value stored to 'start' \
  is used in the enclosing expression, the value is never actually read \
  from 'start' [clang-analyzer-deadcode.DeadStores]

Compilers will detect this superfluous assignment and optimize that
expression anyway. So, the resulting binary is identical before and after
the change.

Drop this superfluous assignment to make clang-analyzer happy.

No functional change.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on v5.9-rc7 and next-20200925

Dave, Andy, Peter, please pick this minor non-urgent clean-up patch.

I quickly confirmed that the binary did not change with this change to the
source code; the hash of init.o remained the same before and after the change.

So, in my setup:
  md5sum arch/x86/mm/init.o
  b26f6380760f32d2ef2c7525301eebd3  init.o

linux-safety, please verify and validate this change.

 arch/x86/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index c7a47603537f..5632f02146ca 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
 	step_size = PMD_SIZE;
 	max_pfn_mapped = 0; /* will get exact value next */
 	min_pfn_mapped = real_end >> PAGE_SHIFT;
-	last_start = start = real_end;
+	last_start = real_end;
 
 	/*
 	 * We start from the top (end of memory) and go to the bottom.
-- 
2.17.1

[PATCH] x86/mm: drop superfluous initialization

[Lukas Bulwahn]

It is not required to initialize the local variable start in
memory_map_top_down(), as the variable will be initialized in any path
before it is used.

make clang-analyzer on x86_64 tinyconfig reports:

  arch/x86/mm/init.c:612:15: warning: Although the value stored to 'start' \
  is used in the enclosing expression, the value is never actually read \
  from 'start' [clang-analyzer-deadcode.DeadStores]

Compilers will detect this superfluous assignment and optimize that
expression anyway. So, the resulting binary is identical before and after
the change.

Drop this superfluous assignment to make clang-analyzer happy.

No functional change.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on v5.9-rc7 and next-20200925

Dave, Andy, Peter, please pick this minor non-urgent clean-up patch.

I quickly confirmed that the binary did not change with this change to the
source code; the hash of init.o remained the same before and after the change.

So, in my setup:
  md5sum arch/x86/mm/init.o
  b26f6380760f32d2ef2c7525301eebd3  init.o

linux-safety, please verify and validate this change.

 arch/x86/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index c7a47603537f..5632f02146ca 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
 	step_size = PMD_SIZE;
 	max_pfn_mapped = 0; /* will get exact value next */
 	min_pfn_mapped = real_end >> PAGE_SHIFT;
-	last_start = start = real_end;
+	last_start = real_end;
 
 	/*
 	 * We start from the top (end of memory) and go to the bottom.
-- 
2.17.1

Re: [PATCH] x86/mm: drop superfluous initialization

[Dave Hansen]

On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index c7a47603537f..5632f02146ca 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
>  	step_size = PMD_SIZE;
>  	max_pfn_mapped = 0; /* will get exact value next */
>  	min_pfn_mapped = real_end >> PAGE_SHIFT;
> -	last_start = start = real_end;
> +	last_start = real_end;

Thanks for finding this.

This becomes even more obviously correct if we just move the 'start'
declaration into the while() loop.  If we do that, it puts the three
assignment locations right next to the definition, and its trivial to
spot that the initialization was not missed:

        while (last_start > map_start) {
		unsigned long start;

                if (last_start > step_size) {
                        start = round_down(last_start - 1, step_size);
                        if (start < map_start)
                                start = map_start;
                } else
                        start = map_start;
		...

Either way, your patch looks correct to me:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH] x86/mm: drop superfluous initialization

[Dave Hansen]

On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index c7a47603537f..5632f02146ca 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
>  	step_size = PMD_SIZE;
>  	max_pfn_mapped = 0; /* will get exact value next */
>  	min_pfn_mapped = real_end >> PAGE_SHIFT;
> -	last_start = start = real_end;
> +	last_start = real_end;

Thanks for finding this.

This becomes even more obviously correct if we just move the 'start'
declaration into the while() loop.  If we do that, it puts the three
assignment locations right next to the definition, and its trivial to
spot that the initialization was not missed:

        while (last_start > map_start) {
		unsigned long start;

                if (last_start > step_size) {
                        start = round_down(last_start - 1, step_size);
                        if (start < map_start)
                                start = map_start;
                } else
                        start = map_start;
		...

Either way, your patch looks correct to me:

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH] x86/mm: drop superfluous initialization

[Lukas Bulwahn]

On Mon, 28 Sep 2020, Dave Hansen wrote:

> On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index c7a47603537f..5632f02146ca 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
> >  	step_size = PMD_SIZE;
> >  	max_pfn_mapped = 0; /* will get exact value next */
> >  	min_pfn_mapped = real_end >> PAGE_SHIFT;
> > -	last_start = start = real_end;
> > +	last_start = real_end;
> 
> Thanks for finding this.
> 
> This becomes even more obviously correct if we just move the 'start'
> declaration into the while() loop.  If we do that, it puts the three
> assignment locations right next to the definition, and its trivial to
> spot that the initialization was not missed:
> 
>         while (last_start > map_start) {
> 		unsigned long start;
> 
>                 if (last_start > step_size) {
>                         start = round_down(last_start - 1, step_size);
>                         if (start < map_start)
>                                 start = map_start;
>                 } else
>                         start = map_start;
> 		...
>

Agree, this point is simply a question of style:

Shall local variables be defined as "local" as possible or simply 
consistently at the beginning of each function?

If there are no strong opinions of style, I would just keep this patch 
as-is.

> Either way, your patch looks correct to me:
> 
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> 

Thanks for the Ack.

Lukas

Re: [PATCH] x86/mm: drop superfluous initialization

[Lukas Bulwahn]

On Mon, 28 Sep 2020, Dave Hansen wrote:

> On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index c7a47603537f..5632f02146ca 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
> >  	step_size = PMD_SIZE;
> >  	max_pfn_mapped = 0; /* will get exact value next */
> >  	min_pfn_mapped = real_end >> PAGE_SHIFT;
> > -	last_start = start = real_end;
> > +	last_start = real_end;
> 
> Thanks for finding this.
> 
> This becomes even more obviously correct if we just move the 'start'
> declaration into the while() loop.  If we do that, it puts the three
> assignment locations right next to the definition, and its trivial to
> spot that the initialization was not missed:
> 
>         while (last_start > map_start) {
> 		unsigned long start;
> 
>                 if (last_start > step_size) {
>                         start = round_down(last_start - 1, step_size);
>                         if (start < map_start)
>                                 start = map_start;
>                 } else
>                         start = map_start;
> 		...
>

Agree, this point is simply a question of style:

Shall local variables be defined as "local" as possible or simply 
consistently at the beginning of each function?

If there are no strong opinions of style, I would just keep this patch 
as-is.

> Either way, your patch looks correct to me:
> 
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> 

Thanks for the Ack.

Lukas

Re: [PATCH] x86/mm: drop superfluous initialization

[Lukas Bulwahn]

On Mon, 28 Sep 2020, Dave Hansen wrote:

> On 9/28/20 3:00 AM, Lukas Bulwahn wrote:
> > diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> > index c7a47603537f..5632f02146ca 100644
> > --- a/arch/x86/mm/init.c
> > +++ b/arch/x86/mm/init.c
> > @@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
> >  	step_size = PMD_SIZE;
> >  	max_pfn_mapped = 0; /* will get exact value next */
> >  	min_pfn_mapped = real_end >> PAGE_SHIFT;
> > -	last_start = start = real_end;
> > +	last_start = real_end;
> 
> Thanks for finding this.
> 
> This becomes even more obviously correct if we just move the 'start'
> declaration into the while() loop.  If we do that, it puts the three
> assignment locations right next to the definition, and its trivial to
> spot that the initialization was not missed:
> 
>         while (last_start > map_start) {
> 		unsigned long start;
> 
>                 if (last_start > step_size) {
>                         start = round_down(last_start - 1, step_size);
>                         if (start < map_start)
>                                 start = map_start;
>                 } else
>                         start = map_start;
> 		...
>

Agree, this point is simply a question of style:

Shall local variables be defined as "local" as possible or simply 
consistently at the beginning of each function?

If there are no strong opinions of style, I would just keep this patch 
as-is.

> Either way, your patch looks correct to me:
> 
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> 

Thanks for the Ack.

Lukas

[tip: x86/cleanups] x86/mm: Declare 'start' variable where it is used

[tip-bot2 for Lukas Bulwahn]

The following commit has been merged into the x86/cleanups branch of tip:

Commit-ID:     bab202ab87ba4da48018daf0f6810b22705a570d
Gitweb:        https://git.kernel.org/tip/bab202ab87ba4da48018daf0f6810b22705a570d
Author:        Lukas Bulwahn <lukas.bulwahn@gmail.com>
AuthorDate:    Mon, 28 Sep 2020 12:00:04 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 20 Nov 2020 12:49:00 +01:00

x86/mm: Declare 'start' variable where it is used

It is not required to initialize the local variable start in
memory_map_top_down(), as the variable will be initialized in any path
before it is used.

make clang-analyzer on x86_64 tinyconfig reports:

  arch/x86/mm/init.c:612:15: warning: Although the value stored to 'start' \
  is used in the enclosing expression, the value is never actually read \
  from 'start' [clang-analyzer-deadcode.DeadStores]

Move the variable declaration into the loop, where it is used.

No code changed:

  # arch/x86/mm/init.o:

   text    data     bss     dec     hex filename
   7105    1424   26768   35297    89e1 init.o.before
   7105    1424   26768   35297    89e1 init.o.after

md5:
   a8d76c1bb5fce9cae251780a7ee7730f  init.o.before.asm
   a8d76c1bb5fce9cae251780a7ee7730f  init.o.after.asm

 [ bp: Massage. ]

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Link: https://lkml.kernel.org/r/20200928100004.25674-1-lukas.bulwahn@gmail.com
---
 arch/x86/mm/init.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index c7a4760..e26f5c5 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -596,7 +596,7 @@ static unsigned long __init get_new_step_size(unsigned long step_size)
 static void __init memory_map_top_down(unsigned long map_start,
 				       unsigned long map_end)
 {
-	unsigned long real_end, start, last_start;
+	unsigned long real_end, last_start;
 	unsigned long step_size;
 	unsigned long addr;
 	unsigned long mapped_ram_size = 0;
@@ -609,7 +609,7 @@ static void __init memory_map_top_down(unsigned long map_start,
 	step_size = PMD_SIZE;
 	max_pfn_mapped = 0; /* will get exact value next */
 	min_pfn_mapped = real_end >> PAGE_SHIFT;
-	last_start = start = real_end;
+	last_start = real_end;
 
 	/*
 	 * We start from the top (end of memory) and go to the bottom.
@@ -618,6 +618,8 @@ static void __init memory_map_top_down(unsigned long map_start,
 	 * for page table.
 	 */
 	while (last_start > map_start) {
+		unsigned long start;
+
 		if (last_start > step_size) {
 			start = round_down(last_start - 1, step_size);
 			if (start < map_start)