[Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Richard PALO]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

I'm noticing on catching up recently, e.g. on 20150717, that there is the following issue:
> /home/richard/ws/illumos-gate/usr/src/common/acpica/components/events/evgpeinit.c: In function 'AcpiEvMatchGpeMethod':
> /home/richard/ws/illumos-gate/usr/src/common/acpica/components/events/evgpeinit.c:398: error: implicit declaration of function 'strtoul' [-Wimplicit-function-declaration]

strtoul is indirectly defined in acsolaris via a system header as:

I like to propose the following patch substituting for a simplified, specific conversion
in the file events/evgpeinit.c.
>extern unsigned long strtoul(const char *, char **, int);

This seems to be the only real use of strtoul with _KERNEL defined.

Looking at the code in question, for two hex digits is doesn't seem the worth to use it so I propose
to replace as follows:
> diff --git a/usr/src/common/acpica/components/events/evgpeinit.c b/usr/src/common/acpica/components/events/evgpeinit.c
> index d23a345..de1e15a 100644
> --- a/usr/src/common/acpica/components/events/evgpeinit.c
> +++ b/usr/src/common/acpica/components/events/evgpeinit.c
> @@ -395,8 +395,22 @@ AcpiEvMatchGpeMethod (
>  
>      /* 4) The last two characters of the name are the hex GPE Number */
>  
> -    GpeNumber = strtoul (&Name[2], NULL, 16);
> -    if (GpeNumber == ACPI_UINT32_MAX)
> +    /* GpeNumber = strtoul (&Name[2], NULL, 16); */
> +    /*
> +     * As this function can be called in KERNEL, not all systems have
> +     * strtoul() available.  Since this is only for a single hex value,
> +     * manually convert nibble by nibble, checking for possible errors.
> +     */
> +    GpeNumber = isdigit(Name[2]) ? (Name[2] - '0')<<4 :
> +       isxdigit(Name[2]) ? (toupper(Name[2]) - 'A' + 10)<<4 : 0xFFFF;
> +
> +    if (GpeNumber != 0xFFFF) {
> +        GpeNumber = isdigit(Name[3]) ? GpeNumber + (Name[3] - '0') :
> +           isxdigit(Name[3]) ? GpeNumber + (toupper(Name[3]) - 'A' + 10) :
> +           0xFFFF;
> +    }
> +
> +    if (GpeNumber == 0xFFFF || Name[4] != '\0')
>      {
>          /* Conversion failed; invalid method, just ignore it */
>  

-- 
Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2753 bytes --]

Sure, but the original code is much simpler.


> -----Original Message-----
> From: Devel [mailto:devel-bounces(a)acpica.org] On Behalf Of Richard PALO
> Sent: Friday, September 04, 2015 4:12 AM
> To: devel(a)acpica.org
> Subject: [Devel] PATCH proposal removing use of strtoul in
> events/evgpeinit.c
> 
> I'm noticing on catching up recently, e.g. on 20150717, that there is the
> following issue:
> > /home/richard/ws/illumos-
> gate/usr/src/common/acpica/components/events/evgpeinit.c: In function
> 'AcpiEvMatchGpeMethod':
> > /home/richard/ws/illumos-gate/usr/src/common/acpica/components/events/
> > evgpeinit.c:398: error: implicit declaration of function 'strtoul'
> > [-Wimplicit-function-declaration]
> 
> strtoul is indirectly defined in acsolaris via a system header as:
> 
> I like to propose the following patch substituting for a simplified,
> specific conversion in the file events/evgpeinit.c.
> >extern unsigned long strtoul(const char *, char **, int);
> 
> This seems to be the only real use of strtoul with _KERNEL defined.
> 
> Looking at the code in question, for two hex digits is doesn't seem the
> worth to use it so I propose to replace as follows:
> > diff --git a/usr/src/common/acpica/components/events/evgpeinit.c
> > b/usr/src/common/acpica/components/events/evgpeinit.c
> > index d23a345..de1e15a 100644
> > --- a/usr/src/common/acpica/components/events/evgpeinit.c
> > +++ b/usr/src/common/acpica/components/events/evgpeinit.c
> > @@ -395,8 +395,22 @@ AcpiEvMatchGpeMethod (
> >
> >      /* 4) The last two characters of the name are the hex GPE Number
> > */
> >
> > -    GpeNumber = strtoul (&Name[2], NULL, 16);
> > -    if (GpeNumber == ACPI_UINT32_MAX)
> > +    /* GpeNumber = strtoul (&Name[2], NULL, 16); */
> > +    /*
> > +     * As this function can be called in KERNEL, not all systems have
> > +     * strtoul() available.  Since this is only for a single hex value,
> > +     * manually convert nibble by nibble, checking for possible errors.
> > +     */
> > +    GpeNumber = isdigit(Name[2]) ? (Name[2] - '0')<<4 :
> > +       isxdigit(Name[2]) ? (toupper(Name[2]) - 'A' + 10)<<4 : 0xFFFF;
> > +
> > +    if (GpeNumber != 0xFFFF) {
> > +        GpeNumber = isdigit(Name[3]) ? GpeNumber + (Name[3] - '0') :
> > +           isxdigit(Name[3]) ? GpeNumber + (toupper(Name[3]) - 'A' +
> 10) :
> > +           0xFFFF;
> > +    }
> > +
> > +    if (GpeNumber == 0xFFFF || Name[4] != '\0')
> >      {
> >          /* Conversion failed; invalid method, just ignore it */
> >
> 
> --
> Richard PALO
> 
> _______________________________________________
> Devel mailing list
> Devel(a)acpica.org
> https://lists.acpica.org/mailman/listinfo/devel

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Richard PALO]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

Le 04/09/15 15:58, Moore, Robert a écrit :
> Sure, but the original code is much simpler.
> 
 
The reason for the error was that, by default, the extern declaration was forcluded by
_DDI_STRICT being defined.. 
which is to say that strtoul, strictly speaking is unavailable in the Solaris kernel (excepting kmdb).

So I kludged and added to acsolaris.h #ifdef _KERNEL section :
> static __GNU_INLINE unsigned long
> strtoul(const char *str, char **nptr, int base)
> {
> 	unsigned long ret;
> 	int status;
> 
> 	if ((status = ddi_strtoul(str, nptr, base, &ret)) != 0) {
> 		if (status == ERANGE)
> 			return (ULONG_MAX);
> 		return (0);
> 	}
> 	return (ret);
> }

Still seems for one call in the kernel to convert a single HEX byte from ascii to unsigned long, that 
the portable solution is to avoid using 'strtoul' in paths operating in kernel space.
(also considering the Linux discussion also around strtoul in the kernel)

cheers,
-- 
Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

This is hardly the only use of strtoul in ACPICA. May or may not be an issue on solaris, depending on the kernel/ACPICA configuration.


> -----Original Message-----
> From: Richard PALO [mailto:richard(a)netbsd.org]
> Sent: Friday, September 04, 2015 11:02 PM
> To: Moore, Robert; devel(a)acpica.org
> Subject: Re: [Devel] PATCH proposal removing use of strtoul in
> events/evgpeinit.c
> 
> Le 04/09/15 15:58, Moore, Robert a écrit :
> > Sure, but the original code is much simpler.
> >
> 
> The reason for the error was that, by default, the extern declaration was
> forcluded by _DDI_STRICT being defined..
> which is to say that strtoul, strictly speaking is unavailable in the
> Solaris kernel (excepting kmdb).
> 
> So I kludged and added to acsolaris.h #ifdef _KERNEL section :
> > static __GNU_INLINE unsigned long
> > strtoul(const char *str, char **nptr, int base) {
> > 	unsigned long ret;
> > 	int status;
> >
> > 	if ((status = ddi_strtoul(str, nptr, base, &ret)) != 0) {
> > 		if (status == ERANGE)
> > 			return (ULONG_MAX);
> > 		return (0);
> > 	}
> > 	return (ret);
> > }
> 
> Still seems for one call in the kernel to convert a single HEX byte from
> ascii to unsigned long, that the portable solution is to avoid using
> 'strtoul' in paths operating in kernel space.
> (also considering the Linux discussion also around strtoul in the kernel)
> 
> cheers,
> --
> Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

I've got a better question:

> which is to say that strtoul, strictly speaking is unavailable in the
> Solaris kernel (excepting kmdb).

Why not?

> -----Original Message-----
> From: Richard PALO [mailto:richard(a)netbsd.org]
> Sent: Friday, September 04, 2015 11:02 PM
> To: Moore, Robert; devel(a)acpica.org
> Subject: Re: [Devel] PATCH proposal removing use of strtoul in
> events/evgpeinit.c
> 
> Le 04/09/15 15:58, Moore, Robert a écrit :
> > Sure, but the original code is much simpler.
> >
> 
> The reason for the error was that, by default, the extern declaration was
> forcluded by _DDI_STRICT being defined..
> which is to say that strtoul, strictly speaking is unavailable in the
> Solaris kernel (excepting kmdb).
> 
> So I kludged and added to acsolaris.h #ifdef _KERNEL section :
> > static __GNU_INLINE unsigned long
> > strtoul(const char *str, char **nptr, int base) {
> > 	unsigned long ret;
> > 	int status;
> >
> > 	if ((status = ddi_strtoul(str, nptr, base, &ret)) != 0) {
> > 		if (status == ERANGE)
> > 			return (ULONG_MAX);
> > 		return (0);
> > 	}
> > 	return (ret);
> > }
> 
> Still seems for one call in the kernel to convert a single HEX byte from
> ascii to unsigned long, that the portable solution is to avoid using
> 'strtoul' in paths operating in kernel space.
> (also considering the Linux discussion also around strtoul in the kernel)
> 
> cheers,
> --
> Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Richard PALO]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

Le 09/09/15 20:01, Moore, Robert a écrit :
> I've got a better question:
> 
>> which is to say that strtoul, strictly speaking is unavailable in the
>> Solaris kernel (excepting kmdb).
> 
> Why not?
> 
>

Given numerous other build problems involving ACPI_DISASSEMLER in _KERNEL, those are disabled for now
(issues with MpSave* which I believe other platforms are experiencing as well, just like the visibility of AcpiUtConvertStringToUuid).

I believe other uses of strtoul may tend to be concentrated there...

As to the history of decisions involving ddi_strtoul/strtoul, a veteran sun kernel developer needs to be asked...

In any event, currently the wrapper around ddi_strtoul seems reasonable enough.

BTW, have things been linted recently (we use sun studio lint)? 

I had to add the following to acsolaris.h
> #ifdef __SUNPRO_C
> #pragma error_messages(off, E_STATIC_UNUSED)
> #pragma error_messages(off, E_FUNC_ARG_UNUSED)
> #pragma error_messages(off, E_END_OF_LOOP_CODE_NOT_REACHED)
> #pragma error_messages(off, E_FUNC_RET_MAYBE_IGNORED2)
> #endif

not to mention kludge needed when linting global crosschecks in the illumos gate, for the later
> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dispatcher/dsobject.c", line 351: warning: function returns value which is sometimes ignored: memcpy (E_FUNC_RET_MAYBE_IGNORED2)
> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dispatcher/dsutils.c", line 608: warning: function returns value which is sometimes ignored: strncpy (E_FUNC_RET_MAYBE_IGNORED2)
> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/executer/exmisc.c", line 370: warning: function returns value which is sometimes ignored: strcpy (E_FUNC_RET_MAYBE_IGNORED2)
> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/executer/exnames.c", line 224: warning: function returns value which is sometimes ignored: strcat (E_FUNC_RET_MAYBE_IGNORED2)

-- 
Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]



