Hi,
This is v2 of a series that fixes up build errors and warnings for at
least the 64-bit builds on x86 with clang.
There are lots of changes since v1 [1], thanks to reviews from Peter Xu, David
Hildenbrand, and Muhammad Usama Anjum. These include:
* Using "make headers", and documenting that prerequisite as well.
* Better ways to avoid clang's Wformat-security warnings
* Added Cc's, ack-by's, reviewed-by's.
* Updated commit log messages.
The series also includes an optional "improvement" of moving some uffd
code into uffd-common.[ch], which is proving to be somewhat
controversial, and so if that doesn't get resolved, then patches 9 and
10 may just get dropped. They are not required in order to get a clean
build, now that "make headers" is happening.
[1]: https://lore.kernel.org/all/[email protected]/
thanks,
John Hubbard
NVIDIA
John Hubbard (11):
selftests/mm: fix uffd-stress unused function warning
selftests/mm: fix unused variable warnings in hugetlb-madvise.c,
migration.c
selftests/mm: fix "warning: expression which evaluates to zero..." in
mlock2-tests.c
selftests/mm: fix invocation of tests that are run via shell scripts
selftests/mm: .gitignore: add mkdirty, va_high_addr_switch
selftests/mm: fix two -Wformat-security warnings in uffd builds
selftests/mm: fix a "possibly uninitialized" warning in pkey-x86.h
selftests/mm: fix uffd-unit-tests.c build failure due to missing
MADV_COLLAPSE
selftests/mm: move psize(), pshift() into vm_utils.c
selftests/mm: move uffd* routines from vm_util.c to uffd-common.c
Documentation: kselftest: "make headers" is a prerequisite
Documentation/dev-tools/kselftest.rst | 1 +
tools/testing/selftests/mm/.gitignore | 2 +
tools/testing/selftests/mm/Makefile | 7 +-
tools/testing/selftests/mm/cow.c | 7 --
tools/testing/selftests/mm/hugepage-mremap.c | 2 +-
tools/testing/selftests/mm/hugetlb-madvise.c | 8 +-
tools/testing/selftests/mm/khugepaged.c | 10 --
.../selftests/mm/ksm_functional_tests.c | 2 +-
tools/testing/selftests/mm/migration.c | 5 +-
tools/testing/selftests/mm/mlock2-tests.c | 1 -
tools/testing/selftests/mm/pkey-x86.h | 2 +-
tools/testing/selftests/mm/run_vmtests.sh | 6 +-
tools/testing/selftests/mm/uffd-common.c | 105 +++++++++++++++++
tools/testing/selftests/mm/uffd-common.h | 12 +-
tools/testing/selftests/mm/uffd-stress.c | 10 --
tools/testing/selftests/mm/uffd-unit-tests.c | 16 +--
tools/testing/selftests/mm/vm_util.c | 106 ++----------------
tools/testing/selftests/mm/vm_util.h | 36 ++----
18 files changed, 165 insertions(+), 173 deletions(-)
base-commit: 929ed21dfdb6ee94391db51c9eedb63314ef6847
--
2.40.1
The stop variable is a char*, and the code was assigning a char value to
it. This was generating a warning when compiling with clang.
However, as both David and Peter pointed out, stop is not even used
after the problematic assignment to a char type. So just delete that
line entirely.
Cc: David Hildenbrand <[email protected]>
Cc: Peter Xu <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
tools/testing/selftests/mm/mlock2-tests.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
index 11b2301f3aa3..80cddc0de206 100644
--- a/tools/testing/selftests/mm/mlock2-tests.c
+++ b/tools/testing/selftests/mm/mlock2-tests.c
@@ -50,7 +50,6 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
printf("cannot parse /proc/self/maps\n");
goto out;
}
- stop = '\0';
sscanf(line, "%lx", &start);
sscanf(end_addr, "%lx", &end);
--
2.40.1
The uffd tests generate two compile time warnings from clang's
-Wformat-security setting. These trigger at the call sites for
uffd_test_start() and uffd_test_skip().
1) Fix the uffd_test_start() issue by removing the intermediate
test_name variable (thanks to David Hildenbrand for showing how to do
this).
2) Fix the uffd_test_skip() issue by observing that there is no need for
a macro and a variable args approach, because all callers of
uffd_test_skip() pass in a simple char* string, without any format
specifiers. So just change uffd_test_skip() into a regular C function.
Cc: David Hildenbrand <[email protected]>
Cc: Peter Xu <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
tools/testing/selftests/mm/uffd-unit-tests.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
index 269c86768a02..04d91f144d1c 100644
--- a/tools/testing/selftests/mm/uffd-unit-tests.c
+++ b/tools/testing/selftests/mm/uffd-unit-tests.c
@@ -109,12 +109,11 @@ static void uffd_test_pass(void)
ksft_inc_fail_cnt(); \
} while (0)
-#define uffd_test_skip(...) do { \
- printf("skipped [reason: "); \
- printf(__VA_ARGS__); \
- printf("]\n"); \
- ksft_inc_xskip_cnt(); \
- } while (0)
+static void uffd_test_skip(const char *message)
+{
+ printf("skipped [reason: %s]\n", message);
+ ksft_inc_xskip_cnt();
+}
/*
* Returns 1 if specific userfaultfd supported, 0 otherwise. Note, we'll
@@ -1149,7 +1148,6 @@ int main(int argc, char *argv[])
uffd_test_case_t *test;
mem_type_t *mem_type;
uffd_test_args_t args;
- char test_name[128];
const char *errmsg;
int has_uffd, opt;
int i, j;
@@ -1192,10 +1190,8 @@ int main(int argc, char *argv[])
mem_type = &mem_types[j];
if (!(test->mem_targets & mem_type->mem_flag))
continue;
- snprintf(test_name, sizeof(test_name),
- "%s on %s", test->name, mem_type->name);
- uffd_test_start(test_name);
+ uffd_test_start("%s on %s", test->name, mem_type->name);
if (!uffd_feature_supported(test)) {
uffd_test_skip("feature missing");
continue;
--
2.40.1
On 03.06.23 04:15, John Hubbard wrote:
> The uffd tests generate two compile time warnings from clang's
> -Wformat-security setting. These trigger at the call sites for
> uffd_test_start() and uffd_test_skip().
>
> 1) Fix the uffd_test_start() issue by removing the intermediate
> test_name variable (thanks to David Hildenbrand for showing how to do
> this).
>
> 2) Fix the uffd_test_skip() issue by observing that there is no need for
> a macro and a variable args approach, because all callers of
> uffd_test_skip() pass in a simple char* string, without any format
> specifiers. So just change uffd_test_skip() into a regular C function.
>
> Cc: David Hildenbrand <[email protected]>
> Cc: Peter Xu <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> tools/testing/selftests/mm/uffd-unit-tests.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> index 269c86768a02..04d91f144d1c 100644
> --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -109,12 +109,11 @@ static void uffd_test_pass(void)
> ksft_inc_fail_cnt(); \
> } while (0)
>
> -#define uffd_test_skip(...) do { \
> - printf("skipped [reason: "); \
> - printf(__VA_ARGS__); \
> - printf("]\n"); \
> - ksft_inc_xskip_cnt(); \
> - } while (0)
> +static void uffd_test_skip(const char *message)
> +{
> + printf("skipped [reason: %s]\n", message);
> + ksft_inc_xskip_cnt();
> +}
>
> /*
> * Returns 1 if specific userfaultfd supported, 0 otherwise. Note, we'll
> @@ -1149,7 +1148,6 @@ int main(int argc, char *argv[])
> uffd_test_case_t *test;
> mem_type_t *mem_type;
> uffd_test_args_t args;
> - char test_name[128];
> const char *errmsg;
> int has_uffd, opt;
> int i, j;
> @@ -1192,10 +1190,8 @@ int main(int argc, char *argv[])
> mem_type = &mem_types[j];
> if (!(test->mem_targets & mem_type->mem_flag))
> continue;
> - snprintf(test_name, sizeof(test_name),
> - "%s on %s", test->name, mem_type->name);
>
> - uffd_test_start(test_name);
> + uffd_test_start("%s on %s", test->name, mem_type->name);
> if (!uffd_feature_supported(test)) {
> uffd_test_skip("feature missing");
> continue;
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On 03.06.23 04:15, John Hubbard wrote:
> The stop variable is a char*, and the code was assigning a char value to
> it. This was generating a warning when compiling with clang.
>
> However, as both David and Peter pointed out, stop is not even used
> after the problematic assignment to a char type. So just delete that
> line entirely.
>
> Cc: David Hildenbrand <[email protected]>
> Cc: Peter Xu <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On Fri, Jun 02, 2023 at 07:15:50PM -0700, John Hubbard wrote:
> The stop variable is a char*, and the code was assigning a char value to
> it. This was generating a warning when compiling with clang.
>
> However, as both David and Peter pointed out, stop is not even used
> after the problematic assignment to a char type. So just delete that
> line entirely.
>
> Cc: David Hildenbrand <[email protected]>
> Cc: Peter Xu <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> tools/testing/selftests/mm/mlock2-tests.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
> index 11b2301f3aa3..80cddc0de206 100644
> --- a/tools/testing/selftests/mm/mlock2-tests.c
> +++ b/tools/testing/selftests/mm/mlock2-tests.c
> @@ -50,7 +50,6 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
> printf("cannot parse /proc/self/maps\n");
> goto out;
> }
> - stop = '\0';
>
> sscanf(line, "%lx", &start);
> sscanf(end_addr, "%lx", &end);
I'd rather simply make it "*stop = '\0'", or as David suggested dropping
stop completely when we're it (assumes that scanf() will always work with
number ending with space ' ').
No strong opinion here, though.
--
Peter Xu
On Fri, Jun 02, 2023 at 07:15:53PM -0700, John Hubbard wrote:
> The uffd tests generate two compile time warnings from clang's
> -Wformat-security setting. These trigger at the call sites for
> uffd_test_start() and uffd_test_skip().
>
> 1) Fix the uffd_test_start() issue by removing the intermediate
> test_name variable (thanks to David Hildenbrand for showing how to do
> this).
>
> 2) Fix the uffd_test_skip() issue by observing that there is no need for
> a macro and a variable args approach, because all callers of
> uffd_test_skip() pass in a simple char* string, without any format
> specifiers. So just change uffd_test_skip() into a regular C function.
>
> Cc: David Hildenbrand <[email protected]>
> Cc: Peter Xu <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
Reviewed-by: Peter Xu <[email protected]>
Thanks,
--
Peter Xu
On 6/5/23 08:43, Peter Xu wrote:
> On Fri, Jun 02, 2023 at 07:15:50PM -0700, John Hubbard wrote:
>> The stop variable is a char*, and the code was assigning a char value to
>> it. This was generating a warning when compiling with clang.
>>
>> However, as both David and Peter pointed out, stop is not even used
>> after the problematic assignment to a char type. So just delete that
>> line entirely.
>>
>> Cc: David Hildenbrand <[email protected]>
>> Cc: Peter Xu <[email protected]>
>> Signed-off-by: John Hubbard <[email protected]>
>> ---
>> tools/testing/selftests/mm/mlock2-tests.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
>> index 11b2301f3aa3..80cddc0de206 100644
>> --- a/tools/testing/selftests/mm/mlock2-tests.c
>> +++ b/tools/testing/selftests/mm/mlock2-tests.c
>> @@ -50,7 +50,6 @@ static int get_vm_area(unsigned long addr, struct vm_boundaries *area)
>> printf("cannot parse /proc/self/maps\n");
>> goto out;
>> }
>> - stop = '\0';
>>
>> sscanf(line, "%lx", &start);
>> sscanf(end_addr, "%lx", &end);
>
> I'd rather simply make it "*stop = '\0'", or as David suggested dropping
> stop completely when we're it (assumes that scanf() will always work with
> number ending with space ' ').
Actually it does not assume that. Rather, it follows the documented behavior
of strchr(3), which is:
The strchr() and strrchr() functions return a pointer to the matched
character or NULL if the character is not found. The terminating
null byte is considered part of the string, so that if c is
specified as '\0', these functions return a pointer to the
terminator.
And we have this code now:
stop = strchr(end_addr, ' ');
if (!stop) {
printf("cannot parse /proc/self/maps\n");
goto out;
}
So, either stop has a valid char* in it, or we goto out. There are no
fragile assumptions in there, as far as I can see anyway.
>
> No strong opinion here, though.
>
OK, I think it's kind of a flip of the coin whether to write this:
stop = strchr(end_addr, ' ');
if (!stop) {
or this:
if (!strchr(end_addr, ' ')) {
So I'll just leave it as the first one, which (depending on the
day of the week) might read slightly clearer. :)
thanks,
--
John Hubbard
NVIDIA