From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40802 invoked by alias); 28 Oct 2016 13:07:50 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 40787 invoked by uid 89); 28 Oct 2016 13:07:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.3 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=indicator, hides, worried, assured X-HELO: mail-qk0-f182.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-transfer-encoding; bh=QadSu+cQ/GRLSMKMXglRWqbxqiGMIgYHJjWoq1P6fsM=; b=AFUazQQpUDm9oMUgHwb2HCa4J+16mk0C6oE4QYp83AbNO9xgbIRp0NxUL7gLnAFRZx 4R+z96rxQIE82g1n1nMSq10rRnH6dd6H/NxzceK9z6WhEeJZipoefUXqrmxG4J9x9hw6 ANCq5cOJOzVNWm9QDoayTudGcrsySknz3E7b5zDx6oQiiV9+NLbtPqKN9HLieogZBrq4 77c0tah8tpPdG3IuwapcB9hPsFi1Y+HZKMeI4dErewRRYGvbkmJcJcjjfWGyjv66lfKU +v5LGIV4E0M5+0RgLYQthymxXHqvdYYI65tXOSld/FwrsRghoKVQ7Hi+flf9JSxOEI+J itTw== X-Gm-Message-State: ABUngveeC9v1akxHPTvP5RCSbjWQZGuxQwRYxQferz6uWTM7WWysjnt5wcOePbaE1Pnulpib X-Received: by 10.55.145.129 with SMTP id t123mr10595561qkd.130.1477660067083; Fri, 28 Oct 2016 06:07:47 -0700 (PDT) Subject: Re: [PATCH] Fix -Os related -Werror failures. To: Florian Weimer , GNU C Library References: <6eac682f-26fa-6a47-9497-357206266ba1@redhat.com> <6be7dce5-bfa7-32c7-5bac-6c3b79776683@redhat.com> From: Carlos O'Donell Message-ID: <96d1926b-4b53-1453-88e3-9bf5e050439e@redhat.com> Date: Fri, 28 Oct 2016 13:07:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-10/txt/msg00530.txt.bz2 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.