> -----Original Message-----
> From: Richard PALO [mailto:richard(a)netbsd.org]
> Sent: Friday, September 11, 2015 3:57 AM
> To: Moore, Robert; devel(a)acpica.org
> Subject: Re: [Devel] PATCH proposal removing use of strtoul in
> events/evgpeinit.c
> 
> Le 09/09/15 20:01, Moore, Robert a écrit :
> > I've got a better question:
> >
> >> which is to say that strtoul, strictly speaking is unavailable in the
> >> Solaris kernel (excepting kmdb).
> >
> > Why not?
> >
> >
> 
> Given numerous other build problems involving ACPI_DISASSEMLER in _KERNEL,
> those are disabled for now (issues with MpSave* which I believe other
> platforms are experiencing as well, just like the visibility of
> AcpiUtConvertStringToUuid).
> 
> I believe other uses of strtoul may tend to be concentrated there...
> 
> As to the history of decisions involving ddi_strtoul/strtoul, a veteran
> sun kernel developer needs to be asked...
> 
> In any event, currently the wrapper around ddi_strtoul seems reasonable
> enough.
> 
> BTW, have things been linted recently (we use sun studio lint)?

Every lint is different, unfortunately. And there are lots of things that we need to ignore, which in itself depends on which lint. So, it is mostly useless for us.


