[v1] selftests/mqueue: fix 5 warnings about signed/unsigned mismatches

[PATCH] selftests/mqueue: fix 5 warnings about signed/unsigned mismatches

Posted by John Hubbard 1 week, 6 days ago

When building with clang, via:

    make LLVM=1 -C tools/testing/selftest

...clang warns about several cases of using a signed integer for the
priority argument to mq_receive(3), which expects an unsigned int.

Fix this by declaring the type as unsigned int in all cases.

Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/mqueue/mq_perf_tests.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c b/tools/testing/selftests/mqueue/mq_perf_tests.c
index 5c16159d0bcd..fb898850867c 100644
--- a/tools/testing/selftests/mqueue/mq_perf_tests.c
+++ b/tools/testing/selftests/mqueue/mq_perf_tests.c
@@ -323,7 +323,8 @@ void *fake_cont_thread(void *arg)
 void *cont_thread(void *arg)
 {
 	char buff[MSG_SIZE];
-	int i, priority;
+	int i;
+	unsigned int priority;
 
 	for (i = 0; i < num_cpus_to_pin; i++)
 		if (cpu_threads[i] == pthread_self())
@@ -425,7 +426,8 @@ struct test test2[] = {
 void *perf_test_thread(void *arg)
 {
 	char buff[MSG_SIZE];
-	int prio_out, prio_in;
+	int prio_out;
+	unsigned int prio_in;
 	int i;
 	clockid_t clock;
 	pthread_t *t;

base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
-- 
2.45.0

Re: [PATCH] selftests/mqueue: fix 5 warnings about signed/unsigned mismatches

Posted by Ryan Roberts 1 week, 5 days ago

On 05/05/2024 23:13, John Hubbard wrote:
> When building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftest
> 
> ...clang warns about several cases of using a signed integer for the
> priority argument to mq_receive(3), which expects an unsigned int.
> 
> Fix this by declaring the type as unsigned int in all cases.
> 
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/mqueue/mq_perf_tests.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c b/tools/testing/selftests/mqueue/mq_perf_tests.c
> index 5c16159d0bcd..fb898850867c 100644
> --- a/tools/testing/selftests/mqueue/mq_perf_tests.c
> +++ b/tools/testing/selftests/mqueue/mq_perf_tests.c
> @@ -323,7 +323,8 @@ void *fake_cont_thread(void *arg)
>  void *cont_thread(void *arg)
>  {
>  	char buff[MSG_SIZE];
> -	int i, priority;
> +	int i;
> +	unsigned int priority;
>  
>  	for (i = 0; i < num_cpus_to_pin; i++)
>  		if (cpu_threads[i] == pthread_self())
> @@ -425,7 +426,8 @@ struct test test2[] = {
>  void *perf_test_thread(void *arg)
>  {
>  	char buff[MSG_SIZE];
> -	int prio_out, prio_in;
> +	int prio_out;

It feels a bit odd for prio_out and prio_in to have different types. I don't
have any prior familiararity with these tests but looks like they are ultimately
the parameters of mq_send() and mq_receive() which both define them as unsigned
ints. Perhaps both should be converted?

> +	unsigned int prio_in;
>  	int i;
>  	clockid_t clock;
>  	pthread_t *t;
> 
> base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27

Re: [PATCH] selftests/mqueue: fix 5 warnings about signed/unsigned mismatches

Posted by John Hubbard 1 week, 5 days ago

On 5/7/24 12:54 AM, Ryan Roberts wrote:
> On 05/05/2024 23:13, John Hubbard wrote:
...
>> diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c b/tools/testing/selftests/mqueue/mq_perf_tests.c
>> index 5c16159d0bcd..fb898850867c 100644
>> --- a/tools/testing/selftests/mqueue/mq_perf_tests.c
>> +++ b/tools/testing/selftests/mqueue/mq_perf_tests.c
>> @@ -323,7 +323,8 @@ void *fake_cont_thread(void *arg)
>>   void *cont_thread(void *arg)
>>   {
>>   	char buff[MSG_SIZE];
>> -	int i, priority;
>> +	int i;
>> +	unsigned int priority;
>>   
>>   	for (i = 0; i < num_cpus_to_pin; i++)
>>   		if (cpu_threads[i] == pthread_self())
>> @@ -425,7 +426,8 @@ struct test test2[] = {
>>   void *perf_test_thread(void *arg)
>>   {
>>   	char buff[MSG_SIZE];
>> -	int prio_out, prio_in;
>> +	int prio_out;
> 
> It feels a bit odd for prio_out and prio_in to have different types. I don't
> have any prior familiararity with these tests but looks like they are ultimately
> the parameters of mq_send() and mq_receive() which both define them as unsigned
> ints. Perhaps both should be converted?


This makes sense, and I recall wondering about it. Looking at it again,
I see why didn't go that far: there is a mini-unit test manager inside,
passing around priorities that are signed, throughout:

         struct test {
         	char *desc; void (*func)(int *);
         };

         ...

         void inc_prio(int *prio) {
         	if (++*prio == mq_prio_max)
         		*prio = 0;
         }

However, I can probably fix up everything to match up. Given that you've
called it out, I'll go ahead with that approach. Iit will be quite a few
changes but they will all be trivial too.		


thanks,
-- 
John Hubbard
NVIDIA

> 
>> +	unsigned int prio_in;
>>   	int i;
>>   	clockid_t clock;
>>   	pthread_t *t;
>>
>> base-commit: f462ae0edd3703edd6f22fe41d336369c38b884b
>> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
>

Re: [PATCH] selftests/mqueue: fix 5 warnings about signed/unsigned mismatches

Posted by Ryan Roberts 1 week, 4 days ago

On 07/05/2024 18:04, John Hubbard wrote:
> On 5/7/24 12:54 AM, Ryan Roberts wrote:
>> On 05/05/2024 23:13, John Hubbard wrote:
> ...
>>> diff --git a/tools/testing/selftests/mqueue/mq_perf_tests.c
>>> b/tools/testing/selftests/mqueue/mq_perf_tests.c
>>> index 5c16159d0bcd..fb898850867c 100644
>>> --- a/tools/testing/selftests/mqueue/mq_perf_tests.c
>>> +++ b/tools/testing/selftests/mqueue/mq_perf_tests.c
>>> @@ -323,7 +323,8 @@ void *fake_cont_thread(void *arg)
>>>   void *cont_thread(void *arg)
>>>   {
>>>       char buff[MSG_SIZE];
>>> -    int i, priority;
>>> +    int i;
>>> +    unsigned int priority;
>>>         for (i = 0; i < num_cpus_to_pin; i++)
>>>           if (cpu_threads[i] == pthread_self())
>>> @@ -425,7 +426,8 @@ struct test test2[] = {
>>>   void *perf_test_thread(void *arg)
>>>   {
>>>       char buff[MSG_SIZE];
>>> -    int prio_out, prio_in;
>>> +    int prio_out;
>>
>> It feels a bit odd for prio_out and prio_in to have different types. I don't
>> have any prior familiararity with these tests but looks like they are ultimately
>> the parameters of mq_send() and mq_receive() which both define them as unsigned
>> ints. Perhaps both should be converted?
> 
> 
> This makes sense, and I recall wondering about it. Looking at it again,
> I see why didn't go that far: there is a mini-unit test manager inside,
> passing around priorities that are signed, throughout:
> 
>         struct test {
>             char *desc; void (*func)(int *);
>         };
> 
>         ...
> 
>         void inc_prio(int *prio) {
>             if (++*prio == mq_prio_max)
>                 *prio = 0;
>         }
> 
> However, I can probably fix up everything to match up. Given that you've
> called it out, I'll go ahead with that approach. Iit will be quite a few
> changes but they will all be trivial too.     

Ahh I see. It would certainly be an improvement, but if you don't think it's
worth the effort, then don't feel you need to do it on my account.

  
> 
> 
> thanks,