From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 46465 invoked by alias); 22 Jun 2015 11:47:44 -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 46451 invoked by uid 89); 22 Jun 2015 11:47:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=BAYES_00,FREEMAIL_FROM,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:47:42 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35418) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1Z70CR-0006u8-Og for gcc-patches@gnu.org; Mon, 22 Jun 2015 07:47:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z70CQ-0006le-2Q for gcc-patches@gnu.org; Mon, 22 Jun 2015 07:47:39 -0400 Received: from mail-oi0-x236.google.com ([2607:f8b0:4003:c06::236]:36075) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z70CP-0006lY-TA for gcc-patches@gnu.org; Mon, 22 Jun 2015 07:47:38 -0400 Received: by oigb199 with SMTP id b199so77790816oig.3 for ; Mon, 22 Jun 2015 04:47:37 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.182.230.234 with SMTP id tb10mr24898840obc.23.1434973657251; Mon, 22 Jun 2015 04:47:37 -0700 (PDT) Received: by 10.76.115.167 with HTTP; Mon, 22 Jun 2015 04:47:37 -0700 (PDT) In-Reply-To: <5587F277.1080401@mentor.com> References: <5587C18A.9050304@mentor.com> <5587F277.1080401@mentor.com> Date: Mon, 22 Jun 2015 12:39:00 -0000 Message-ID: Subject: Re: [PATCH] Check dominator info in compute_dominance_frontiers From: Richard Biener To: Tom de Vries Cc: "gcc-patches@gnu.org" Content-Type: text/plain; charset=UTF-8 X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:4003:c06::236 X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg01418.txt.bz2 On Mon, Jun 22, 2015 at 1:33 PM, Tom de Vries wrote: > 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? No, instead the check below should catch the error much earlier. >> 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)) > ... Yes. > I didn't fully understand your comment, do you want me to test this? Sure, it should catch the error. Richard. > Thanks, > - Tom > > >> (eventually also for the case where we >> end up only computing the fast-query stuff). > >