| Message ID | 20200928100004.25674-1-lukas.bulwahn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: drop superfluous initialization | expand |
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> -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#37): https://lists.elisa.tech/g/linux-safety/message/37 Mute This Topic: https://lists.elisa.tech/mt/77171089/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#40): https://lists.elisa.tech/g/linux-safety/message/40 Mute This Topic: https://lists.elisa.tech/mt/77171089/4688437 Group Owner: linux-safety+owner@lists.elisa.tech Unsubscribe: https://lists.elisa.tech/g/linux-safety/unsub [patchwork-linux-safety@patchwork.kernel.org] -=-=-=-=-=-=-=-=-=-=-=-
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.
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(-)