Re: [PATCH 1/25] Remove nested functions: crypt/md5-crypt.c

[Konstantin Serebryany <konstantin dot s dot serebryany at gmail dot com>]

On Tue, May 20, 2014 at 7:46 PM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Tue, May 20, 2014 at 07:25:52PM +0400, Konstantin Serebryany wrote:
>> With salt == "$1$" I get this profile:
>>  84.70%  a.out  libcrypt-2.15.so   [.] __md5_process_block
>>   5.98%  a.out  libc-2.15.so       [.] __memcpy_ssse3_back
>>   4.71%  a.out  libcrypt-2.15.so   [.] __md5_process_bytes
>>   1.92%  a.out  libcrypt-2.15.so   [.] __md5_crypt_r
>>   1.62%  a.out  libcrypt-2.15.so   [.] __md5_finish_ctx
>>
>> Again, __md5_crypt_r is within noise.
>
> That's good.
>
>>
>> >> Another thing that can be done is to look at the code generated for the
>> >> relevant file before and after the patch - I'd expect very little change.
>>
>> I like this method much more, but there is actually a change.
>> The original binary has calls to b64_from_24bit.7858 (the nested function),
>> while in the binary built with my patch these calls are inlined.
>> Of course, this is a feature of a particular version of GCC, not of
>> the glibc code.
>
> Given that this does not have a very big impact, I'm inclined to
> accepting this change.  I'll push it in on Thursday after noon UTC if
> nobody objects.

I realized that two other files have the same situation with the
function b64_from_24bit.
md5-crypt.c, sha256-crypt.c, and sha512-crypt.c have exactly the same
implementations of b64_from_24bit as a nested function.
One possible fix is to move those two out and make them regular static
functions, just like I did in md5-crypt.c.

Another solution is to create just one implementation of
b64_from_24bit and place it somewhere else.
For example declare it as __b64_from_24bit in crypt-private.h and
implement it in crypt_util.c.

WDYT?