Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()

[Eric Blake]

On 07/20/2017 01:26 PM, Richard Henderson wrote:
> On 07/20/2017 06:31 AM, Peter Maydell wrote:
>> gdbstub.c:914:13: warning: array subscript has type 'char'
>>
>> This reflects the fact that toupper() and tolower() give
>> undefined behaviour if they are passed a value that isn't
>> a valid 'unsigned char' or EOF.
> 
> Not saying we shouldn't use qemu_tolower etc, but this statement is not
> true at all.  Officially, the argument to toupper and tolower is type int.

Correct. Officially, the argument to toupper is an int, but
range-constrained to the set of ints that represent either EOF or an
unsigned char value.  Calling toupper(char) (on platforms where char is
signed) is thus undefined behavior for chars < 0 (and which happen to
not coincide with EOF).

> 
> This sounds like a bug in NetBSD -- though it may not even be that, as
> they may have done something clever and put the symbol in the middle of
> the data.  A trick that worked before compiler warnings got smarter.

No, it is intentional of NetBSD.  In fact, the Cygwin environment also
INTENTIONALLY uses C99 magic to make gcc complain about calling
toupper(char) while being silent on toupper(int) and toupper(unsigned
char).  Because you really DO have portability problems (even if
toupper(char) _happens_ to work on most platforms, even where char is
signed, does not make it portable).  In fact, I'd love it if glibc would
also adopt the appropriate gcc magic to warn about nonportable usage of
ctype functions on signed char.

From Cygwin's ctype.h:

/* These macros are intentionally written in a manner that will trigger
   a gcc -Wall warning if the user mistakenly passes a 'char' instead
   of an int containing an 'unsigned char'.  Note that the sizeof will
   always be 1, which is what we want for mapping EOF to __CTYPE_PTR[0];
   the use of a raw index inside the sizeof triggers the gcc warning if
   __c was of type char, and sizeof masks side effects of the extra __c.
   Meanwhile, the real index to __CTYPE_PTR+1 must be cast to int,
   since isalpha(0x100000001LL) must equal isalpha(1), rather than being
   an out-of-bounds reference on a 64-bit machine.  */
#define __ctype_lookup(__c) ((__CTYPE_PTR+sizeof(""[__c]))[(int)(__c)])

#define isalpha(__c)    (__ctype_lookup(__c)&(_U|_L))

> 
> Anyway, should we poison the iso name so this doesn't creep in again?

Yes, for ALL of the ctype names where we work around the issue (and
maybe we should work around the issue in more places?)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature