From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id DB1DE385703C for ; Wed, 27 Jul 2022 10:09:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DB1DE385703C Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id B58DB2064C; Wed, 27 Jul 2022 10:09:34 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id AD3D22C141; Wed, 27 Jul 2022 10:09:34 +0000 (UTC) Date: Wed, 27 Jul 2022 10:09:34 +0000 (UTC) From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org, Jason Merrill , Jan Hubicka Subject: Re: [PATCH] gimple, internal-fn: Add IFN_TRAP and use it for __builtin_unreachable [PR106099] In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Jul 2022 10:09:37 -0000 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.