From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30285 invoked by alias); 22 Jun 2015 11:33:30 -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 30275 invoked by uid 89); 22 Jun 2015 11:33:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: fencepost.gnu.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (208.118.235.10) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 22 Jun 2015 11:33:28 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59955) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1Z6zyf-0005Xj-Rk for gcc-patches@gnu.org; Mon, 22 Jun 2015 07:33:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z6zya-00087o-FQ for gcc-patches@gnu.org; Mon, 22 Jun 2015 07:33:25 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:35860) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z6zya-00087T-97 for gcc-patches@gnu.org; Mon, 22 Jun 2015 07:33:20 -0400 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-02.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1Z6zyY-0004Dw-MD from Tom_deVries@mentor.com ; Mon, 22 Jun 2015 04:33:19 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-02.mgc.mentorg.com (137.202.0.106) with Microsoft SMTP Server id 14.3.224.2; Mon, 22 Jun 2015 12:33:17 +0100 Message-ID: <5587F277.1080401@mentor.com> Date: Mon, 22 Jun 2015 11:47:00 -0000 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Richard Biener CC: "gcc-patches@gnu.org" Subject: Re: [PATCH] Check dominator info in compute_dominance_frontiers References: <5587C18A.9050304@mentor.com> In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: Windows NT kernel [generic] [fuzzy] X-Received-From: 192.94.38.131 X-SW-Source: 2015-06/txt/msg01417.txt.bz2 On 22/06/15 12:14, Richard Biener wrote: > On Mon, Jun 22, 2015 at 10:04 AM, Tom de Vries wrote: >> Hi, >> >> during development of a patch I ran into a case where >> compute_dominance_frontiers was called with incorrect dominance info. >> >> The result was a segmentation violation somewhere in the bitmap code while >> executing this bitmap_set_bit in compute_dominance_frontiers_1: >> ... >> if (!bitmap_set_bit (&frontiers[runner->index], >> b->index)) >> break; >> ... >> >> The segmentation violation happens because runner->index is 0, and >> frontiers[0] is uninitialized. >> >> [ The initialization in update_ssa looks like this: >> ... >> dfs = XNEWVEC (bitmap_head, last_basic_block_for_fn (cfun)); >> FOR_EACH_BB_FN (bb, cfun) >> bitmap_initialize (&dfs[bb->index], &bitmap_default_obstack); >> compute_dominance_frontiers (dfs); >> ... >> >> FOR_EACH_BB_FN skips over the entry-block and the exit-block, so dfs[0] >> (frontiers[0] in compute_dominance_frontiers_1) is not initialized. >> >> We could add initialization by making the entry/exit-block bitmap_heads >> empty and setting the obstack to a reserved obstack bitmap_no_obstack for >> which allocation results in an assert. ] >> >> AFAIU, the immediate problem is not that frontiers[0] is uninitialized, but >> that the loop reaches the state of runner->index == 0, due to the incorrect >> dominance info. >> >> The patch adds an assert to the loop in compute_dominance_frontiers_1, to >> make the failure mode cleaner and easier to understand. >> >> I think we wouldn't catch all errors in dominance info with this assert. So >> the patch also contains an ENABLE_CHECKING-enabled verify_dominators call at >> the start of compute_dominance_frontiers. I'm not sure if: >> - adding the verify_dominators call is too costly in runtime. >> - the verify_dominators call should be inside or outside the >> TV_DOM_FRONTIERS measurement. >> - there is a level of ENABLE_CHECKING that is more appropriate for the >> verify_dominators call. >> >> Is this ok for trunk if bootstrap and reg-test on x86_64 succeeds? > > I don't think these kind of asserts are good. A segfault is good by itself > (so you can just add the comment if you like). > The segfault is not guaranteed to trigger, because it works on uninitialized data. Instead, we may end up modifying valid memory and silently generating wrong code or causing sigsegvs (which will be difficult to track back this error). So I don't think doing nothing is an option here. If we're not going to add this assert, we should initialize the uninitialized data in such a way that we are guaranteed to detect the error. The scheme I proposed above would take care of that. Should I implement that instead? > Likewise the verify_dominators call is too expensive and misplaced. > > If then the call belongs in the dom_computed[] == DOM_OK early-out > in calculate_dominance_info OK, like this: ... diff --git a/gcc/dominance.c b/gcc/dominance.c index a9e042e..1827eda9 100644 --- a/gcc/dominance.c +++ b/gcc/dominance.c @@ -646,7 +646,12 @@ calculate_dominance_info (enum cdi_direction dir) bool reverse = (dir == CDI_POST_DOMINATORS) ? true : false; if (dom_computed[dir_index] == DOM_OK) - return; + { +#if ENABLE_CHECKING + verify_dominators (CDI_DOMINATORS); +#endif + return; + } timevar_push (TV_DOMINANCE); if (!dom_info_available_p (dir)) ... I didn't fully understand your comment, do you want me to test this? Thanks, - Tom > (eventually also for the case where we > end up only computing the fast-query stuff).