This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix -Os related -Werror failures.
On 10/28/2016 08:08 AM, Carlos O'Donell wrote:
> On 10/28/2016 02:32 AM, Florian Weimer wrote:
>> On 10/28/2016 06:46 AM, Carlos O'Donell wrote:
>>> +/* With GCC 5.3 when compiling with -Os the compiler emits a warning
>>> + that buf[0] and buf[1] may be used uninitialized. This can only
>>> + happen in the case where tmpbuf[3] is used, and in that case the
>>> + write to the tmpbuf[1] and tmpbuf[2] was assured because
>>> + ucs4_to_cns11643 would have filled in those entries. The difficulty
>>> + is in getting the compiler to see this logic because tmpbuf[0] is
>>> + involved in determining the code page and is the indicator that
>>> + tmpbuf[2] is initialized. */
>>> +DIAG_PUSH_NEEDS_COMMENT;
>>> +DIAG_IGNORE_NEEDS_COMMENT (5.3, "-Wmaybe-uninitialized");
>>
>> This hides the warning for -O2 builds as well, so I don't think this is a good idea.
>>
>> Those who want to build with -Os or other special compiler flags
>> should just configure with --disable-werror. We can't account for
>> every optimization someone might want to disable in their build.
>
> I agree that we can't account for _all_ optimizations someone might want
> to disable in their build, but I think it is a reasonable goal to target
> a few key _default_ optimization including -O3, -O2, and -Os.
>
> In the case above we only limit the emitted warnings for the narrow
> code involved in iso-2022-cn-ext conversions. I'd be more worried if it
> required a widely used function with broadly disabled warnings.
>
> I agree with Arnd that this code is _overly_ complex and could be
> rewritten such that it's a little clearer and makes sense to the compiler
> at -Os.
>
> Should I try to cleanup the BODY code a bit to remove this particular
> diagnostic disabling?
>
> I know we've had several real uninitialized variable problems in the
> conversion code recently, so I'm also interested in having the compiler
> help us find more of these problems.
For example, initializing the tmpbuf in this fallback case is enough to
silence the compiler warning:
diff --git a/iconvdata/iso-2022-cn-ext.c b/iconvdata/iso-2022-cn-ext.c
index df5b5df..d0b32df 100644
--- a/iconvdata/iso-2022-cn-ext.c
+++ b/iconvdata/iso-2022-cn-ext.c
@@ -456,7 +456,7 @@ enum
used = CNS11643_2_set; \
else \
{ \
- unsigned char tmpbuf[3]; \
+ unsigned char tmpbuf[3] = { 0, 0, 0 }; \
\
switch (0) \
{ \
---
We already initialize buf similarly e.g.
429 unsigned char buf[2] = { 0, 0 }; \
At -Os the compiler is unable to determine if tmpbuf can or can't be used
in one of the failure cases e.g. return __UNKNOWN_10646_CHAR;.
This particular case we are into the 3rd conversion attempt of an unknown
character, so it can't possibly be a performance case to zero tmpbuf and
simplify the analysis for all kinds of static analysis tooling.
Thoughts?
--
Cheers,
Carlos.