[PATCH] kdb: match return value to function signature

[PATCH] kdb: match return value to function signature

[Nicholas Mc Guire]

kdb_task_state_string() introduced in the initial commit 5d5314d6795f
("kdb: core for kgdb back end (1 of 2)") returns unsigned long (a bit
array of states) but intermediately it is being assigned to a long which
make static code checkers unhappy and also does not not help readability
(technically there is no reason to use a signed type here).

Type-checking coccinelle spatches are being used to locate type mismatches
between function signatures and return values in this case it produced:
./kernel/debug/kdb/kdb_support.c:611 WARNING: return of wrong type
                                 unsigned long != long

Patch was compile tested with x86_64_defconfig + CONFIG_KGDB=y,
CONFIG_KGDB_KDB=y

Patch is against 4.1-rc3 (localversion-next is -next-20150512)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 kernel/debug/kdb/kdb_support.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

All (5) call-sites of kdb_task_state_string() were checked and all are
expecting an unsigned long as the function signature provides - so this
change should have no effect as automatic type conversion did not make
the signed type visible externally and internally the signed nature was
also not in use.

Doc fixup: long -> unsigned long only (and some reformatting this caused)

diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index d35cc2d..0bb3b81 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -544,12 +544,12 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
  * Returns:
  *	Mask for process state.
  * Notes:
- *	The mask folds data from several sources into a single long value, so
- *	be careful not to overlap the bits.  TASK_* bits are in the LSB,
- *	special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1 there
- *	is no overlap between TASK_* and EXIT_* but that may not always be
- *	true, so EXIT_* bits are shifted left 16 bits before being stored in
- *	the mask.
+ *	The mask folds data from several sources into a single unsigned long
+ *      value, so be careful not to overlap the bits.  TASK_* bits are in the
+ *      LSB, special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1
+ *      there is no overlap between TASK_* and EXIT_* but that may not always
+ *      be true, so EXIT_* bits are shifted left 16 bits before being stored
+ *      in the mask.
  */
 
 /* unrunnable is < 0 */
