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

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

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.
>