From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 98227 invoked by alias); 2 Mar 2017 11:14:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 97937 invoked by uid 89); 2 Mar 2017 11:14:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=sa X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Mar 2017 11:14:32 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 216BEAB5D; Thu, 2 Mar 2017 11:14:31 +0000 (UTC) Date: Thu, 02 Mar 2017 11:14:00 -0000 From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR79345, better uninit warnings for memory In-Reply-To: <20170302103640.GM1849@tucnak> Message-ID: References: <20170302103640.GM1849@tucnak> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-03/txt/msg00100.txt.bz2 On Thu, 2 Mar 2017, Jakub Jelinek wrote: > On Wed, Mar 01, 2017 at 03:03:29PM +0100, Richard Biener wrote: > > One issue with the patch is duplicate warnings as TREE_NO_WARNING > > doesn't work very well on tcc_reference trees which are not > > shared. A followup could use some sort of hash table to mitigate > > Can't we use TREE_NO_WARNING on get_base_address instead? That is > shared... At least for VAR_DECL based refs, yes (the only ones we currently analyze). For MEM_REF bases it won't work. It would of course only warn once per aggregate and for a maybe-uninit warning it might be the false positive (the patch only sets TREE_NO_WARNING on the always-executed case). For the user maybe we should change the order we walk BBs to RPO and thus warn at the first maybe-uninit point so we can say "further uninit warnings suppressed for X"? > > @@ -2925,14 +2927,22 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree > > if (!*visited) > > *visited = BITMAP_ALLOC (NULL); > > for (i = 0; i < gimple_phi_num_args (def_stmt); ++i) > > - cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i), > > - walker, data, visited, 0, > > - function_entry_reached); > > + { > > + int res = walk_aliased_vdefs_1 (ref, > > + gimple_phi_arg_def (def_stmt, i), > > + walker, data, visited, 0, > > + function_entry_reached, limit); > > + if (res == -1) > > + return -1; > > + cnt += res; > > + } > > This doesn't look right. The recursive call starts with cnt of 0 again, > so if say you have limit 20 and cnt 10 when you are about to recurse and > that walk does 15 oracle comparisons, it won't return -1, but 15 and > you cnt += 15 to get 25 and never reach the limit, so then you walk perhaps > another 100000 times. > Can't you just pass cnt instead of 0 and then use > if (res == -1) > return -1; > cnt = res; > ? Or you'd need to play gaves with decrementing the limit for the recursive > call. Ah, indeed. Will fix (passing cnt). > > + /* For limiting the alias walk below we count all > > + vdefs in the function. We then give the alias walk an > > + overall budget (and simply not warn for further stmts). > > + ??? The interface doesn't give us the chance to assign > > + a maximum cost to each walk (and abort the walk if hit). */ > > I don't understand the last two lines, I thought you've added the ability > to the interface above and you actually use it. Will adjust the comment. > Otherwise it LGTM, yes, we will end up with new warnings, but unless the > alias oracle is wrong, there shouldn't be too many false positives, right? Yes, we are not warning for the case where there is at least one path with an initialization, like void foo(int b) { S s; if (b) s.a = 1; return s.a; } will not warn (trivial to add but not necessary for the regression fix). We also do not warn for pointer-based accesses (so unfortunately no fix for PR2972). Also trivial to add, but see PR79814 for more bootstrap fallout. Thanks, Richard.