This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] __builtin_expect cleanup for iconv{,data}/*.c
- From: Florian Weimer <fweimer at redhat dot com>
- To: Kalle Olavi Niemitalo <kon at iki dot fi>, libc-alpha <libc-alpha at sourceware dot org>
- Date: Wed, 10 Sep 2014 09:45:48 +0200
- Subject: Re: [PATCH v2] __builtin_expect cleanup for iconv{,data}/*.c
- Authentication-results: sourceware.org; auth=none
- References: <540E06B0 dot 50406 at redhat dot com> <540F501B dot 2080808 at redhat dot com> <87ha0g4adb dot fsf at Niukka dot kon dot iki dot fi>
On 09/10/2014 01:55 AM, Kalle Olavi Niemitalo wrote:
Florian Weimer <fweimer@redhat.com> writes:
--- a/iconv/gconv_simple.c
+++ b/iconv/gconv_simple.c
@@ -387,8 +387,7 @@ ucs4_internal_loop_single (struct __gconv_step *step,
return __GCONV_INCOMPLETE_INPUT;
}
- if (__builtin_expect (((unsigned char *) state->__value.__wchb)[0] > 0x80,
- 0))
+ if (__glibc_unlikely (((unsigned char *) state->__value.__wchb)[0] > 0x80))
I think that is a bug in the original code. Should compare > 0x7f.
Likewise in internal_ucs4_loop_unaligned.
Contrast to ucs4_internal_loop, which compares inval > 0x7fffffff.
This might be the case. But it seems this code is effectively
dead—iconv only converts complete multi-byte sequences, and the mbr*
need a matching locale, and we haven't got a UCS-4 locale (multi-byte
character sets which regularly use NUL bytes do not work with C).
Please file a bug if you think this still needs fixing, especially if
you can write a test case which shows the issue.
- if (__builtin_expect (flags & GCONV_AVOID_NOCONV, 0)
+ if (__glibc_unlikely (flags & GCONV_AVOID_NOCONV)
This looks a bit suspicious because it is not obvious that the
flags & GCONV_AVOID_NOCONV expression always evaluates to 0 or 1.
I'm not sure __glibc_unlikely ((flags & GCONV_AVOID_NOCONV) != 0)
would be any nicer, though.
Has glibc a rule not to rely on implicit booleans? Than the != 0
variant would be preferred.
- /*if (__builtin_expect (ch, 0) == __UNKNOWN_10646_CHAR)*/ \
+ /*if (__glibc_unlikely (ch == __UNKNOWN_10646_CHAR))*/ \
if (__glibc_unlikely (ch == __UNKNOWN_10646_CHAR)) \
I don't think that comment has any value left.
Okay, I'll remove it.
I didn't finish reading the patch yet.
Thanks so far!
--
Florian Weimer / Red Hat Product Security