> 
> I had to add the following to acsolaris.h
> > #ifdef __SUNPRO_C
> > #pragma error_messages(off, E_STATIC_UNUSED) #pragma
> > error_messages(off, E_FUNC_ARG_UNUSED) #pragma error_messages(off,
> > E_END_OF_LOOP_CODE_NOT_REACHED) #pragma error_messages(off,
> > E_FUNC_RET_MAYBE_IGNORED2) #endif
> 
> not to mention kludge needed when linting global crosschecks in the
> illumos gate, for the later
> > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dispa
> > tcher/dsobject.c", line 351: warning: function returns value which is
> > sometimes ignored: memcpy (E_FUNC_RET_MAYBE_IGNORED2)
> > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dispa
> > tcher/dsutils.c", line 608: warning: function returns value which is
> > sometimes ignored: strncpy (E_FUNC_RET_MAYBE_IGNORED2)
> > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/execu
> > ter/exmisc.c", line 370: warning: function returns value which is
> > sometimes ignored: strcpy (E_FUNC_RET_MAYBE_IGNORED2)
> > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/execu
> > ter/exnames.c", line 224: warning: function returns value which is
> > sometimes ignored: strcat (E_FUNC_RET_MAYBE_IGNORED2)
> 
> --
> Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 3183 bytes --]

