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

[Florian Weimer]

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