@@ -560,7 +560,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
 
 unsigned long kdb_task_state_string(const char *s)
 {
-	long res = 0;
+	unsigned long res = 0;
 	if (!s) {
 		s = kdbgetenv("PS");
 		if (!s)
-- 
1.7.10.4

Re: [PATCH] kdb: match return value to function signature

[Daniel Thompson]

On 12/05/15 13:46, Nicholas Mc Guire wrote:
> kdb_task_state_string() introduced in the initial commit 5d5314d6795f
> ("kdb: core for kgdb back end (1 of 2)") returns unsigned long (a bit
> array of states) but intermediately it is being assigned to a long which
> make static code checkers unhappy and also does not not help readability
> (technically there is no reason to use a signed type here).
>
> Type-checking coccinelle spatches are being used to locate type mismatches
> between function signatures and return values in this case it produced:
> ./kernel/debug/kdb/kdb_support.c:611 WARNING: return of wrong type
>                                   unsigned long != long
>
> Patch was compile tested with x86_64_defconfig + CONFIG_KGDB=y,
> CONFIG_KGDB_KDB=y
>
> Patch is against 4.1-rc3 (localversion-next is -next-20150512)
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>   kernel/debug/kdb/kdb_support.c |   14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> All (5) call-sites of kdb_task_state_string() were checked and all are
> expecting an unsigned long as the function signature provides - so this
> change should have no effect as automatic type conversion did not make
> the signed type visible externally and internally the signed nature was
> also not in use.
>
> Doc fixup: long -> unsigned long only (and some reformatting this caused)
>
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index d35cc2d..0bb3b81 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -544,12 +544,12 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
>    * Returns:
>    *	Mask for process state.
>    * Notes:
> - *	The mask folds data from several sources into a single long value, so
> - *	be careful not to overlap the bits.  TASK_* bits are in the LSB,
> - *	special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1 there
> - *	is no overlap between TASK_* and EXIT_* but that may not always be
> - *	true, so EXIT_* bits are shifted left 16 bits before being stored in
> - *	the mask.
> + *	The mask folds data from several sources into a single unsigned long
> + *      value, so be careful not to overlap the bits.  TASK_* bits are in the
> + *      LSB, special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1
> + *      there is no overlap between TASK_* and EXIT_* but that may not always
> + *      be true, so EXIT_* bits are shifted left 16 bits before being stored
> + *      in the mask.

The ragged alignment here looks like a spurious replacement of tabs with 
spaces.

BTW having the type of the variable int he comment is pointless anyway. 
If you simply remove the type from the comment ("folds data into several 
sources into a single value") then you don't have to reflow the comment 
in the first place.

>    */
>
>   /* unrunnable is < 0 */
> @@ -560,7 +560,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
>
>   unsigned long kdb_task_state_string(const char *s)
>   {
> -	long res = 0;
> +	unsigned long res = 0;
>   	if (!s) {
>   		s = kdbgetenv("PS");
>   		if (!s)
>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

[providing the above tab/space comment above addressed]

Re: [PATCH] kdb: match return value to function signature

[Nicholas Mc Guire]

On Tue, 12 May 2015, Daniel Thompson wrote:

> On 12/05/15 13:46, Nicholas Mc Guire wrote:
>> kdb_task_state_string() introduced in the initial commit 5d5314d6795f
>> ("kdb: core for kgdb back end (1 of 2)") returns unsigned long (a bit
>> array of states) but intermediately it is being assigned to a long which
>> make static code checkers unhappy and also does not not help readability
>> (technically there is no reason to use a signed type here).
>>
>> Type-checking coccinelle spatches are being used to locate type mismatches
>> between function signatures and return values in this case it produced:
>> ./kernel/debug/kdb/kdb_support.c:611 WARNING: return of wrong type
>>                                   unsigned long != long
>>
>> Patch was compile tested with x86_64_defconfig + CONFIG_KGDB=y,
>> CONFIG_KGDB_KDB=y
>>
>> Patch is against 4.1-rc3 (localversion-next is -next-20150512)
>>
>> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
>> ---
>>   kernel/debug/kdb/kdb_support.c |   14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> All (5) call-sites of kdb_task_state_string() were checked and all are
>> expecting an unsigned long as the function signature provides - so this
>> change should have no effect as automatic type conversion did not make
>> the signed type visible externally and internally the signed nature was
>> also not in use.
>>
>> Doc fixup: long -> unsigned long only (and some reformatting this caused)
>>
>> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
>> index d35cc2d..0bb3b81 100644
>> --- a/kernel/debug/kdb/kdb_support.c
>> +++ b/kernel/debug/kdb/kdb_support.c
>> @@ -544,12 +544,12 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
>>    * Returns:
>>    *	Mask for process state.
>>    * Notes:
>> - *	The mask folds data from several sources into a single long value, so
>> - *	be careful not to overlap the bits.  TASK_* bits are in the LSB,
>> - *	special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1 there
>> - *	is no overlap between TASK_* and EXIT_* but that may not always be
>> - *	true, so EXIT_* bits are shifted left 16 bits before being stored in
>> - *	the mask.
>> + *	The mask folds data from several sources into a single unsigned long
>> + *      value, so be careful not to overlap the bits.  TASK_* bits are in the
>> + *      LSB, special cases like UNRUNNABLE are in the MSB.  As of 2.6.10-rc1
>> + *      there is no overlap between TASK_* and EXIT_* but that may not always
>> + *      be true, so EXIT_* bits are shifted left 16 bits before being stored
>> + *      in the mask.
>
> The ragged alignment here looks like a spurious replacement of tabs with  
> spaces.
>
> BTW having the type of the variable int he comment is pointless anyway.  
> If you simply remove the type from the comment ("folds data into several  
> sources into a single value") then you don't have to reflow the comment  
> in the first place.
>

makes sense - will resubmit without the noise around the doc fix.

>>    */
>>
>>   /* unrunnable is < 0 */
>> @@ -560,7 +560,7 @@ int kdb_putword(unsigned long addr, unsigned long word, size_t size)
>>
>>   unsigned long kdb_task_state_string(const char *s)
>>   {
>> -	long res = 0;
>> +	unsigned long res = 0;
>>   	if (!s) {
>>   		s = kdbgetenv("PS");
>>   		if (!s)
>>
>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>

thx!
hofrat