Re: [PATCH] lib/strtoul: fix MISRA R10.2 violation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2024 00:52, Stefano Stabellini wrote:
> On Tue, 14 May 2024, Jan Beulich wrote:
>> On 14.05.2024 02:32, Stefano Stabellini wrote:
>>> Fix last violation of R10.2 by casting the result of toupper to plain
>>> char. Note that we don't want to change toupper itself as it is a legacy
>>> interface and it would cause more issues.
>>
>> Can you point me at a single example where a new issue would arise? All
>> places I've spotted (including tolower() uses) would appear to benefit
>> from changing toupper() / tolower() themselves. Further, since they are
>> both wrapper macros only anyway, if any concern remained, fiddling with
>> the wrapper macros while leaving alone the underlying inline functions
>> would allow any such use site to simply be switched to using the inline
>> functions directly. As said, from looking at it I don't expect that
>> would be necessary, so instead I'd rather hope that eventually we can
>> do away with the wrapper macros, renaming the inline functions
>> accordingly.
> 
> If we change __toupper to return a plain char, then there are a few
> other things we need to change for consistency, see below. To be honest
> I thought it would cause more problems. I am OK to go with that if you
> all agree. (Nicola please have a look in case this introduces more
> issues elsewhere.)
> 
> 
> diff --git a/xen/include/xen/ctype.h b/xen/include/xen/ctype.h
> index 6dec944a37..6a6854e01c 100644
> --- a/xen/include/xen/ctype.h
> +++ b/xen/include/xen/ctype.h
> @@ -15,9 +15,9 @@
>  #define _X   0x40    /* hex digit */
>  #define _SP  0x80    /* hard space (0x20) */
>  
> -extern const unsigned char _ctype[];
> +extern const char _ctype[];

Why would this be needed? I can't see a connection to toupper() / tolower().

> -#define __ismask(x) (_ctype[(int)(unsigned char)(x)])
> +#define __ismask(x) (_ctype[(int)(char)(x)])

This almost certainly is wrong. Whether plain char is signed or unsigned is
left to the compiler, and it being signed would result in possibly negative
array indexes. Again I can't see a connection to the issue at hand.

> @@ -34,14 +34,14 @@ extern const unsigned char _ctype[];
>  #define isascii(c) (((unsigned char)(c))<=0x7f)
>  #define toascii(c) (((unsigned char)(c))&0x7f)
>  
> -static inline unsigned char __tolower(unsigned char c)
> +static inline char __tolower(char c)
>  {
>       if (isupper(c))
>               c -= 'A'-'a';
>       return c;
>  }
>  
> -static inline unsigned char __toupper(unsigned char c)
> +static inline char __toupper(char c)
>  {
>       if (islower(c))
>               c -= 'a'-'A';

This isn't what I had suggested. First I said to leave alone the double-
underscore prefixed functions, and only touch the wrapper macros (as a
precaution in case any use site exists which relies on present behavior).
And then I didn't suggest to alter parameter types; only the return type
would need adjustment, I think, for what you're aiming at:

#define tolower(c) ((char)__tolower(c))
#define toupper(c) ((char)__toupper(c))

Jan