[PATCH] remove nested function hack_digit

[Konstantin Serebryany]

I've attached a patch that addresses your comments.

2014-09-26  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>
        * stdio-common/printf_fp.c
        (hack_digit): New function, broken out of ...
        (__printf_fp): ... local function here.  Update call sites.
        hack_digit now takes an additional parameter that is a pointer
        to a struct of the referenced locals.  Those locals moved inside
        the struct and references updated.


On Fri, Sep 26, 2014 at 12:38 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> Here is a better formatted and tested patch that uses a single parameter.
>> The generated code is nearly identical (same instructions, differences
>> only in constant offsets).
>
> This change looks mostly OK to me.  It's not clear to me what you're saying
> this version's generated code is identical to.  If you're just saying it's
> identical to the version of the patch that differs only cosmetically, then
> that's not interesting.  The clear comparison of the trunk code to the
> patched code is what we need.

The comparison is between the trunk and the current patch.
The differences are minimal.

hack_digit has the same number of instructions and the diff mostly
look like this:
23c24
<    4a39b:     74 14                   je     4a3b1 <hack_digit.13608+0x61>
---
>    4a39b:     74 14                   je     4a3b1 <hack_digit+0x61>
25,27c26,28
<    4a3a1:     48 8d 4a 01             lea    0x1(%rdx),%rcx
<    4a3a5:     48 89 4b 18             mov    %rcx,0x18(%rbx)
<    4a3a9:     48 8b 4b 10             mov    0x10(%rbx),%rcx
---
>    4a3a1:     48 8b 4b 10             mov    0x10(%rbx),%rcx
>    4a3a5:     48 8d 72 01             lea    0x1(%rdx),%rsi
>    4a3a9:     48 89 73 18             mov    %rsi,0x18(%rbx)
...

The changes in __printf_fp are also minimal (same number of instructions)

> It's not especially helpful to attach the
> whole disassembly (we can all compile it for ourselves).  What's important
> is to do the careful analysis of the code differences and explain their
> expected performance impact.

I don't see any changes that may impact performance.

>
>> +2014-09-15  Kostya Serebryany  <konstantin.s.serebryany@gmail.com>
>
> In patch submissions, the log entry should not be part of the patch.
>
>> +     * stdio-common/printf_fp.c (hack_digit): New function.
>> +     (___printf_fp): Remove nested function hack_digit. Call non-nested
>> +     function hack_digit.
>
> Two spaces between sentences.  I would have written this:
>
>         * stdio-common/printf_fp.c
>         (hack_digit): New function, broken out of ...
>         (__printf_fp): ... local function here.  Update call sites.
>         hack_digit now takes an additional parameter that is a pointer
>         to a struct of the referenced locals.  Those locals moved inside
>         the struct and references updated.
>
>> +static wchar_t hack_digit (struct hack_digit_param *p)
>> +{
>
> The parts of the declaration before the function name get their own line,
> with the function name starting the next line.
>
> You didn't report what testing you did on this patch.

I've runs "make check".

On my system I get same number of failures with and w/o the patch:

    107 FAIL
   1616 PASS
    121 XFAIL
      3 XPASS

All of the tests in stdio-common pass.

The failures I see are in nptl (many) and in these tests
FAIL: debug/tst-backtrace2
FAIL: debug/tst-backtrace3
FAIL: debug/tst-backtrace4
FAIL: debug/tst-backtrace5
FAIL: debug/tst-backtrace6
FAIL: dlfcn/bug-atexit3
FAIL: elf/check-abi-libc
FAIL: grp/testgrp
FAIL: posix/globtest
FAIL: rt/tst-cpuclock2
FAIL: rt/tst-mqueue8
FAIL: rt/tst-mqueue8x

off-topic -- how do I achieve a clean "make check" run on Ubuntu 14.04?
This is what I do:
../glibc/configure --prefix=$HOME/glibc-clang/inst && make -j 40 &&
make -j 40 check

--kcc

>
>
> Thanks,
> Roland
-------------- next part --------------
A non-text attachment was scrubbed...
Name: unnest-hack_digit-single-param-2.patch
Type: text/x-patch
Size: 28587 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20140926/7bde3740/attachment.bin>