Re: [PATCH] remove nested functions from regcomp.c

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

>> This should be looked at first before considering any further changes,
>> however, Konstantin need not do this work himself, and we can help.

Any update with this? Is there anything I can do here?
This patch is the last one required to build libc.so with clang.
(A few more patches will be needed to build other targets, such as ld.so)

<shameless plug>
Building glibc with clang already provides benefits not currently
available with gcc: https://sourceware.org/glibc/wiki/FuzzingLibc (see
the last bullet)
</shameless plug>

On Tue, Sep 30, 2014 at 9:55 AM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Tue, Sep 30, 2014 at 7:44 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 09/30/2014 04:13 AM, Will Newton wrote:
>>> On 29 September 2014 21:54, Konstantin Serebryany
>>> <konstantin.s.serebryany@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> Please review the patch that removes nested functions from posix/regcomp.c.
>>>> The patch does not noticeably affect the generated code (same
>>>> instructions, a few differences in used registers, offsets, etc)
>>>> because all the affected functions have __attribute ((always_inline)).
>>>>
>>>> No regressions in 'make check' on x86_64-linux-gnu (Ubuntu 14.04)
>>>>
>>>> This code has some #ifdef branches for "not _LIBC",
>>>> please let me know if any separate testing is required for that case.
>>>
>>> This file is shared with gnulib so ideally the changes should be OKed
>>> there too. Unfortunately this particular file seems to have diverged a
>>> bit from gnulib so that might not be that straightforward.
>>
>> The first step is to synchronize this file with gnulib.
>>
>> This should be looked at first before considering any further changes,
>> however, Konstantin need not do this work himself, and we can help.
>
> Thanks!
> I'll wait for this being resolved and work on cleaning something else
> in the meantime.
> Let me know if there is anything I can do here.
>
>>
>> Lastly, we want *certainty* in our statements about compiler warnings
>> being wrong. A thorough reasoning needs to be brought forward to say
>> why the compiler warning is wrong. A hand-waving argument doesn't cut it.
>
> I've filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63418
>
> Note that there is a large number of similar bugs in gcc bugzilla:
> https://gcc.gnu.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=SUSPENDED&bug_status=WAITING&bug_status=REOPENED&field0-0-0=product&field0-0-1=component&field0-0-2=alias&field0-0-3=short_desc&field0-0-4=status_whiteboard&field0-0-5=content&list_id=98897&order=bug_id%20DESC&query_based_on=&query_format=advanced&type0-0-0=substring&type0-0-1=substring&type0-0-2=substring&type0-0-3=substring&type0-0-4=substring&type0-0-5=matches&value0-0-0=Wmaybe-uninitialized&value0-0-1=Wmaybe-uninitialized&value0-0-2=Wmaybe-uninitialized&value0-0-3=Wmaybe-uninitialized&value0-0-4=Wmaybe-uninitialized&value0-0-5=%22Wmaybe-uninitialized%22
> In my experience, this gcc warning never worked properly.
>
> Here is a tiny example that demonstrates that the warning is shut down
> in the presence of nested functions, this is why we did not see it
> before on regcomp.c
> (There is no point in complaining to GCC about this, IMHO, since
> nested functions are known to be poorly supported).
>
> % cat local_func_warning.c
> static static_func(unsigned cond, int *a) { *a = 1; }
> void foo(unsigned cond, int *b) {
>   int *a;
>   if (cond) a = b;
>
> #ifdef USE_NESTED
>   void inline
>   __attribute ((always_inline))
>   nested_func() {
>     *a = 1;
>   }
>   nested_func();
> #else
>   static_func1(cond, a);
> #endif
> }
> % ~/gcc-inst/bin/gcc -O2  -c   -Wmaybe-uninitialized local_func_warning.c
> local_func_warning.c: In function âfooâ:
> local_func_warning.c:14:3: warning: âaâ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>    static_func1(cond, a);
>    ^
> % ~/gcc-inst/bin/gcc -O2  -c   -Wmaybe-uninitialized
> local_func_warning.c -DUSE_NESTED
> %
>
> --kcc
>
>
>
>
>>
>> Cheers,
>> Carlos.
>>