[PATCH] selftests/harness: fix many "format string is empty" warnings

[PATCH] selftests/harness: fix many "format string is empty" warnings

[John Hubbard]

In order to build with clang at all, in order to see these symptoms, one
must first apply Valentin Obst's build fix for LLVM [1]. Once that is
done, then when building with clang, via:

    make LLVM=1 -C tools/testing/selftests

...clang emits a "format string is empty" warning. (gcc also emits a
similar warning.)

Fix by passing NULL, instead of "", for the msg argument to
ksft_test_result_code(). This removes dozens of warnings and a few
errors (some tests have -Werror set).

[1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/

Cc: Valentin Obst <kernel@valentinobst.de>
Cc: Kees Cook <keescook@chromium.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Bill Wendling <morbo@google.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/kselftest_harness.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index d98702b6955d..456b8694e678 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -1207,8 +1207,10 @@ void __run_test(struct __fixture_metadata *f,
 	else
 		diagnostic = "unknown";
 
-	ksft_test_result_code(t->exit_code, test_name,
-			      diagnostic ? "%s" : NULL, diagnostic);
+	if (diagnostic)
+		ksft_test_result_code(t->exit_code, test_name, "%s", diagnostic);
+	else
+		ksft_test_result_code(t->exit_code, test_name, NULL);
 	free(test_name);
 }
 

base-commit: 18daea77cca626f590fb140fc11e3a43c5d41354
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
-- 
2.45.0

Re: [PATCH] selftests/harness: fix many "format string is empty" warnings

[Nathan Chancellor]

Hi John,

On Tue, Apr 30, 2024 at 07:08:13PM -0700, John Hubbard wrote:
> In order to build with clang at all, in order to see these symptoms, one
> must first apply Valentin Obst's build fix for LLVM [1]. Once that is
> done, then when building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftests
> 
> ...clang emits a "format string is empty" warning. (gcc also emits a
> similar warning.)

The warning you are describing here sounds like the same exact one that
commit caed8eba2215 ("selftests: kselftest_harness: fix Clang warning
about zero-length format") should have addressed in 6.9-rc5. Is this
patch actually necessary?

> Fix by passing NULL, instead of "", for the msg argument to

Because this text is describing what was done in caed8eba2215...

> ksft_test_result_code(). This removes dozens of warnings and a few
> errors (some tests have -Werror set).
>
> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/
> 
> Cc: Valentin Obst <kernel@valentinobst.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Justin Stitt <justinstitt@google.com>
> Cc: Bill Wendling <morbo@google.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/kselftest_harness.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index d98702b6955d..456b8694e678 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -1207,8 +1207,10 @@ void __run_test(struct __fixture_metadata *f,
>  	else
>  		diagnostic = "unknown";
>  
> -	ksft_test_result_code(t->exit_code, test_name,
> -			      diagnostic ? "%s" : NULL, diagnostic);
> +	if (diagnostic)
> +		ksft_test_result_code(t->exit_code, test_name, "%s", diagnostic);
> +	else
> +		ksft_test_result_code(t->exit_code, test_name, NULL);

but this diff is not doing that because it is based on a tree that has
caed8eba2215; instead, it appears to be a completely identical
transformation?

Cheers,
Nathan

>  	free(test_name);
>  }
>  
> 
> base-commit: 18daea77cca626f590fb140fc11e3a43c5d41354
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
> -- 
> 2.45.0
> 

Re: [PATCH] selftests/harness: fix many "format string is empty" warnings

[John Hubbard]

On 5/1/24 8:13 AM, Nathan Chancellor wrote:
> Hi John,
> 
> On Tue, Apr 30, 2024 at 07:08:13PM -0700, John Hubbard wrote:
>> In order to build with clang at all, in order to see these symptoms, one
>> must first apply Valentin Obst's build fix for LLVM [1]. Once that is
>> done, then when building with clang, via:
>>
>>      make LLVM=1 -C tools/testing/selftests
>>
>> ...clang emits a "format string is empty" warning. (gcc also emits a
>> similar warning.)
> 
> The warning you are describing here sounds like the same exact one that
> commit caed8eba2215 ("selftests: kselftest_harness: fix Clang warning
> about zero-length format") should have addressed in 6.9-rc5. Is this
> patch actually necessary?

Apparently not!

> 
>> Fix by passing NULL, instead of "", for the msg argument to
> 
> Because this text is describing what was done in caed8eba2215...

Yes. I took a while between discovering the issue, and posting a fix, I
think that's why.

> 
>> ksft_test_result_code(). This removes dozens of warnings and a few
>> errors (some tests have -Werror set).
>>
>> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/
>>
>> Cc: Valentin Obst <kernel@valentinobst.de>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Nick Desaulniers <ndesaulniers@google.com>
>> Cc: Nathan Chancellor <nathan@kernel.org>
>> Cc: Shuah Khan <shuah@kernel.org>
>> Cc: Justin Stitt <justinstitt@google.com>
>> Cc: Bill Wendling <morbo@google.com>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   tools/testing/selftests/kselftest_harness.h | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
>> index d98702b6955d..456b8694e678 100644
>> --- a/tools/testing/selftests/kselftest_harness.h
>> +++ b/tools/testing/selftests/kselftest_harness.h
>> @@ -1207,8 +1207,10 @@ void __run_test(struct __fixture_metadata *f,
>>   	else
>>   		diagnostic = "unknown";
>>   
>> -	ksft_test_result_code(t->exit_code, test_name,
>> -			      diagnostic ? "%s" : NULL, diagnostic);
>> +	if (diagnostic)
>> +		ksft_test_result_code(t->exit_code, test_name, "%s", diagnostic);
>> +	else
>> +		ksft_test_result_code(t->exit_code, test_name, NULL);
> 
> but this diff is not doing that because it is based on a tree that has
> caed8eba2215; instead, it appears to be a completely identical
> transformation?
> 

It's nice to see that someone else had the same fix, anyway. Sorry
about the noise, and yes let's please just drop this one.

thanks,
-- 
John Hubbard
NVIDIA