From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by sourceware.org (Postfix) with ESMTPS id 384533858428 for ; Mon, 3 Jan 2022 13:41:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 384533858428 Received: by mail-ed1-x52a.google.com with SMTP id bm14so136078994edb.5 for ; Mon, 03 Jan 2022 05:41:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=BT9R4JehdKo/EYLg5pfaZ0//SQZNCeEqb76E/AlTrrA=; b=QT6Txay4b0H0OuI7CS339KZUNM9oT0gzZw4KoCeIWQQc4lSf+5EroUtCtoq7fA0Ykb w/N1x0zeKDdVepXRjgjGV9ykf0vInbHdTqumOFODOxiCNjuYBOk+TTj6YJIGWnJriNI/ UlPtSp1gegEgJ3T2CIuOV7A5+JIBZ2qMhxNuXrQKRw4wTTyTl6qffHMOl9qeO1bxiUaW VCNVyuJXQwSnDGUjRGCxs1S9QNJ25etW8U/+N+0g/JD7vhwg32NYLya0KutUZnKp2QfY 1xvBXoaEuIvB1ehi57R9BDYt+mZMRjZ6L6vuZu1miwFnuVufNEDZF6kJnrglKlRH5Ckw oiEA== X-Gm-Message-State: AOAM533IbTvGn+ugO3cSfKSUS/Djk7zr85KTWHVI8YlOaVj+U91hH3Wm YMGjvQKLRLXWDgqWe8d81FUrqIp0zO1XYGLnU0g= X-Google-Smtp-Source: ABdhPJwupoJBTDdJH5nHdhgo4ZfT6mo8clFl/+RRPXKUGJV9OTU6fFU/Darl/ZpeZq0GVzI81V534YWnAvnGdVnkwcA= X-Received: by 2002:a17:907:97d5:: with SMTP id js21mr36297615ejc.445.1641217282186; Mon, 03 Jan 2022 05:41:22 -0800 (PST) MIME-Version: 1.0 References: <4493B84A-DB6D-468B-86BF-DA5383D8CFE4@gmail.com> <794111d4-a64c-17e5-a4fe-f96e5182ee1@ispras.ru> <9252c923-c86a-7c98-ec5f-97410fbca8ed@ispras.ru> In-Reply-To: <9252c923-c86a-7c98-ec5f-97410fbca8ed@ispras.ru> From: Richard Biener Date: Mon, 3 Jan 2022 14:41:11 +0100 Message-ID: Subject: Re: [RFC PATCH] tree-ssa-sink: do not sink to in front of setjmp To: =?UTF-8?B?0JDQu9C10LrRgdC10Lkg0J3Rg9GA0LzRg9GF0LDQvNC10YLQvtCy?= Cc: Alexander Monakov , GCC Patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jan 2022 13:41:24 -0000 On Tue, Dec 14, 2021 at 12:10 PM =D0=90=D0=BB=D0=B5=D0=BA=D1=81=D0=B5=D0=B9= =D0=9D=D1=83=D1=80=D0=BC=D1=83=D1=85=D0=B0=D0=BC=D0=B5=D1=82=D0=BE=D0=B2 wrote: > > On 13.12.2021 18:20, Alexander Monakov wrote: > > On Mon, 13 Dec 2021, Richard Biener wrote: > > > >> On December 13, 2021 3:25:47 PM GMT+01:00, Alexander Monakov wrote: > >>> Greetings! > >>> > >>> While testing our patch that reimplements -Wclobbered on GIMPLE we fo= und > >>> a case where tree-ssa-sink moves a statement to a basic block in fron= t > >>> of a setjmp call. > >>> > >>> I am confident that this is unintended and should be considered inval= id > >>> GIMPLE. > >> Does CFG validation not catch this? That is, doesn't setjmp force the = start of > >> a new BB? > > Oh, good point. There's stmt_start_bb_p which returns true for setjmp, = but > > gimple_verify_flow_info doesn't check it. I guess we can try adding tha= t > > and collect the fallout on bootstrap/regtest. > > Bootstrap looks good, but testsuite has some regression (the applied > patch is below). > > The overall number of unexpected failures and unresolved testcases is > around 100. The diff is in attachment. > >> I think sinking relies on dominance and post dominance here but post d= ominance > >> may be too fragile with the abnormal cycles which are likely not backw= ards > >> reachable from exit. > >> > >> That said, checking for abnormal preds is OK, I just want to make sure= we > >> detect the invalid CFG - do we? > > As above, no, otherwise it would have been caught much earlier than ICE= 'ing > > our -Wclobbered patch :) > > > > Thank you. > > Alexander > > The patch for CFG validation: > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > index ebbd894ae03..92b08d1d6d8 100644 > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -5663,6 +5663,7 @@ gimple_verify_flow_info (void) > } > > /* Verify that body of basic block BB is free of control flow. *= / > + gimple *prev_stmt =3D NULL; > for (; !gsi_end_p (gsi); gsi_next (&gsi)) > { > gimple *stmt =3D gsi_stmt (gsi); > @@ -5674,6 +5675,14 @@ gimple_verify_flow_info (void) > err =3D 1; > } > > + if (prev_stmt && stmt_starts_bb_p (stmt, prev_stmt)) stmt_starts_bb_p is really a helper used during CFG build, I'd rather test explicitely for a GIMPLE call with ECF_RETURNS_TWICE, or maybe, verify that iff a block has abnormal predecessors it starts with such a call (because IIRC we in some cases elide abnormal edges and then it's OK to move "down" the calls). So yes, if a block has abnormal preds it should start with a ECF_RETURNS_TWICE call, I think we cannot verify the reverse reliably - abnormal edges cannot easily be re-built in late stages (it's a bug that we do so during RTL expansion). > + { > + error ("setjmp in the middle of basic block %d", bb->index)= ; > + err =3D 1; > + } > + if (!is_gimple_debug (stmt)) > + prev_stmt =3D stmt; > + > if (stmt_ends_bb_p (stmt)) > found_ctrl_stmt =3D true; >