Not to mention that every compiler is different, and you need to put your own "ignore error" types of things in your platform-specific or compiler-specific header. Such is the price of attempting to build OS- and compiler-independent code.


> -----Original Message-----
> From: Moore, Robert
> Sent: Friday, September 11, 2015 7:54 AM
> To: 'Richard PALO'; devel(a)acpica.org
> Cc: Box, David E; Zheng, Lv
> Subject: RE: [Devel] PATCH proposal removing use of strtoul in
> events/evgpeinit.c
> 
> 
> 
> > -----Original Message-----
> > From: Richard PALO [mailto:richard(a)netbsd.org]
> > Sent: Friday, September 11, 2015 3:57 AM
> > To: Moore, Robert; devel(a)acpica.org
> > Subject: Re: [Devel] PATCH proposal removing use of strtoul in
> > events/evgpeinit.c
> >
> > Le 09/09/15 20:01, Moore, Robert a écrit :
> > > I've got a better question:
> > >
> > >> which is to say that strtoul, strictly speaking is unavailable in
> > >> the Solaris kernel (excepting kmdb).
> > >
> > > Why not?
> > >
> > >
> >
> > Given numerous other build problems involving ACPI_DISASSEMLER in
> > _KERNEL, those are disabled for now (issues with MpSave* which I
> > believe other platforms are experiencing as well, just like the
> > visibility of AcpiUtConvertStringToUuid).
> >
> > I believe other uses of strtoul may tend to be concentrated there...
> >
> > As to the history of decisions involving ddi_strtoul/strtoul, a
> > veteran sun kernel developer needs to be asked...
> >
> > In any event, currently the wrapper around ddi_strtoul seems
> > reasonable enough.
> >
> > BTW, have things been linted recently (we use sun studio lint)?
> 
> Every lint is different, unfortunately. And there are lots of things that
> we need to ignore, which in itself depends on which lint. So, it is mostly
> useless for us.
> 
> 
> >
> > I had to add the following to acsolaris.h
> > > #ifdef __SUNPRO_C
> > > #pragma error_messages(off, E_STATIC_UNUSED) #pragma
> > > error_messages(off, E_FUNC_ARG_UNUSED) #pragma error_messages(off,
> > > E_END_OF_LOOP_CODE_NOT_REACHED) #pragma error_messages(off,
> > > E_FUNC_RET_MAYBE_IGNORED2) #endif
> >
> > not to mention kludge needed when linting global crosschecks in the
> > illumos gate, for the later
> > > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dis
> > > pa tcher/dsobject.c", line 351: warning: function returns value
> > > which is sometimes ignored: memcpy (E_FUNC_RET_MAYBE_IGNORED2)
> > > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dis
> > > pa tcher/dsutils.c", line 608: warning: function returns value which
> > > is sometimes ignored: strncpy (E_FUNC_RET_MAYBE_IGNORED2)
> > > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/exe
> > > cu ter/exmisc.c", line 370: warning: function returns value which is
> > > sometimes ignored: strcpy (E_FUNC_RET_MAYBE_IGNORED2)
> > > "/home/richard/src/illumos-gate/usr/src/common/acpica/components/exe
> > > cu ter/exnames.c", line 224: warning: function returns value which
> > > is sometimes ignored: strcat (E_FUNC_RET_MAYBE_IGNORED2)
> >
> > --
> > Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Richard PALO]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

