This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] __builtin_expect cleanup for iconv{,data}/*.c

On 09/10/2014 01:55 AM, Kalle Olavi Niemitalo wrote:
Florian Weimer <> 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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]