Message ID | YuEErqyUK9EFfn5Z@tucnak |
---|---|
State | New, archived |
Headers |
Return-Path: <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6a10:b5d6:b0:2b9:3548:2db5 with SMTP id v22csp279803pxt; Wed, 27 Jul 2022 02:27:42 -0700 (PDT) X-Google-Smtp-Source: AGRyM1vd6EBilveM5X9GTcq3q4d/HY+FgLU8eo+EJeKeZ012J+WwdESwANOzW9nSYdKnDZY//VVx X-Received: by 2002:a17:907:b590:b0:72f:90ba:bef0 with SMTP id qx16-20020a170907b59000b0072f90babef0mr16947354ejc.333.1658914062162; Wed, 27 Jul 2022 02:27:42 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1658914062; cv=none; d=google.com; s=arc-20160816; b=mSTdAdocdBAu8nYoNINA1LhyijJF7qSG3iqDrkBxE5elPLulG45ZL0Cmpk1Z8F8Fm7 NNQcf29kBX8ahrXryV+lJyj4x4hQl7zSqQfYie3Xv7kBQTHgH464IitlhHtFqDex6zkD mHc24XCeMOKUfrS8ezWmXB8t0XqIKFEMO7k08ul1zHdvr31ii7Hs2beE6rGgcBN7gr4i d6ecOiPqnE0HnIevRuumbIJ3Y/GYNED1vGwDh7e8CvWzSxaWTvgU/+q0It8X0LLxSjO0 MriuptVf4RfZ3k1rb5la/Hk2r7q96A6PyuE1HIaagO47OvTl+WHCr+Dm0BtNIef+ymfI Zjxw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:reply-to:from:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence :content-disposition:mime-version:message-id:subject:to:date :dmarc-filter:delivered-to:dkim-signature:dkim-filter; bh=WyedpWSvdvyDTCK3Wcg0lpT6Himmq3VqkON3ugpefxQ=; b=K0eCqMHAyKf5JmXEuGYlycFgUixxROlCLS9IUTi7WEvVWJWBj9NYzk2Rzfhajn3kUv rcrCpaZbpa0KDoZZStTJ1D/0WOtSpWev2qalDk4sw5p5C892NLUZfUYAZshXI6nyF9Qz 6wNZMtO+8DUfmC9RC6I/EL+OL0Z2PTqgGCQE+cBGph2AwqqKBNDCP4ISYc+h5H+YDF3U 7xmD8n6LTclukPSUdRLLOC4k1garmLvPChrIdcI5VtBuiLK88pOFFcNKr0EOWLmNdwOU Iw1i5QxiEMeWlXOeU+G8g4cEeRZPKobLR0l0fvrLgLBiCiiKGRqKofqAN5Y8vVbIN31u yi5w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=lnsoyGp2; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id f22-20020a0564021e9600b0043c32b20a26si6708382edf.38.2022.07.27.02.27.41 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Jul 2022 02:27:42 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=lnsoyGp2; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7D60B3857020 for <ouuuleilei@gmail.com>; Wed, 27 Jul 2022 09:26:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7D60B3857020 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1658914016; bh=WyedpWSvdvyDTCK3Wcg0lpT6Himmq3VqkON3ugpefxQ=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=lnsoyGp2VHJFcBRaj1saclT08hiBOVlkiI2xb3TErBnuxVrLBg03VBS10Pk2OWM/j DeeSyQcr2VQ4Xc9QgPNjmem/FE+++9Z75qXgcaYU6FmM24K471h0gGPnD9a9Dbn0Y2 2LfNKGEfpy6JYCcAXK5872S4fRxV55FYzQs5/zcQ= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id D58E8385AE40 for <gcc-patches@gcc.gnu.org>; Wed, 27 Jul 2022 09:26:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D58E8385AE40 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-554-z_yfwuGdPJKMyUf9nA_wzQ-1; Wed, 27 Jul 2022 05:26:10 -0400 X-MC-Unique: z_yfwuGdPJKMyUf9nA_wzQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0978B185A7A4; Wed, 27 Jul 2022 09:26:10 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BEB5B18ECC; Wed, 27 Jul 2022 09:26:09 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 26R9Q7PM2208198 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 27 Jul 2022 11:26:07 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 26R9Q6sV2208197; Wed, 27 Jul 2022 11:26:06 +0200 Date: Wed, 27 Jul 2022 11:26:06 +0200 To: Richard Biener <rguenther@suse.de> Subject: [PATCH] gimple, internal-fn: Add IFN_TRAP and use it for __builtin_unreachable [PR106099] Message-ID: <YuEErqyUK9EFfn5Z@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Jakub Jelinek <jakub@redhat.com> Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1739497471404784322?= X-GMAIL-MSGID: =?utf-8?q?1739497471404784322?= |
Series |
gimple, internal-fn: Add IFN_TRAP and use it for __builtin_unreachable [PR106099]
|
|
Commit Message
Jakub Jelinek
July 27, 2022, 9:26 a.m. UTC
Hi! __builtin_unreachable and __ubsan_handle_builtin_unreachable don't use vops, they are marked const/leaf/noreturn/nothrow/cold. But __builtin_trap uses vops, isn't const, just leaf/noreturn/nothrow/cold. This is I believe so that when users explicitly use __builtin_trap in their sources they get stores visible at the trap side. -fsanitize=unreachable -fsanitize-undefined-trap-on-error used to transform __builtin_unreachable to __builtin_trap even in the past, but the sanopt pass has TODO_update_ssa, so it worked fine. Now that gimple_build_builtin_unreachable can build a __builtin_trap call right away, we can run into problems that whenever we need it we would need to either manually or through TODO_update* ensure the vops being updated. Though, as it is originally __builtin_unreachable which is just implemented as trap, I think for this case it is fine to avoid vops. For this the patch introduces IFN_TRAP, which has ECF_* flags like __builtin_unreachable and is expanded as __builtin_trap. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2022-07-27 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/106099 * internal-fn.def (TRAP): New internal fn. * internal-fn.h (expand_TRAP): Declare. * internal-fn.cc (expand_TRAP): Define. * gimple.cc (gimple_build_builtin_unreachable): For BUILT_IN_TRAP, use internal fn rather than builtin. * gcc.dg/ubsan/pr106099.c: New test. Jakub
Comments
On Wed, 27 Jul 2022, Jakub Jelinek wrote: > Hi! > > __builtin_unreachable and __ubsan_handle_builtin_unreachable don't > use vops, they are marked const/leaf/noreturn/nothrow/cold. > But __builtin_trap uses vops, isn't const, just leaf/noreturn/nothrow/cold. > This is I believe so that when users explicitly use __builtin_trap in their > sources they get stores visible at the trap side. > -fsanitize=unreachable -fsanitize-undefined-trap-on-error used to transform > __builtin_unreachable to __builtin_trap even in the past, but the sanopt pass > has TODO_update_ssa, so it worked fine. > > Now that gimple_build_builtin_unreachable can build a __builtin_trap call > right away, we can run into problems that whenever we need it we would need > to either manually or through TODO_update* ensure the vops being updated. > > Though, as it is originally __builtin_unreachable which is just implemented > as trap, I think for this case it is fine to avoid vops. For this the > patch introduces IFN_TRAP, which has ECF_* flags like __builtin_unreachable > and is expanded as __builtin_trap. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? I think for the sake of sanitizing unreachable as trap this is OK but it seems this isn't actually what is done. I still think the original change fiddling with the unreachable decl is wrong. Likewise unrolling shouldn't use gimple_build_builtin_unreachable - it isn't sanitizing anything but telling the middle-end to DCE a path. IMHO only few select places where the middle-end builds unreachable () should be using this function (which means it probably shouldn't exist), like path isolation which IIRC uses a trap anyway. > 2022-07-27 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/106099 > * internal-fn.def (TRAP): New internal fn. > * internal-fn.h (expand_TRAP): Declare. > * internal-fn.cc (expand_TRAP): Define. > * gimple.cc (gimple_build_builtin_unreachable): For BUILT_IN_TRAP, > use internal fn rather than builtin. > > * gcc.dg/ubsan/pr106099.c: New test. > > --- gcc/internal-fn.def.jj 2022-07-26 10:32:23.886269144 +0200 > +++ gcc/internal-fn.def 2022-07-26 11:40:41.799927048 +0200 > @@ -456,6 +456,10 @@ DEF_INTERNAL_FN (SHUFFLEVECTOR, ECF_CONS > /* <=> optimization. */ > DEF_INTERNAL_FN (SPACESHIP, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > > +/* __builtin_trap created from/for __builtin_unreachable. */ > +DEF_INTERNAL_FN (TRAP, ECF_CONST | ECF_LEAF | ECF_NORETURN > + | ECF_NOTHROW | ECF_COLD, NULL) > + > #undef DEF_INTERNAL_INT_FN > #undef DEF_INTERNAL_FLT_FN > #undef DEF_INTERNAL_FLT_FLOATN_FN > --- gcc/internal-fn.h.jj 2022-06-16 10:56:28.945385251 +0200 > +++ gcc/internal-fn.h 2022-07-26 11:45:50.483837472 +0200 > @@ -242,6 +242,7 @@ extern void expand_internal_call (intern > extern void expand_PHI (internal_fn, gcall *); > extern void expand_SHUFFLEVECTOR (internal_fn, gcall *); > extern void expand_SPACESHIP (internal_fn, gcall *); > +extern void expand_TRAP (internal_fn, gcall *); > > extern bool vectorized_internal_fn_supported_p (internal_fn, tree); > > --- gcc/internal-fn.cc.jj 2022-07-26 10:32:23.885269157 +0200 > +++ gcc/internal-fn.cc 2022-07-26 11:42:02.611856420 +0200 > @@ -4494,3 +4494,9 @@ expand_SPACESHIP (internal_fn, gcall *st > if (!rtx_equal_p (target, ops[0].value)) > emit_move_insn (target, ops[0].value); > } > + > +void > +expand_TRAP (internal_fn, gcall *) > +{ > + expand_builtin_trap (); > +} > --- gcc/gimple.cc.jj 2022-06-27 11:18:02.680058429 +0200 > +++ gcc/gimple.cc 2022-07-26 11:57:17.049760135 +0200 > @@ -430,7 +430,16 @@ gimple_build_builtin_unreachable (locati > { > tree data = NULL_TREE; > tree fn = sanitize_unreachable_fn (&data, loc); > - gcall *g = gimple_build_call (fn, data != NULL_TREE, data); > + gcall *g; > + if (DECL_FUNCTION_CODE (fn) != BUILT_IN_TRAP) > + g = gimple_build_call (fn, data != NULL_TREE, data); > + else > + { > + /* Instead of __builtin_trap use .TRAP, so that it doesn't > + need vops. */ > + gcc_checking_assert (data == NULL_TREE); > + g = gimple_build_call_internal (IFN_TRAP, 0); > + } > gimple_set_location (g, loc); > return g; > } > --- gcc/testsuite/gcc.dg/ubsan/pr106099.c.jj 2022-07-26 12:22:26.248156163 +0200 > +++ gcc/testsuite/gcc.dg/ubsan/pr106099.c 2022-07-26 11:34:25.660909186 +0200 > @@ -0,0 +1,10 @@ > +/* PR tree-optimization/106099 */ > +/* { dg-do compile } */ > +/* { dg-options "-O -fsanitize=unreachable -fsanitize-undefined-trap-on-error -fno-tree-ccp -fno-tree-dominator-opts" } */ > + > +void > +foo (void) > +{ > + for (unsigned i = 0; i == 0; i++) > + ; > +} > > Jakub > >
On Wed, Jul 27, 2022 at 09:33:47AM +0000, Richard Biener wrote: > > __builtin_unreachable and __ubsan_handle_builtin_unreachable don't > > use vops, they are marked const/leaf/noreturn/nothrow/cold. > > But __builtin_trap uses vops, isn't const, just leaf/noreturn/nothrow/cold. > > This is I believe so that when users explicitly use __builtin_trap in their > > sources they get stores visible at the trap side. > > -fsanitize=unreachable -fsanitize-undefined-trap-on-error used to transform > > __builtin_unreachable to __builtin_trap even in the past, but the sanopt pass > > has TODO_update_ssa, so it worked fine. > > > > Now that gimple_build_builtin_unreachable can build a __builtin_trap call > > right away, we can run into problems that whenever we need it we would need > > to either manually or through TODO_update* ensure the vops being updated. > > > > Though, as it is originally __builtin_unreachable which is just implemented > > as trap, I think for this case it is fine to avoid vops. For this the > > patch introduces IFN_TRAP, which has ECF_* flags like __builtin_unreachable > > and is expanded as __builtin_trap. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > I think for the sake of sanitizing unreachable as trap this is OK > but it seems this isn't actually what is done. We chose to sanitize not just explicit user __builtin_unreachable calls, but also the internally generated __builtin_unreachable calls (with the one exception of fall through to end of C++ function returning non-void, which had before a separate sanitizer) and we've been doing it since 2013 when ubsan was added. Even for the internally generated unreachable calls like those from devirtualization or other reasons like ivcanon/unrolling, having the possibility to get some runtime diagnostics or trap can be useful over just falling through to random following code. Previously we'd always emit __builtin_unreachable, then perhaps in some cases could e.g. optimize it away (say if there is a guarding condition around the implicitly added unreachable turning the condition into VRP info and optimizing the conditional away), otherwise the sanopt pass would turn those __builtin_unreachable calls into __builtin_trap. With the recent changes, we don't run the sanopt pass when only doing -fsanitize=unreachable (or -funrechable-traps) though, so we need to emit the trap/__ubsan_handle_unreachable/__builtin_unreachable right away. Jakub
On Wed, 27 Jul 2022, Jakub Jelinek wrote: > On Wed, Jul 27, 2022 at 09:33:47AM +0000, Richard Biener wrote: > > > __builtin_unreachable and __ubsan_handle_builtin_unreachable don't > > > use vops, they are marked const/leaf/noreturn/nothrow/cold. > > > But __builtin_trap uses vops, isn't const, just leaf/noreturn/nothrow/cold. > > > This is I believe so that when users explicitly use __builtin_trap in their > > > sources they get stores visible at the trap side. > > > -fsanitize=unreachable -fsanitize-undefined-trap-on-error used to transform > > > __builtin_unreachable to __builtin_trap even in the past, but the sanopt pass > > > has TODO_update_ssa, so it worked fine. > > > > > > Now that gimple_build_builtin_unreachable can build a __builtin_trap call > > > right away, we can run into problems that whenever we need it we would need > > > to either manually or through TODO_update* ensure the vops being updated. > > > > > > Though, as it is originally __builtin_unreachable which is just implemented > > > as trap, I think for this case it is fine to avoid vops. For this the > > > patch introduces IFN_TRAP, which has ECF_* flags like __builtin_unreachable > > > and is expanded as __builtin_trap. > > > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > I think for the sake of sanitizing unreachable as trap this is OK > > but it seems this isn't actually what is done. > > We chose to sanitize not just explicit user __builtin_unreachable calls, > but also the internally generated __builtin_unreachable calls (with the > one exception of fall through to end of C++ function returning non-void, > which had before a separate sanitizer) and we've been doing it since 2013 > when ubsan was added. > Even for the internally generated unreachable calls like those from > devirtualization or other reasons like ivcanon/unrolling, having the > possibility to get some runtime diagnostics or trap can be useful over > just falling through to random following code. So at least for the unrolling use the intent is to have the unreachable () fully elided by later passes. Honza can correct me if I'm wrong. Using __builtin_trap from the start until sanopt may prevent some of that from happening, keeping dead conditions live, no? > Previously we'd always emit __builtin_unreachable, then perhaps in some > cases could e.g. optimize it away (say if there is a guarding condition > around the implicitly added unreachable turning the condition into VRP > info and optimizing the conditional away), otherwise the sanopt pass > would turn those __builtin_unreachable calls into __builtin_trap. > With the recent changes, we don't run the sanopt pass when only > doing -fsanitize=unreachable (or -funrechable-traps) though, so we need > to emit the trap/__ubsan_handle_unreachable/__builtin_unreachable right > away. Why did the recent changes not just replace __builtin_unreachable at RTL expansion time? Was the intent really to force the paths to be kept live? I can see that for user or frontend generated unreachables but not so much for some of the middle-end ones. Richard.
On Wed, Jul 27, 2022 at 10:09:34AM +0000, Richard Biener wrote: > > We chose to sanitize not just explicit user __builtin_unreachable calls, > > but also the internally generated __builtin_unreachable calls (with the > > one exception of fall through to end of C++ function returning non-void, > > which had before a separate sanitizer) and we've been doing it since 2013 > > when ubsan was added. > > Even for the internally generated unreachable calls like those from > > devirtualization or other reasons like ivcanon/unrolling, having the > > possibility to get some runtime diagnostics or trap can be useful over > > just falling through to random following code. > > So at least for the unrolling use the intent is to have the > unreachable () fully elided by later passes. Honza can correct me > if I'm wrong. Using __builtin_trap from the start until sanopt > may prevent some of that from happening, keeping dead conditions > live, no? That is true. I guess changing the sanopt gate back and building __builtin_unreachable only in the ivcanon/unrolling case is possible too. Without or with this patch then, the advantage of the patch would be that we wouldn't need to recompute vops if we replace unreachables with traps during the sanopt pass. > > > Previously we'd always emit __builtin_unreachable, then perhaps in some > > cases could e.g. optimize it away (say if there is a guarding condition > > around the implicitly added unreachable turning the condition into VRP > > info and optimizing the conditional away), otherwise the sanopt pass > > would turn those __builtin_unreachable calls into __builtin_trap. > > With the recent changes, we don't run the sanopt pass when only > > doing -fsanitize=unreachable (or -funrechable-traps) though, so we need > > to emit the trap/__ubsan_handle_unreachable/__builtin_unreachable right > > away. > > Why did the recent changes not just replace __builtin_unreachable > at RTL expansion time? Was the intent really to force the paths > to be kept live? I can see that for user or frontend generated > unreachables but not so much for some of the middle-end ones. It is easier on GIMPLE and perhaps allows e.g. sharing the data for __ubsan_handle_unreachable calls. sanopt pass is quite late anyway. Jakub
On Wed, 27 Jul 2022, Jakub Jelinek wrote: > On Wed, Jul 27, 2022 at 10:09:34AM +0000, Richard Biener wrote: > > > We chose to sanitize not just explicit user __builtin_unreachable calls, > > > but also the internally generated __builtin_unreachable calls (with the > > > one exception of fall through to end of C++ function returning non-void, > > > which had before a separate sanitizer) and we've been doing it since 2013 > > > when ubsan was added. > > > Even for the internally generated unreachable calls like those from > > > devirtualization or other reasons like ivcanon/unrolling, having the > > > possibility to get some runtime diagnostics or trap can be useful over > > > just falling through to random following code. > > > > So at least for the unrolling use the intent is to have the > > unreachable () fully elided by later passes. Honza can correct me > > if I'm wrong. Using __builtin_trap from the start until sanopt > > may prevent some of that from happening, keeping dead conditions > > live, no? > > That is true. > I guess changing the sanopt gate back and building __builtin_unreachable > only in the ivcanon/unrolling case is possible too. > > Without or with this patch then, the advantage of the patch would be that > we wouldn't need to recompute vops if we replace unreachables with traps > during the sanopt pass. Yes, as I said on that ground the patch is OK, but I think it doesn't really address the few PRs that popped up after the earlier change. Richard. > > > > > Previously we'd always emit __builtin_unreachable, then perhaps in some > > > cases could e.g. optimize it away (say if there is a guarding condition > > > around the implicitly added unreachable turning the condition into VRP > > > info and optimizing the conditional away), otherwise the sanopt pass > > > would turn those __builtin_unreachable calls into __builtin_trap. > > > With the recent changes, we don't run the sanopt pass when only > > > doing -fsanitize=unreachable (or -funrechable-traps) though, so we need > > > to emit the trap/__ubsan_handle_unreachable/__builtin_unreachable right > > > away. > > > > Why did the recent changes not just replace __builtin_unreachable > > at RTL expansion time? Was the intent really to force the paths > > to be kept live? I can see that for user or frontend generated > > unreachables but not so much for some of the middle-end ones. > > It is easier on GIMPLE and perhaps allows e.g. sharing the data for > __ubsan_handle_unreachable calls. sanopt pass is quite late anyway.
On 7/27/22 03:09, Richard Biener wrote: > On Wed, 27 Jul 2022, Jakub Jelinek wrote: > >> On Wed, Jul 27, 2022 at 09:33:47AM +0000, Richard Biener wrote: >>>> __builtin_unreachable and __ubsan_handle_builtin_unreachable don't >>>> use vops, they are marked const/leaf/noreturn/nothrow/cold. >>>> But __builtin_trap uses vops, isn't const, just leaf/noreturn/nothrow/cold. >>>> This is I believe so that when users explicitly use __builtin_trap in their >>>> sources they get stores visible at the trap side. >>>> -fsanitize=unreachable -fsanitize-undefined-trap-on-error used to transform >>>> __builtin_unreachable to __builtin_trap even in the past, but the sanopt pass >>>> has TODO_update_ssa, so it worked fine. >>>> >>>> Now that gimple_build_builtin_unreachable can build a __builtin_trap call >>>> right away, we can run into problems that whenever we need it we would need >>>> to either manually or through TODO_update* ensure the vops being updated. >>>> >>>> Though, as it is originally __builtin_unreachable which is just implemented >>>> as trap, I think for this case it is fine to avoid vops. For this the >>>> patch introduces IFN_TRAP, which has ECF_* flags like __builtin_unreachable >>>> and is expanded as __builtin_trap. >>>> >>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? >>> >>> I think for the sake of sanitizing unreachable as trap this is OK >>> but it seems this isn't actually what is done. >> >> We chose to sanitize not just explicit user __builtin_unreachable calls, >> but also the internally generated __builtin_unreachable calls (with the >> one exception of fall through to end of C++ function returning non-void, >> which had before a separate sanitizer) and we've been doing it since 2013 >> when ubsan was added. >> Even for the internally generated unreachable calls like those from >> devirtualization or other reasons like ivcanon/unrolling, having the >> possibility to get some runtime diagnostics or trap can be useful over >> just falling through to random following code. > > So at least for the unrolling use the intent is to have the > unreachable () fully elided by later passes. Honza can correct me > if I'm wrong. Using __builtin_trap from the start until sanopt > may prevent some of that from happening, keeping dead conditions > live, no? > >> Previously we'd always emit __builtin_unreachable, then perhaps in some >> cases could e.g. optimize it away (say if there is a guarding condition >> around the implicitly added unreachable turning the condition into VRP >> info and optimizing the conditional away), otherwise the sanopt pass >> would turn those __builtin_unreachable calls into __builtin_trap. >> With the recent changes, we don't run the sanopt pass when only >> doing -fsanitize=unreachable (or -funrechable-traps) though, so we need >> to emit the trap/__ubsan_handle_unreachable/__builtin_unreachable right >> away. > > Why did the recent changes not just replace __builtin_unreachable > at RTL expansion time? Was the intent really to force the paths > to be kept live? I can see that for user or frontend generated > unreachables but not so much for some of the middle-end ones. Yes, the intent was to force user and frontend generated unreachables to be kept live, particularly the one for flowing off the end of a non-void function. I have also wondered if treating middle-end unreachables the same was a mistake. Jason
--- gcc/internal-fn.def.jj 2022-07-26 10:32:23.886269144 +0200 +++ gcc/internal-fn.def 2022-07-26 11:40:41.799927048 +0200 @@ -456,6 +456,10 @@ DEF_INTERNAL_FN (SHUFFLEVECTOR, ECF_CONS /* <=> optimization. */ DEF_INTERNAL_FN (SPACESHIP, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) +/* __builtin_trap created from/for __builtin_unreachable. */ +DEF_INTERNAL_FN (TRAP, ECF_CONST | ECF_LEAF | ECF_NORETURN + | ECF_NOTHROW | ECF_COLD, NULL) + #undef DEF_INTERNAL_INT_FN #undef DEF_INTERNAL_FLT_FN #undef DEF_INTERNAL_FLT_FLOATN_FN --- gcc/internal-fn.h.jj 2022-06-16 10:56:28.945385251 +0200 +++ gcc/internal-fn.h 2022-07-26 11:45:50.483837472 +0200 @@ -242,6 +242,7 @@ extern void expand_internal_call (intern extern void expand_PHI (internal_fn, gcall *); extern void expand_SHUFFLEVECTOR (internal_fn, gcall *); extern void expand_SPACESHIP (internal_fn, gcall *); +extern void expand_TRAP (internal_fn, gcall *); extern bool vectorized_internal_fn_supported_p (internal_fn, tree); --- gcc/internal-fn.cc.jj 2022-07-26 10:32:23.885269157 +0200 +++ gcc/internal-fn.cc 2022-07-26 11:42:02.611856420 +0200 @@ -4494,3 +4494,9 @@ expand_SPACESHIP (internal_fn, gcall *st if (!rtx_equal_p (target, ops[0].value)) emit_move_insn (target, ops[0].value); } + +void +expand_TRAP (internal_fn, gcall *) +{ + expand_builtin_trap (); +} --- gcc/gimple.cc.jj 2022-06-27 11:18:02.680058429 +0200 +++ gcc/gimple.cc 2022-07-26 11:57:17.049760135 +0200 @@ -430,7 +430,16 @@ gimple_build_builtin_unreachable (locati { tree data = NULL_TREE; tree fn = sanitize_unreachable_fn (&data, loc); - gcall *g = gimple_build_call (fn, data != NULL_TREE, data); + gcall *g; + if (DECL_FUNCTION_CODE (fn) != BUILT_IN_TRAP) + g = gimple_build_call (fn, data != NULL_TREE, data); + else + { + /* Instead of __builtin_trap use .TRAP, so that it doesn't + need vops. */ + gcc_checking_assert (data == NULL_TREE); + g = gimple_build_call_internal (IFN_TRAP, 0); + } gimple_set_location (g, loc); return g; } --- gcc/testsuite/gcc.dg/ubsan/pr106099.c.jj 2022-07-26 12:22:26.248156163 +0200 +++ gcc/testsuite/gcc.dg/ubsan/pr106099.c 2022-07-26 11:34:25.660909186 +0200 @@ -0,0 +1,10 @@ +/* PR tree-optimization/106099 */ +/* { dg-do compile } */ +/* { dg-options "-O -fsanitize=unreachable -fsanitize-undefined-trap-on-error -fno-tree-ccp -fno-tree-dominator-opts" } */ + +void +foo (void) +{ + for (unsigned i = 0; i == 0; i++) + ; +}