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.133.124]) by sourceware.org (Postfix) with ESMTPS id 8F94138582AB for ; Tue, 21 Jun 2022 11:17:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8F94138582AB 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-646-7qP63oEOP3KiJjQ7QU1C4Q-1; Tue, 21 Jun 2022 07:17:43 -0400 X-MC-Unique: 7qP63oEOP3KiJjQ7QU1C4Q-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 B0C3D85A584; Tue, 21 Jun 2022 11:17:42 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.14]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6A21240C141F; Tue, 21 Jun 2022 11:17:42 +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 25LBHdrh695790 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 21 Jun 2022 13:17:40 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 25LBHdXN695788; Tue, 21 Jun 2022 13:17:39 +0200 Date: Tue, 21 Jun 2022 13:17:38 +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> <98e07b35-6472-aec1-abbb-a4cf49d66a9c@redhat.com> MIME-Version: 1.0 In-Reply-To: <98e07b35-6472-aec1-abbb-a4cf49d66a9c@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=-10.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, 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: Tue, 21 Jun 2022 11:17:47 -0000 On Mon, Jun 20, 2022 at 04:30:51PM -0400, Jason Merrill wrote: I'd still prefer to see a separate -funreachable-traps. The thing is that -fsanitize{,-recover,-trap}= are global options, not per function (and only tweaked by no_sanitize attribute), while something that needs to depend on the per-function -O0/-Og setting is necessarily per function. The *.awk changes I understand make -fsanitize= kind of per function but -fsanitize-{recover,trap}= remain global, that is going to be a nightmare especially with LTO which saves/restores the per function flags and for the global ones merges them across TUs. By separating sanitizers (which would remain global with no_sanitize overrides) from -funreachable-traps which would be Optimization option (with default set if unset in default_options_optimization or so) saved/restored upon function changes that issue is gone. > --- a/gcc/tree.h > +++ b/gcc/tree.h > @@ -5858,6 +5858,11 @@ builtin_decl_implicit (enum built_in_function fncode) > return builtin_info[uns_fncode].decl; > } > > +/* For BUILTIN_UNREACHABLE, use one of these instead of one of the above. */ > +extern tree builtin_decl_unreachable (); > +extern gcall *gimple_build_builtin_unreachable (location_t); > +extern tree build_builtin_unreachable (location_t); I think we generally try to declare functions in the header with same basename as the source file in which they are defined. So, the question is if builtin_decl_unreachable and build_builtin_unreachable shouldn't be defined in tree.cc and declared in tree.h and gimple_build_builtin_unreachable in gimple.cc and declared in gimple.h, using a helper defined in ubsan.cc and declared in ubsan.h (your current unreachable_1). > + > /* Set explicit builtin function nodes and whether it is an implicit > function. */ > > --- a/gcc/builtins.cc > +++ b/gcc/builtins.cc > --- a/gcc/cgraphunit.cc > +++ b/gcc/cgraphunit.cc > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > --- a/gcc/cp/cp-gimplify.cc > +++ b/gcc/cp/cp-gimplify.cc > --- a/gcc/gimple-fold.cc > +++ b/gcc/gimple-fold.cc > --- a/gcc/ipa-fnsummary.cc > +++ b/gcc/ipa-fnsummary.cc > --- a/gcc/ipa-prop.cc > +++ b/gcc/ipa-prop.cc > --- a/gcc/ipa.cc > +++ b/gcc/ipa.cc The above changes LGTM. > if (dump_enabled_p ()) > { > diff --git a/gcc/opts.cc b/gcc/opts.cc > index 959d48d173f..d92699a1bc9 100644 > --- a/gcc/opts.cc > +++ b/gcc/opts.cc > @@ -1122,6 +1122,17 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, > opts->x_flag_no_inline = 1; > } > > + /* At -O0 or -Og, turn __builtin_unreachable into a trap. */ > + if (!opts_set->x_flag_sanitize) > + { > + if (!opts->x_optimize || opts->x_optimize_debug) > + opts->x_flag_sanitize = SANITIZE_UNREACHABLE|SANITIZE_RETURN; > + > + /* Change this without regard to optimization level so we don't need to > + deal with it in optc-save-gen.awk. */ > + opts->x_flag_sanitize_trap = SANITIZE_UNREACHABLE|SANITIZE_RETURN; > + } > + > /* Pipelining of outer loops is only possible when general pipelining > capabilities are requested. */ > if (!opts->x_flag_sel_sched_pipelining) See above. > --- a/gcc/sanopt.cc > +++ b/gcc/sanopt.cc > @@ -942,7 +942,15 @@ public: > {} > > /* opt_pass methods: */ > - virtual bool gate (function *) { return flag_sanitize; } > + virtual bool gate (function *) > + { > + /* SANITIZE_RETURN is handled in the front-end. When trapping, > + SANITIZE_UNREACHABLE is handled by builtin_decl_unreachable. */ > + unsigned int mask = SANITIZE_RETURN; There are other sanitizers purely handled in the FEs, guess as a follow-up we should look at which of them don't really need any sanopt handling. > + if (flag_sanitize_trap & SANITIZE_UNREACHABLE) > + mask |= SANITIZE_UNREACHABLE; > + return flag_sanitize & ~mask; > + } > --- a/gcc/tree-cfg.cc > +++ b/gcc/tree-cfg.cc > --- a/gcc/tree-ssa-loop-ivcanon.cc > +++ b/gcc/tree-ssa-loop-ivcanon.cc > --- a/gcc/tree-ssa-sccvn.cc > +++ b/gcc/tree-ssa-sccvn.cc > --- a/gcc/tree.cc > +++ b/gcc/tree.cc LGTM. > --- a/gcc/ubsan.cc > +++ b/gcc/ubsan.cc > @@ -638,27 +638,84 @@ ubsan_create_data (const char *name, int loccnt, const location_t *ploc, ...) > return var; > } > > -/* Instrument the __builtin_unreachable call. We just call the libubsan > - routine instead. */ > +/* The built-in decl to use to mark code points believed to be unreachable. > + Typically __builtin_unreachable, but __builtin_trap if > + -fsanitize=unreachable -fsanitize-trap=unreachable. If only > + -fsanitize=unreachable, we rely on sanopt to replace any calls with the > + appropriate ubsan function. When building a call directly, use > + {gimple_},build_builtin_unreachable instead. */ > + > +tree > +builtin_decl_unreachable () > +{ > + enum built_in_function fncode = BUILT_IN_UNREACHABLE; > + > + if (sanitize_flags_p (SANITIZE_UNREACHABLE)) > + { > + if (flag_sanitize_trap & SANITIZE_UNREACHABLE) > + fncode = BUILT_IN_TRAP; > + /* Otherwise we want __builtin_unreachable () later folded into > + __ubsan_handle_builtin_unreachable with extra args. */ > + } I'd add the flag_unreachable_traps stuff here as else > +/* Shared between *build_builtin_unreachable. */ > + > +static void > +unreachable_1 (tree &fn, tree &data, location_t loc) Besides the potential moving, I think the coding guidelines don't recommend using references that way. But even if it is used, wouldn't it be better to return fn instead of void and just set data (either using reference or taking address of data)? Jakub