From: Stefano Stabellini <sstabellini@kernel.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
"Roger Pau Monné" <roger.pau@citrix.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"Bertrand Marquis" <bertrand.marquis@arm.com>,
"Michal Orzel" <michal.orzel@amd.com>,
"Julien Grall" <julien@xen.org>,
xen-devel@lists.xenproject.org, nicola.vetrini@bugseng.com,
consulting@bugseng.com
Subject: Re: [PATCH] lib/strtoul: fix MISRA R10.2 violation
Date: Wed, 15 May 2024 15:47:58 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2405151547260.2544314@ubuntu-linux-20-04-desktop> (raw)
In-Reply-To: <a3fa7be4-48ce-4b68-a8f7-f94df58d97de@suse.com>
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.
prev parent reply other threads:[~2024-05-15 22:48 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-14 0:32 [PATCH] lib/strtoul: fix MISRA R10.2 violation Stefano Stabellini
2024-05-14 8:46 ` Jan Beulich
2024-05-14 22:52 ` Stefano Stabellini
2024-05-15 6:24 ` Jan Beulich
2024-05-15 22:47 ` Stefano Stabellini [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.22.394.2405151547260.2544314@ubuntu-linux-20-04-desktop \
--to=sstabellini@kernel.org \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=consulting@bugseng.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=michal.orzel@amd.com \
--cc=nicola.vetrini@bugseng.com \
--cc=roger.pau@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).