[PATCH] selftests/vm: silence uninitialized variable warning

[PATCH] selftests/vm: silence uninitialized variable warning

[Dan Carpenter]

This code just reads from memory without caring about the data itself.
However static checkers complain that "tmp" is never properly
initialized.  Initialize it to zero and change the name to "dummy" to
show that we don't care about the value stored in it.

Fixes: c4b6cb884011 ("selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
index 6c6af40f5747..3c9943131881 100644
--- a/tools/testing/selftests/vm/hugetlb-madvise.c
+++ b/tools/testing/selftests/vm/hugetlb-madvise.c
@@ -89,10 +89,11 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
 
 void read_fault_pages(void *addr, unsigned long nr_pages)
 {
-	unsigned long i, tmp;
+	unsigned long dummy = 0;
+	unsigned long i;
 
 	for (i = 0; i < nr_pages; i++)
-		tmp += *((unsigned long *)(addr + (i * huge_page_size)));
+		dummy += *((unsigned long *)(addr + (i * huge_page_size)));
 }
 
 int main(int argc, char **argv)
-- 
2.35.1

Re: [PATCH] selftests/vm: silence uninitialized variable warning

[Souptick Joarder]

On Tue, Jul 19, 2022 at 3:13 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> This code just reads from memory without caring about the data itself.

The caller has put an attempt to at least validate the address read
from mmap() before
passing it to read_fault_pages() which looks correct. I think this
line is not needed.

> However static checkers complain that "tmp" is never properly
> initialized.  Initialize it to zero and change the name to "dummy" to
> show that we don't care about the value stored in it.
>
> Fixes: c4b6cb884011 ("selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Souptick Joarder (HPE) <jrdr.linux@gmail.com>

> ---
>  tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
> index 6c6af40f5747..3c9943131881 100644
> --- a/tools/testing/selftests/vm/hugetlb-madvise.c
> +++ b/tools/testing/selftests/vm/hugetlb-madvise.c
> @@ -89,10 +89,11 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
>
>  void read_fault_pages(void *addr, unsigned long nr_pages)
>  {
> -       unsigned long i, tmp;
> +       unsigned long dummy = 0;
> +       unsigned long i;
>
>         for (i = 0; i < nr_pages; i++)
> -               tmp += *((unsigned long *)(addr + (i * huge_page_size)));
> +               dummy += *((unsigned long *)(addr + (i * huge_page_size)));
>  }
>
>  int main(int argc, char **argv)
> --
> 2.35.1
>
>

Re: [PATCH] selftests/vm: silence uninitialized variable warning

[Dan Carpenter]

On Thu, Jul 21, 2022 at 06:38:59AM +0530, Souptick Joarder wrote:
> On Tue, Jul 19, 2022 at 3:13 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > This code just reads from memory without caring about the data itself.
> 
> The caller has put an attempt to at least validate the address read
> from mmap() before
> passing it to read_fault_pages() which looks correct. I think this
> line is not needed.

What I mean is, the value of "tmp" was always nonsense but it doesn't
matter because we don't care.  In other words, we're just reading but we
don't care about the actual data.

regards,
dan carpenter

> 
> > However static checkers complain that "tmp" is never properly
> > initialized.  Initialize it to zero and change the name to "dummy" to
> > show that we don't care about the value stored in it.
> >
> > Fixes: c4b6cb884011 ("selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Acked-by: Souptick Joarder (HPE) <jrdr.linux@gmail.com>
> 
> > ---
> >  tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
> > index 6c6af40f5747..3c9943131881 100644
> > --- a/tools/testing/selftests/vm/hugetlb-madvise.c
> > +++ b/tools/testing/selftests/vm/hugetlb-madvise.c
> > @@ -89,10 +89,11 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
> >
> >  void read_fault_pages(void *addr, unsigned long nr_pages)
> >  {
> > -       unsigned long i, tmp;
> > +       unsigned long dummy = 0;
> > +       unsigned long i;
> >
> >         for (i = 0; i < nr_pages; i++)
> > -               tmp += *((unsigned long *)(addr + (i * huge_page_size)));
> > +               dummy += *((unsigned long *)(addr + (i * huge_page_size)));
> >  }
> >
> >  int main(int argc, char **argv)
> > --
> > 2.35.1
> >
> >

Re: [PATCH] selftests/vm: silence uninitialized variable warning

[Mike Kravetz]

On 07/19/22 12:42, Dan Carpenter wrote:
> This code just reads from memory without caring about the data itself.
> However static checkers complain that "tmp" is never properly
> initialized.  Initialize it to zero and change the name to "dummy" to
> show that we don't care about the value stored in it.
> 
> Fixes: c4b6cb884011 ("selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Thanks Dan!

Your analysis is correct.  We do not care about the value returned, and
just want to trigger a read fault.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

> 
> diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
> index 6c6af40f5747..3c9943131881 100644
> --- a/tools/testing/selftests/vm/hugetlb-madvise.c
> +++ b/tools/testing/selftests/vm/hugetlb-madvise.c
> @@ -89,10 +89,11 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
>  
>  void read_fault_pages(void *addr, unsigned long nr_pages)
>  {
> -	unsigned long i, tmp;
> +	unsigned long dummy = 0;
> +	unsigned long i;
>  
>  	for (i = 0; i < nr_pages; i++)
> -		tmp += *((unsigned long *)(addr + (i * huge_page_size)));

When I originally wrote this, something must have complained if written as:

	tmp = *((unsigned long *)(addr + (i * huge_page_size)));

changing to += eliminated that complaint, but caused this one.  Happy
with your changes, but if there is an even better way to write this, I am
happy to change it.

-- 
Mike Kravetz

> +		dummy += *((unsigned long *)(addr + (i * huge_page_size)));
>  }
>  
>  int main(int argc, char **argv)
> -- 
> 2.35.1
> 

Re: [PATCH] selftests/vm: silence uninitialized variable warning

[Dan Carpenter]

On Thu, Jul 21, 2022 at 10:52:54AM -0700, Mike Kravetz wrote:
> > diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
> > index 6c6af40f5747..3c9943131881 100644
> > --- a/tools/testing/selftests/vm/hugetlb-madvise.c
> > +++ b/tools/testing/selftests/vm/hugetlb-madvise.c
> > @@ -89,10 +89,11 @@ void write_fault_pages(void *addr, unsigned long nr_pages)
> >  
> >  void read_fault_pages(void *addr, unsigned long nr_pages)
> >  {
> > -	unsigned long i, tmp;
> > +	unsigned long dummy = 0;
> > +	unsigned long i;
> >  
> >  	for (i = 0; i < nr_pages; i++)
> > -		tmp += *((unsigned long *)(addr + (i * huge_page_size)));
> 
> When I originally wrote this, something must have complained if written as:
> 
> 	tmp = *((unsigned long *)(addr + (i * huge_page_size)));
> 
> changing to += eliminated that complaint, but caused this one.  Happy
> with your changes, but if there is an even better way to write this, I am
> happy to change it.

These days compilers don't like when you read something and then don't
use it.  It's possible they might just start optimizing parts of that
away?  I think we've now tricked the compiler into doing what we want.

regards,
dan carpenter