* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 16:31 [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper() Peter Maydell
@ 2017-07-20 17:57 ` Eric Blake
2017-07-20 18:08 ` Christian Borntraeger
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2017-07-20 17:57 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Cornelia Huck, Alexander Graf, Christian Borntraeger,
Kamil Rytarowski, qemu-ppc, Paolo Bonzini, David Gibson,
Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
On 07/20/2017 11:31 AM, Peter Maydell wrote:
> On NetBSD, where tolower() and toupper() are implemented using an
> array lookup, the compiler warns if you pass a plain 'char'
> to these functions:
>
> 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.
>
> We have qemu_tolower() and qemu_toupper() to avoid this problem;
> use them.
>
> (The use in scsi-generic.c does not trigger the warning because
> it passes a uint8_t; we switch it anyway, for consistency.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 16:31 [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper() Peter Maydell
2017-07-20 17:57 ` Eric Blake
@ 2017-07-20 18:08 ` Christian Borntraeger
2017-07-20 18:26 ` Richard Henderson
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Christian Borntraeger @ 2017-07-20 18:08 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Richard Henderson, Alexander Graf, Cornelia Huck,
Paolo Bonzini, David Gibson, Kamil Rytarowski, qemu-ppc
On 07/20/2017 06:31 PM, Peter Maydell wrote:
> On NetBSD, where tolower() and toupper() are implemented using an
> array lookup, the compiler warns if you pass a plain 'char'
> to these functions:
>
> 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.
>
> We have qemu_tolower() and qemu_toupper() to avoid this problem;
> use them.
>
> (The use in scsi-generic.c does not trigger the warning because
> it passes a uint8_t; we switch it anyway, for consistency.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> for the s390 part.
> ---
> gdbstub.c | 2 +-
> hw/s390x/s390-virtio-ccw.c | 2 +-
> hw/scsi/scsi-generic.c | 2 +-
> target/ppc/monitor.c | 4 ++--
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index f936ddd..2a94030 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -911,7 +911,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>
> cur_action = *p++;
> if (cur_action == 'C' || cur_action == 'S') {
> - cur_action = tolower(cur_action);
> + cur_action = qemu_tolower(cur_action);
> res = qemu_strtoul(p + 1, &p, 16, &tmp);
> if (res) {
> goto out;
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index ce3921e..1c7af39 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -318,7 +318,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
> int i;
>
> for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) {
> - uint8_t c = toupper(val[i]); /* mimic HMC */
> + uint8_t c = qemu_toupper(val[i]); /* mimic HMC */
>
> if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') ||
> (c == ' ')) {
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index a55ff87..7e1cbab 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -406,7 +406,7 @@ static int read_naa_id(const uint8_t *p, uint64_t *p_wwn)
> }
> *p_wwn = 0;
> for (i = 8; i < 24; i++) {
> - char c = toupper(p[i]);
> + char c = qemu_toupper(p[i]);
> c -= (c >= '0' && c <= '9' ? '0' : 'A' - 10);
> *p_wwn = (*p_wwn << 4) | c;
> }
> diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
> index b8f30e9..1491511 100644
> --- a/target/ppc/monitor.c
> +++ b/target/ppc/monitor.c
> @@ -115,14 +115,14 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval)
> CPUPPCState *env = &cpu->env;
>
> /* General purpose registers */
> - if ((tolower(name[0]) == 'r') &&
> + if ((qemu_tolower(name[0]) == 'r') &&
> ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->gpr), ®num)) {
> *pval = env->gpr[regnum];
> return 0;
> }
>
> /* Floating point registers */
> - if ((tolower(name[0]) == 'f') &&
> + if ((qemu_tolower(name[0]) == 'f') &&
> ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->fpr), ®num)) {
> *pval = env->fpr[regnum];
> return 0;
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 16:31 [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper() Peter Maydell
2017-07-20 17:57 ` Eric Blake
2017-07-20 18:08 ` Christian Borntraeger
@ 2017-07-20 18:26 ` Richard Henderson
2017-07-20 18:48 ` Eric Blake
2017-07-20 21:03 ` Peter Maydell
2017-07-20 23:58 ` David Gibson
2017-07-21 10:23 ` Peter Maydell
4 siblings, 2 replies; 13+ messages in thread
From: Richard Henderson @ 2017-07-20 18:26 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Alexander Graf, Cornelia Huck, Christian Borntraeger,
Paolo Bonzini, David Gibson, Kamil Rytarowski, qemu-ppc
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.
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.
Anyway, should we poison the iso name so this doesn't creep in again?
Reviewed-by: Richard Henderson <rth@twiddle.net>
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 18:26 ` Richard Henderson
@ 2017-07-20 18:48 ` Eric Blake
2017-07-20 18:57 ` Richard Henderson
2017-07-20 21:03 ` Peter Maydell
1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2017-07-20 18:48 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Cornelia Huck, Alexander Graf, Christian Borntraeger,
Kamil Rytarowski, qemu-ppc, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 18:48 ` Eric Blake
@ 2017-07-20 18:57 ` Richard Henderson
2017-07-20 19:04 ` Eric Blake
0 siblings, 1 reply; 13+ messages in thread
From: Richard Henderson @ 2017-07-20 18:57 UTC (permalink / raw)
To: Eric Blake, Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Cornelia Huck, Alexander Graf, Christian Borntraeger,
Kamil Rytarowski, qemu-ppc, Paolo Bonzini, David Gibson
On 07/20/2017 08:48 AM, Eric Blake wrote:
> 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.
Please show me that language in C11 (or any other version).
My copy has:
7.4.2.2 The toupper function
Synopsis
1 #include <ctype.h>
int toupper(int c);
Description
2 The toupper function converts a lowercase letter to a corresponding
uppercase letter.
Returns
3 If the argument is a character for which islower is true and there
are one or more corresponding characters, as specified by the current
locale, for which isupper is true, the toupper function returns one
of the corresponding characters (always the same one for any given
locale); otherwise, the argument is returned unchanged.
I see nothing about range constraints. The word "unsigned" does not appear.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 18:57 ` Richard Henderson
@ 2017-07-20 19:04 ` Eric Blake
2017-07-20 19:14 ` Richard Henderson
0 siblings, 1 reply; 13+ messages in thread
From: Eric Blake @ 2017-07-20 19:04 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Cornelia Huck, Alexander Graf, Christian Borntraeger,
Kamil Rytarowski, qemu-ppc, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]
On 07/20/2017 01:57 PM, Richard Henderson wrote:
> On 07/20/2017 08:48 AM, Eric Blake wrote:
>> 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.
>
> Please show me that language in C11 (or any other version).
POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/toupper.html
> For toupper(): [CX] [Option Start] The functionality described on this reference page is aligned with the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. This volume of POSIX.1-2008 defers to the ISO C standard. [Option End]
>
> The toupper() [CX] [Option Start] and toupper_l() [Option End] functions have as a domain a type int, the value of which is representable as an unsigned char or the value of EOF. If the argument has any other value, the behavior is undefined.
So POSIX is claiming it is a C99 requirement.
And I found this in C11:
7.4 Character handling <ctype.h>
1 The header <ctype.h> declares several functions useful for classifying
and mapping
characters. 198) In all cases the argument is an int, the value of which
shall be
representable as an unsigned char or shall equal the value of the macro
EOF. If the
argument has any other value, the behavior is undefined.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 19:04 ` Eric Blake
@ 2017-07-20 19:14 ` Richard Henderson
0 siblings, 0 replies; 13+ messages in thread
From: Richard Henderson @ 2017-07-20 19:14 UTC (permalink / raw)
To: Eric Blake, Peter Maydell, qemu-devel, qemu-trivial
Cc: patches, Cornelia Huck, Alexander Graf, Christian Borntraeger,
Kamil Rytarowski, qemu-ppc, Paolo Bonzini, David Gibson
On 07/20/2017 09:04 AM, Eric Blake wrote:
> And I found this in C11:
>
> 7.4 Character handling <ctype.h>
> 1 The header <ctype.h> declares several functions useful for classifying
> and mapping
> characters. 198) In all cases the argument is an int, the value of which
> shall be
> representable as an unsigned char or shall equal the value of the macro
> EOF. If the
> argument has any other value, the behavior is undefined.
Thanks. I should have looked up there.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 18:26 ` Richard Henderson
2017-07-20 18:48 ` Eric Blake
@ 2017-07-20 21:03 ` Peter Maydell
2017-07-20 21:29 ` Eric Blake
1 sibling, 1 reply; 13+ messages in thread
From: Peter Maydell @ 2017-07-20 21:03 UTC (permalink / raw)
To: Richard Henderson
Cc: QEMU Developers, QEMU Trivial, patches@linaro.org, Alexander Graf,
Cornelia Huck, Christian Borntraeger, Paolo Bonzini, David Gibson,
Kamil Rytarowski, qemu-ppc@nongnu.org
On 20 July 2017 at 19:26, Richard Henderson <rth@twiddle.net> 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.
(I was going to quote POSIX here but I see Eric has done so already.)
> 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.
https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/sys/ctype_inline.h
The implementation is
#define toupper(c) ((int)((_toupper_tab_ + 1)[(c)]))
(where I assume _toupper_tab_ is the obvious array.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 21:03 ` Peter Maydell
@ 2017-07-20 21:29 ` Eric Blake
2017-07-20 21:42 ` Peter Maydell
0 siblings, 1 reply; 13+ messages in thread
From: Eric Blake @ 2017-07-20 21:29 UTC (permalink / raw)
To: Peter Maydell, Richard Henderson
Cc: patches@linaro.org, QEMU Trivial, Cornelia Huck, Alexander Graf,
QEMU Developers, Christian Borntraeger, Kamil Rytarowski,
qemu-ppc@nongnu.org, Paolo Bonzini, David Gibson
[-- Attachment #1: Type: text/plain, Size: 2781 bytes --]
On 07/20/2017 04:03 PM, Peter Maydell wrote:
> On 20 July 2017 at 19:26, Richard Henderson <rth@twiddle.net> 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.
>
> (I was going to quote POSIX here but I see Eric has done so already.)
>
>> 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.
>
> https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/sys/ctype_inline.h
> The implementation is
> #define toupper(c) ((int)((_toupper_tab_ + 1)[(c)]))
>
> (where I assume _toupper_tab_ is the obvious array.)
In fact, if NetBSD's implementation is anything like Cygwin's, then it
is a safe bet that _toupper_tab_ is a symbol that lands in the middle of
a larger 384-byte array, such that _toupper_tab_[-1] (for (signed
char)'\xfe') happens to resolve to a valid image address, and will have
the same contents as _toupper_tab_[255] (for (unsigned char)'\xfe'),
precisely to cater to so many clueless programs have forgotten about
signed char widening to negative values (even though the standard says
the behavior is undefined, quality-of-implementation demands that you
try to do the sane thing anyway).
The table is of course only MOSTLY symmetric; there are some single-byte
locales where isspace((unsigned char)'\xff') is true but isspace(EOF) is
false (in using the magic-middle-of-array locations, _tab_[0] cannot
always match _tab_[256] in all locales). And it is wraparound where
'\xff' collides with -1 that explains WHY C99 documents the range to be
ints corresponding to unsigned char.
Side note: NetBSD has a bug in their ctype.h.
toupper(INT64_C(0x100000001)) is supposed to be the same as toupper(1),
since POSIX requires toupper() to be available as a function (and
permits a macro as well). A function with an int parameter must
gracefully truncate a 64-bit input down to 32-bits, so the macro must do
the same; but the NetBSD macro uses all 64 bits to dereference into
garbage memory locations. Fortunately, no one runs into the bug in real
life because no one really tries to pass 64-bit numbers to ctype macros.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 21:29 ` Eric Blake
@ 2017-07-20 21:42 ` Peter Maydell
0 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2017-07-20 21:42 UTC (permalink / raw)
To: Eric Blake
Cc: Richard Henderson, patches@linaro.org, QEMU Trivial,
Cornelia Huck, Alexander Graf, QEMU Developers,
Christian Borntraeger, Kamil Rytarowski, qemu-ppc@nongnu.org,
Paolo Bonzini, David Gibson
On 20 July 2017 at 22:29, Eric Blake <eblake@redhat.com> wrote:
> On 07/20/2017 04:03 PM, Peter Maydell wrote:
>> https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/sys/ctype_inline.h
>> The implementation is
>> #define toupper(c) ((int)((_toupper_tab_ + 1)[(c)]))
>>
>> (where I assume _toupper_tab_ is the obvious array.)
>
> In fact, if NetBSD's implementation is anything like Cygwin's, then it
> is a safe bet that _toupper_tab_ is a symbol that lands in the middle of
> a larger 384-byte array, such that _toupper_tab_[-1] (for (signed
> char)'\xfe') happens to resolve to a valid image address, and will have
> the same contents as _toupper_tab_[255] (for (unsigned char)'\xfe'),
> precisely to cater to so many clueless programs have forgotten about
> signed char widening to negative values (even though the standard says
> the behavior is undefined, quality-of-implementation demands that you
> try to do the sane thing anyway).
Nope, looks like they just implement as _toupper_tab_[0] is for
EOF, and indexes >0 are for the characters, if I've found the
right bit of their libc:
https://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/lib/libc/locale/rune.c
thanks
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 16:31 [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper() Peter Maydell
` (2 preceding siblings ...)
2017-07-20 18:26 ` Richard Henderson
@ 2017-07-20 23:58 ` David Gibson
2017-07-21 10:23 ` Peter Maydell
4 siblings, 0 replies; 13+ messages in thread
From: David Gibson @ 2017-07-20 23:58 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-trivial, patches, Richard Henderson,
Alexander Graf, Cornelia Huck, Christian Borntraeger,
Paolo Bonzini, Kamil Rytarowski, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 3669 bytes --]
On Thu, Jul 20, 2017 at 05:31:30PM +0100, Peter Maydell wrote:
> On NetBSD, where tolower() and toupper() are implemented using an
> array lookup, the compiler warns if you pass a plain 'char'
> to these functions:
>
> 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.
>
> We have qemu_tolower() and qemu_toupper() to avoid this problem;
> use them.
>
> (The use in scsi-generic.c does not trigger the warning because
> it passes a uint8_t; we switch it anyway, for consistency.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
ppc parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> gdbstub.c | 2 +-
> hw/s390x/s390-virtio-ccw.c | 2 +-
> hw/scsi/scsi-generic.c | 2 +-
> target/ppc/monitor.c | 4 ++--
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index f936ddd..2a94030 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -911,7 +911,7 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>
> cur_action = *p++;
> if (cur_action == 'C' || cur_action == 'S') {
> - cur_action = tolower(cur_action);
> + cur_action = qemu_tolower(cur_action);
> res = qemu_strtoul(p + 1, &p, 16, &tmp);
> if (res) {
> goto out;
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index ce3921e..1c7af39 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -318,7 +318,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
> int i;
>
> for (i = 0; i < sizeof(ms->loadparm) && val[i]; i++) {
> - uint8_t c = toupper(val[i]); /* mimic HMC */
> + uint8_t c = qemu_toupper(val[i]); /* mimic HMC */
>
> if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') || (c == '.') ||
> (c == ' ')) {
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index a55ff87..7e1cbab 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -406,7 +406,7 @@ static int read_naa_id(const uint8_t *p, uint64_t *p_wwn)
> }
> *p_wwn = 0;
> for (i = 8; i < 24; i++) {
> - char c = toupper(p[i]);
> + char c = qemu_toupper(p[i]);
> c -= (c >= '0' && c <= '9' ? '0' : 'A' - 10);
> *p_wwn = (*p_wwn << 4) | c;
> }
> diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c
> index b8f30e9..1491511 100644
> --- a/target/ppc/monitor.c
> +++ b/target/ppc/monitor.c
> @@ -115,14 +115,14 @@ int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval)
> CPUPPCState *env = &cpu->env;
>
> /* General purpose registers */
> - if ((tolower(name[0]) == 'r') &&
> + if ((qemu_tolower(name[0]) == 'r') &&
> ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->gpr), ®num)) {
> *pval = env->gpr[regnum];
> return 0;
> }
>
> /* Floating point registers */
> - if ((tolower(name[0]) == 'f') &&
> + if ((qemu_tolower(name[0]) == 'f') &&
> ppc_cpu_get_reg_num(name + 1, ARRAY_SIZE(env->fpr), ®num)) {
> *pval = env->fpr[regnum];
> return 0;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper()
2017-07-20 16:31 [Qemu-devel] [PATCH for-2.10] Use qemu_tolower() and qemu_toupper(), not tolower() and toupper() Peter Maydell
` (3 preceding siblings ...)
2017-07-20 23:58 ` David Gibson
@ 2017-07-21 10:23 ` Peter Maydell
4 siblings, 0 replies; 13+ messages in thread
From: Peter Maydell @ 2017-07-21 10:23 UTC (permalink / raw)
To: QEMU Developers, QEMU Trivial
Cc: patches@linaro.org, Cornelia Huck, Alexander Graf,
Christian Borntraeger, Kamil Rytarowski, qemu-ppc@nongnu.org,
Paolo Bonzini, David Gibson, Richard Henderson
On 20 July 2017 at 17:31, Peter Maydell <peter.maydell@linaro.org> wrote:
> On NetBSD, where tolower() and toupper() are implemented using an
> array lookup, the compiler warns if you pass a plain 'char'
> to these functions:
>
> 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.
>
> We have qemu_tolower() and qemu_toupper() to avoid this problem;
> use them.
>
> (The use in scsi-generic.c does not trigger the warning because
> it passes a uint8_t; we switch it anyway, for consistency.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Applied to master; thanks all for fast reviews/acks.
-- PMM
^ permalink raw reply [flat|nested] 13+ messages in thread