| Message ID | alpine.DEB.2.22.394.2405131729180.2544314@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lib/strtoul: fix MISRA R10.2 violation | expand |
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. Jan > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > --- > I believe this is the last R10.2 violation > > diff --git a/xen/lib/strtoul.c b/xen/lib/strtoul.c > index a378fe735e..345dcf9d8c 100644 > --- a/xen/lib/strtoul.c > +++ b/xen/lib/strtoul.c > @@ -38,7 +38,7 @@ unsigned long simple_strtoul( > > while ( isxdigit(*cp) && > (value = isdigit(*cp) ? *cp - '0' > - : toupper(*cp) - 'A' + 10) < base ) > + : (char)toupper(*cp) - 'A' + 10) < base ) > { > result = result * base + value; > cp++;
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[]; -#define __ismask(x) (_ctype[(int)(unsigned char)(x)]) +#define __ismask(x) (_ctype[(int)(char)(x)]) #define isalnum(c) ((__ismask(c)&(_U|_L|_D)) != 0) #define isalpha(c) ((__ismask(c)&(_U|_L)) != 0) @@ -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'; diff --git a/xen/lib/ctype.c b/xen/lib/ctype.c index 7b233a335f..27e5b61b1c 100644 --- a/xen/lib/ctype.c +++ b/xen/lib/ctype.c @@ -1,7 +1,7 @@ #include <xen/ctype.h> /* for ctype.h */ -const unsigned char _ctype[] = { +const char _ctype[] = { _C,_C,_C,_C,_C,_C,_C,_C, /* 0-7 */ _C,_C|_S,_C|_S,_C|_S,_C|_S,_C|_S,_C,_C, /* 8-15 */ _C,_C,_C,_C,_C,_C,_C,_C, /* 16-23 */
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
On Wed, 15 May 2024, Jan Beulich wrote: > 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)) Oh I see. This is much more similar to the original suggestion from Bugseng. Let me send a v2.
diff --git a/xen/lib/strtoul.c b/xen/lib/strtoul.c index a378fe735e..345dcf9d8c 100644 --- a/xen/lib/strtoul.c +++ b/xen/lib/strtoul.c @@ -38,7 +38,7 @@ unsigned long simple_strtoul( while ( isxdigit(*cp) && (value = isdigit(*cp) ? *cp - '0' - : toupper(*cp) - 'A' + 10) < base ) + : (char)toupper(*cp) - 'A' + 10) < base ) { result = result * base + value; cp++;
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. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- I believe this is the last R10.2 violation