Re: [PATCH] Fix -Os related -Werror failures.

[Carlos O'Donell <carlos@redhat.com>]

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.