From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 8BB08385E010 for ; Thu, 16 Jun 2022 13:14:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8BB08385E010 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-231-Oarz_i9ONv-cZaT4ulPxBA-1; Thu, 16 Jun 2022 09:14:18 -0400 X-MC-Unique: Oarz_i9ONv-cZaT4ulPxBA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 33843811E87; Thu, 16 Jun 2022 13:14:18 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.239]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E71E940C141F; Thu, 16 Jun 2022 13:14:17 +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 25GDEFpo1814143 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 16 Jun 2022 15:14:15 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 25GDEEmK1814142; Thu, 16 Jun 2022 15:14:14 +0200 Date: Thu, 16 Jun 2022 15:14:14 +0200 From: Jakub Jelinek To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, richard.guenther@gmail.com, jwakely.gcc@gmail.com Subject: Re: [PATCH RFA] ubsan: default to trap on unreachable at -O0 and -Og [PR104642] Message-ID: Reply-To: Jakub Jelinek References: <20220613195313.3240547-1-jason@redhat.com> <6c78210d-f2a8-3371-5d88-74bdab399706@redhat.com> MIME-Version: 1.0 In-Reply-To: <6c78210d-f2a8-3371-5d88-74bdab399706@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.11.54.2 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, T_SCC_BODY_TEXT_LINE 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: Thu, 16 Jun 2022 13:14:26 -0000 On Wed, Jun 15, 2022 at 04:38:49PM -0400, Jason Merrill wrote: > > I do not like doing it this way, -fsanitize-undefined-trap-on-error is > > (unfortunately for compatibility with llvm misdesign, users should have > > ways to control which of the enabled sanitizers should be handled which way, > > where the 3 ways are runtime diagnostics without abort, runtime diagnostics > > with abort and __builtin_trap ()) an all or nothing option which affects all > > the sanitizers. > > Makes sense. The first is -fsanitize-recover=, I think, the second is the > default, and perhaps the third could be -fsanitize-trap=? -f{,no-}sanitize-recover= is a bitmask, whether some sanitizer recovers or not. The default is that ubsan sanitizers except for unreachable and return default to recover on and similarly kasan and hwkasan, other sanitizers default to no recover. If we add -f{,no-}sanitize-trap= similar way, we'd need to figure out what it means if a both bits are set (i.e. we are asked to recover and trap at the same time). We could error, or silently prefer trap over recover, etc. And we'd need to define interaction with -fsanitize-undefined-trap-on-error, would that be effectively an alias for -fsanitize-trap=all (but if so, we'd need the silent trap takes priority over recover way)? > > Furthermore, handling it the UBSan way means we slow down the compiler > > (enable a bunch of extra passes, like sanopt, ubsan), which is undesirable > > e.g. for -O0 compilation speed. > > The ubsan pass is not enabled for unreachable|return. sanopt does a single You're right. > pass over the function to rewrite __builtin_unreachable, but that doesn't > seem like much overhead. But I think we are trying to avoid hard any kind of unnecessary whole IL extra walks, especially for -O0. > > So, I think -funreachable-traps should be a separate flag and not an alias, > > enabled by default for -O0 and -Og, which would be handled elsewhere > > (I'd say e.g. in fold_builtin_0 and perhaps gimple_fold_builtin too to > > avoid allocating trees unnecessary) > > I tried this approach, but it misses some __builtin_unreachable calls added > by e.g. execute_fixup_cfg; it seems they never get folded by any subsequent > pass. We could also expand BUILT_IN_UNREACHABLE as BUILT_IN_TRAP during expansion to catch whatever isn't caught by folding. > > and would be done if > > flag_unreachable_traps && !sanitize_flag_p (SANITIZE_UNREACHABLE), > > just replacing that __builtin_unreachable call with __builtin_trap. > > For the function ends in fact under those conditions we could emit > > __builtin_trap right away instead of emitting __builtin_unreachable > > and waiting on folding it later to __builtin_trap. > > Sure, but I generally prefer to change fewer places. I'd say this would be very small change and the fastest + most reliable. Simply replace all builtin_decl_implicit (BUILT_IN_UNREACHABLE) calls with builtin_decl_unreachable () (12 of them) and define tree builtin_decl_unreachable () { enum built_in_function fncode = BUILT_IN_UNREACHABLE; if (sanitize_flag_p (SANITIZE_UNREACHABLE)) { if (flag_sanitize_undefined_trap_on_error) fncode = BUILT_IN_TRAP; /* Otherwise we want __builtin_unreachable () later folded into __ubsan_handle_builtin_unreachable with extra args. */ } else if (flag_unreachable_traps) fncode = BUILT_IN_TRAP; return builtin_decl_implicit (fncode); } and that's it (well, also in build_common_builtin_nodes declare __builtin_trap for FEs that don't do that - like it is done for __builtin_unreachable). Jakub