Skip to contentSkip to conversation

selftests/vm: silence uninitialized variable warning

PATCH Applied Dan Carpenter 3 participants 5 messages Jul 19, 2022 – Jul 22, 2022

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 selftests/vm: silence uninitialized variable warning

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")
---
 tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/hugetlb-madvise.c +3 -2
index 6c6af40f5747..3c9943131881 100644
@@ -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)
Signature · 1 line
2.35.1
Acked-by on v1 1/1
#1 lore raw
Quoted 2 lines · Dan Carpenter

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.

Quoted 6 lines

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")
Quoted 26 lines
---
 tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/hugetlb-madvise.c +3 -3
index 6c6af40f5747..3c9943131881 100644
@@ -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

#2 lore raw
Quoted 8 lines · Souptick Joarder
Quoted 2 lines · Dan Carpenter

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

Quoted 36 lines
Quoted 7 lines

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")
Quoted 26 lines
---
 tools/testing/selftests/vm/hugetlb-madvise.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
tools/testing/selftests/vm/hugetlb-madvise.c +3 -3
index 6c6af40f5747..3c9943131881 100644
@@ -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

Reviewed-by on v1 1/1
#3 lore raw
In reply to Dan Carpenter
Quoted 10 lines · 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")
---
 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.

Quoted 6 lines
tools/testing/selftests/vm/hugetlb-madvise.c +0 -0
index 6c6af40f5747..3c9943131881 100644

tools/testing/selftests/vm/hugetlb-madvise.c void write_fault_pages(void *addr, unsigned long nr_pages) · line 95

Quoted 9 lines
tools/testing/selftests/vm/hugetlb-madvise.c +2 -2
@@ -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.

Signature · 1 line
Mike Kravetz
Quoted 7 lines
tools/testing/selftests/vm/hugetlb-madvise.c +1 -0
+		dummy += *((unsigned long *)(addr + (i * huge_page_size)));
 }
 
 int main(int argc, char **argv)
Signature · 1 line
2.35.1
#4 lore raw
Quoted 17 lines · Mike Kravetz
Quoted 5 lines
tools/testing/selftests/vm/hugetlb-madvise.c +0 -0
index 6c6af40f5747..3c9943131881 100644

tools/testing/selftests/vm/hugetlb-madvise.c void write_fault_pages(void *addr, unsigned long nr_pages) · line 95

Quoted 9 lines
tools/testing/selftests/vm/hugetlb-madvise.c +2 -2
@@ -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:

Quoted 5 lines
	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