Message ID | 55062712.4040104@cs.ucla.edu |
---|---|
State | New |
Headers | show |
On Sun, Mar 15, 2015 at 5:42 PM, Paul Eggert <eggert@cs.ucla.edu> wrote: > I reverted it. Thanks, > Sorry about that; it had a horrible typo (!= vs ==). Oh, right. I should have noticed. > Does > the attached (untested) patch work for you instead? It fixes the typo, and > also pacifies GCC so that GCC does not issue the bogus warning. I've tested the patch on Linux/x86_64. Looks good. +/* Pacify GCC; see the commentary about VALLEN below. This is needed + at least through GCC 4.9.2. Pacify GCC for the entire file, as + there seems to be no way to pacify GCC selectively, only for the + place where it's needed. Do not use DIAG_IGNORE_NEEDS_COMMENT + here, as it's not defined yet. */ +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" You can be more selective with '#pragma GCC diagnostic push/pop' around the function. Clearly you know about DIAG_IGNORE_NEEDS_COMMENT (I didn't), so I assume you also know about DIAG_PUSH_NEEDS_COMMENT / DIAG_POP_NEEDS_COMMENT, which then makes me not understand your comment about selective pacification.
Paul Pluzhnikov wrote: > I assume you also know about DIAG_PUSH_NEEDS_COMMENT / > DIAG_POP_NEEDS_COMMENT, which then makes me not understand your > comment about selective pacification. When I tried to use those macros, I still got the warning and the build still failed. Part of the problem is that 'vallen' gets used as part of an argument to the __mempcpy/memcpy macros, and GCC decides that since 'vallen' is used in the macro body, it'll issue a warning since the macro body is outside the "scope" of the DIAG_PUSH* and DIAG_POP* brackets. It's the classic problem of macro capture. I couldn't even use plain DIAG_IGNORE_NEEDS_COMMENT (i.e., without DIAG_PUSH_NEEDS_COMMENT), because I needed to put the #pragma before including <errno.h> (the first standard include) -- otherwise I got the warning -- and DIAG_IGNORE_NEEDS_COMMENT isn't defined until after <errno.h> is included. I was using GCC 4.9.2 20150212 (Red Hat 4.9.2-6), for what that's worth.
On Sun, Mar 15, 2015 at 9:11 PM, Paul Eggert <eggert@cs.ucla.edu> wrote: > When I tried to use those macros, I still got the warning and the build > still failed. ... > I was using GCC 4.9.2 20150212 (Red Hat 4.9.2-6), for what that's worth. I see. I reproduced that "bad behavior" using GCC trunk @r221169, but it works as I expected using GCC 4.8.2-19ubuntu1. That seems like a GCC mis-feature.
On Sun, 15 Mar 2015, Paul Eggert wrote: > I couldn't even use plain DIAG_IGNORE_NEEDS_COMMENT (i.e., without > DIAG_PUSH_NEEDS_COMMENT), because I needed to put the #pragma before including > <errno.h> (the first standard include) -- otherwise I got the warning -- and > DIAG_IGNORE_NEEDS_COMMENT isn't defined until after <errno.h> is included. Any file using those macros should explicitly include <libc-internal.h> to get their definitions. Are you saying that if you include <libc-internal.h>, then use the macros, then include other headers, that it doesn't work?
On Sun, Mar 15, 2015 at 6:10 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > > On Sun, Mar 15, 2015 at 5:42 PM, Paul Eggert <eggert@cs.ucla.edu> wrote: > I've tested the patch on Linux/x86_64. Looks good. The patch appears to have stalled. I believe we still want it: the current situation is really not good. Thanks,
Paul Pluzhnikov wrote: > The patch appears to have stalled. > I believe we still want it: the current situation is really not good. Yes. Since that patch <https://sourceware.org/ml/libc-alpha/2015-03/msg00527.html> fixes a bug and has been reviewed and tested, I would like to install it now. We can install any further improvements to libc-internal.h later.
Paul Eggert wrote: > Since that patch > <https://sourceware.org/ml/libc-alpha/2015-03/msg00527.html> fixes a bug and has > been reviewed and tested, I would like to install it now. I've done that. Thanks for reminding me about it.
From 7d298c5558df36cf0e6a46940ace042c52284264 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sun, 15 Mar 2015 17:38:10 -0700 Subject: [PATCH] * stdlib/setenv.c (__add_to_environ): Dump core quickly if setenv (..., NULL, ...) is called. This time, do it the right way, and pacify GCC with a pragma. --- ChangeLog | 4 ++++ stdlib/setenv.c | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index c856f79..e61cc17 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2015-03-15 Paul Eggert <eggert@cs.ucla.edu> + * stdlib/setenv.c (__add_to_environ): + Dump core quickly if setenv (..., NULL, ...) is called. + This time, do it the right way, and pacify GCC with a pragma. + * stdlib/setenv.c (__add_to_environ): Revert previous change. 2015-03-14 Andreas Schwab <schwab@linux-m68k.org> diff --git a/stdlib/setenv.c b/stdlib/setenv.c index b60c4f0..184a8cd 100644 --- a/stdlib/setenv.c +++ b/stdlib/setenv.c @@ -19,6 +19,13 @@ # include <config.h> #endif +/* Pacify GCC; see the commentary about VALLEN below. This is needed + at least through GCC 4.9.2. Pacify GCC for the entire file, as + there seems to be no way to pacify GCC selectively, only for the + place where it's needed. Do not use DIAG_IGNORE_NEEDS_COMMENT + here, as it's not defined yet. */ +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" + #include <errno.h> #if !_LIBC # if !defined errno && !defined HAVE_ERRNO_DECL @@ -114,8 +121,17 @@ __add_to_environ (name, value, combined, replace) { char **ep; size_t size; + + /* Compute lengths before locking, so that the critical section is + less of a performance bottleneck. VALLEN is needed only if + COMBINED is null (unfortunately GCC is not smart enough to deduce + this; see the #pragma at the start of this file). Testing + COMBINED instead of VALUE causes setenv (..., NULL, ...) to dump + core now instead of corrupting memory later. */ const size_t namelen = strlen (name); - const size_t vallen = value != NULL ? strlen (value) + 1 : 0; + size_t vallen; + if (combined == NULL) + vallen = strlen (value) + 1; LOCK; -- 2.1.0