Le 11/09/15 16:57, Moore, Robert a écrit :
> Not to mention that every compiler is different, and you need to put your own "ignore error" types of things in your platform-specific or compiler-specific header. Such is the price of attempting to build OS- and compiler-independent code.
> 

>>> not to mention kludge needed when linting global crosschecks in the
>>> illumos gate, for the later
>>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dis
>>>> pa tcher/dsobject.c", line 351: warning: function returns value
>>>> which is sometimes ignored: memcpy (E_FUNC_RET_MAYBE_IGNORED2)
>>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/dis
>>>> pa tcher/dsutils.c", line 608: warning: function returns value which
>>>> is sometimes ignored: strncpy (E_FUNC_RET_MAYBE_IGNORED2)
>>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/exe
>>>> cu ter/exmisc.c", line 370: warning: function returns value which is
>>>> sometimes ignored: strcpy (E_FUNC_RET_MAYBE_IGNORED2)
>>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/exe
>>>> cu ter/exnames.c", line 224: warning: function returns value which
>>>> is sometimes ignored: strcat (E_FUNC_RET_MAYBE_IGNORED2)
>>>

? Sorry, I guess I don't follow. for example, posix, and various c standards define memcpy as 
>  void *memcpy(void *restrict s1, const void *restrict s2, size_t n);

the first reference above, components/dispatcher/dsobject.c does:

> 349         if (ByteList)
> 350         {
> 351             memcpy (ObjDesc->Buffer.Pointer, ByteList->Named.Data,
> 352                          ByteListLength);
> 353         }

what would make most static analyzers happy is a simple void cast -- (void) memcpy(...)

Then, at least, ignoring the return value is explicit instead of possibly 
just being forgotten.. 

This should also be the most portable form of C (at least for the cases at hand).

There are numerous examples of this sort of thing over and above what I listed above...

cheers,
-- 
Richard PALO

Re: [Devel] PATCH proposal removing use of strtoul in events/evgpeinit.c

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]

Agreed

> -----Original Message-----
> From: Richard PALO [mailto:richard(a)netbsd.org]
> Sent: Friday, September 11, 2015 8:36 AM
> To: Moore, Robert; devel(a)acpica.org
> Cc: Box, David E; Zheng, Lv
> Subject: Re: [Devel] PATCH proposal removing use of strtoul in
> events/evgpeinit.c
> 
> Le 11/09/15 16:57, Moore, Robert a écrit :
> > Not to mention that every compiler is different, and you need to put
> your own "ignore error" types of things in your platform-specific or
> compiler-specific header. Such is the price of attempting to build OS- and
> compiler-independent code.
> >
> 
> >>> not to mention kludge needed when linting global crosschecks in the
> >>> illumos gate, for the later
> >>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/di
> >>>> s pa tcher/dsobject.c", line 351: warning: function returns value
> >>>> which is sometimes ignored: memcpy (E_FUNC_RET_MAYBE_IGNORED2)
> >>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/di
> >>>> s pa tcher/dsutils.c", line 608: warning: function returns value
> >>>> which is sometimes ignored: strncpy (E_FUNC_RET_MAYBE_IGNORED2)
> >>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/ex
> >>>> e cu ter/exmisc.c", line 370: warning: function returns value which
> >>>> is sometimes ignored: strcpy (E_FUNC_RET_MAYBE_IGNORED2)
> >>>> "/home/richard/src/illumos-gate/usr/src/common/acpica/components/ex
> >>>> e cu ter/exnames.c", line 224: warning: function returns value
> >>>> which is sometimes ignored: strcat (E_FUNC_RET_MAYBE_IGNORED2)
> >>>
> 
> ? Sorry, I guess I don't follow. for example, posix, and various c
> standards define memcpy as
> >  void *memcpy(void *restrict s1, const void *restrict s2, size_t n);
> 
> the first reference above, components/dispatcher/dsobject.c does:
> 
> > 349         if (ByteList)
> > 350         {
> > 351             memcpy (ObjDesc->Buffer.Pointer, ByteList->Named.Data,
> > 352                          ByteListLength);
> > 353         }
> 
> what would make most static analyzers happy is a simple void cast --
> (void) memcpy(...)
> 
> Then, at least, ignoring the return value is explicit instead of possibly
> just being forgotten..
> 
> This should also be the most portable form of C (at least for the cases at
> hand).
> 

Agreed. Some of these have apparently slipped in, so we will put it on our list.


> There are numerous examples of this sort of thing over and above what I
> listed above...
> 
> cheers,
> --
> Richard PALO