Re: [RESEND][PATCH v2] Fix ctype(3) usage in the kconfig code on NetBSD

[Kamil Rytarowski <n54@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5618 bytes --]

On 17.05.2017 23:24, Andrew Morton wrote:
> On Wed, 17 May 2017 15:33:53 +0200 Kamil Rytarowski <n54@gmx.com> wrote:
> 
>> The current code produces set of warnings on NetBSD-7.99.25 (GCC 4.8.5):
>>
>> In file included from scripts/kconfig/zconf.tab.c:2576:0:
>> scripts/kconfig/confdata.c: In function 'conf_expand_value':
>> scripts/kconfig/confdata.c:97:3:
>> warning: array subscript has type 'char' [-Wchar-subscripts]
>>    while (isalnum(*src) || *src == '_')
>>    ^
>> scripts/kconfig/confdata.c: In function 'conf_set_sym_val':
>> scripts/kconfig/confdata.c:155:4:
>> warning: array subscript has type 'char' [-Wchar-subscripts]
>>     for (p2 = p; *p2 && !isspace(*p2); p2++)
>>     ^
>> scripts/kconfig/confdata.c: In function 'tristate_print_symbol':
>> scripts/kconfig/confdata.c:617:3:
>> warning: array subscript has type 'char' [-Wchar-subscripts]
>>    fprintf(fp, "%s%s=%c\n", CONFIG_, sym->name, (char)toupper(*value));
>>    ^
>>
>> Fix this portability issue by explicit casting to unsigned char.
>>
>> ...
>>
>>  scripts/basic/fixdep.c               |  2 +-
>>  scripts/kconfig/conf.c               |  6 +++---
>>  scripts/kconfig/confdata.c           |  9 +++++----
>>  scripts/kconfig/expr.c               |  3 ++-
>>  scripts/kconfig/lxdialog/checklist.c |  3 ++-
>>  scripts/kconfig/lxdialog/inputbox.c  |  3 ++-
>>  scripts/kconfig/lxdialog/menubox.c   | 11 +++++++----
>>  scripts/kconfig/lxdialog/util.c      |  5 +++--
>>  scripts/kconfig/menu.c               |  4 ++--
>>  scripts/kconfig/nconf.c              |  3 ++-
>>  scripts/kconfig/nconf.gui.c          |  3 ++-
>>  scripts/kconfig/symbol.c             |  8 ++++----
>>  12 files changed, 35 insertions(+), 25 deletions(-)
> 
> This patch is irritating :(  The Linux manpage says
> 
> "These functions check whether c, which must have the value of an
> unsigned char or EOF..."
> 
> so it's legit enough.
> 
> Apart from being ugly, it isn't very maintainable: people on Linux
> systems will add new instances while not including the typecasts.
> 

I proposed this casting as it's a common idiom in portable software
using ctype(3) in C.

Examples in GNU software (I used "grep -r 'unsigned char'" and randomly
picked some sequential lines):

gpl2/gmake/dist/glob/fnmatch.c:             if ((STREQ (str, "alnum") &&
ISALNUM ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "alpha")
&& ISALPHA ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "blank")
&& ISBLANK ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "cntrl")
&& ISCNTRL ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "digit")
&& ISDIGIT ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "graph")
&& ISGRAPH ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "lower")
&& ISLOWER ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "print")
&& ISPRINT ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "punct")
&& ISPUNCT ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "space")
&& ISSPACE ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str, "upper")
&& ISUPPER ((unsigned char) *n))
gpl2/gmake/dist/glob/fnmatch.c:                 || (STREQ (str,
"xdigit") && ISXDIGIT ((unsigned char) *n)))

gpl2/groff/dist/src/libs/libgroff/strtol.c:  while (ISASCII((unsigned
char)*str) && isspace((unsigned char)*str))
gpl2/groff/dist/src/libs/libgroff/strtol.c:  p = strchr(digits,
(ISASCII((unsigned char)*str)
gpl2/groff/dist/src/libs/libgroff/strtol.c:                   &&
isupper((unsigned char)*str)
gpl2/groff/dist/src/libs/libgroff/strtol.c:                   ?
tolower((unsigned char)*str)

gpl2/gettext/dist/gettext-runtime/intl/l10nflist.c:     if (isalpha
((unsigned char) codeset[cnt]))
gpl2/gettext/dist/gettext-runtime/intl/l10nflist.c:       *wp++ =
tolower ((unsigned char) codeset[cnt]);
gpl2/gettext/dist/gettext-runtime/intl/l10nflist.c:     else if (isdigit
((unsigned char) codeset[cnt]))
gpl2/gettext/dist/gettext-runtime/intl/localealias.c:      while
(isspace ((unsigned char) cp[0]))
gpl2/gettext/dist/gettext-runtime/intl/localealias.c:     while (cp[0]
!= '\0' && !isspace ((unsigned char) cp[0]))
gpl2/gettext/dist/gettext-runtime/intl/localealias.c:     while (isspace
((unsigned char) cp[0]))

I also noted it already in the Linux kernel related software (the one I
got under hand to grep):

gpl2/dtc/dist/dtc-lexer.l:                      while
(!isdigit((unsigned char)*line))
gpl2/dtc/dist/dtc-lexer.l:                      while
(!isspace((unsigned char)*tmp))


I put aside discussion about prettiness of C.

> I wonder if there's anothing we can do in the Makefiles to suppress
> that warning in scripts/ for netbsd systems?  ie, disable
> -Wchar-subscripts in that case?
> 

This is another option, to hide warnings.

As of now "menuconfig" does not crash, however I haven't tested it on
signed vs unsigned char type platforms. LIBC implementations on Linux
are generally more tolerant to passing values out of valid range and
hide bugs, that sometimes crash with our native LIBC... and from time to
time some people try to "fix" our compliant implementation:

http://gnats.netbsd.org/34632 "isalpha() and possibly other ctype
functions segfault"


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]