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 DEC463857427 for ; Wed, 15 Jun 2022 20:38:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DEC463857427 Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-255-erD_N0ClMvSz7cq7Sr6zZQ-1; Wed, 15 Jun 2022 16:38:51 -0400 X-MC-Unique: erD_N0ClMvSz7cq7Sr6zZQ-1 Received: by mail-qv1-f69.google.com with SMTP id kd24-20020a056214401800b0046d7fd4a421so9142755qvb.20 for ; Wed, 15 Jun 2022 13:38:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=8d8owarBLVDjJVRLv8Suq+l7dF5wjCHPEfhClZ+CWVo=; b=5wYn0i859V1wzVkNZ/Nwg1bnLUWTPVMrYSP4FLfFOO7yA6phi+35Gtc+HmKnA+UV+P EaCDgcYZXueFWmfoivdyS2c9vGWEFnPiivgUDi3rTe58r2/wwvm7hunBVTvVajTHxMVo 9DPpRty5+VCXfsKIji6McORVpm85qQBHuCItWOaeSVfFysQNNTAY4E4cv2x7276V9LDb 4loMzQPkBmHVKSi4fKC74VvoDGIzQzIOSA0cAkCGOeEKAKskWjN5BwoziFAYL89NyMxt RRxuBqS5QYWs/tN5ZoTInBSx2+pqLuvGnHOt5jTiwINs+SKpXaMwBjOwyJ7o6G9RKCS5 jQEQ== X-Gm-Message-State: AJIora9t+IHbYxvVovWymWLiOhHzDyGNIl2r+AdJ27Nv8pZ4+bvrITGx LsOAIDYuk4cJHI7nguqbyu2OVrnx9v6Zkz/qnwkEkUJuLpMga4/GnBdeHrlY/tiN1MwakCxiJAR WaOrXVy5R4J0F87Hp4w== X-Received: by 2002:a05:622a:1a27:b0:304:f1cb:e8c4 with SMTP id f39-20020a05622a1a2700b00304f1cbe8c4mr1303249qtb.194.1655325530992; Wed, 15 Jun 2022 13:38:50 -0700 (PDT) X-Google-Smtp-Source: AGRyM1vp6GSojF0D/uWOTjqxrrhNx7zMUDLddmINpiEnku01m3bgsAt4PC5gz9AJ3T8irwWlMp3eNg== X-Received: by 2002:a05:622a:1a27:b0:304:f1cb:e8c4 with SMTP id f39-20020a05622a1a2700b00304f1cbe8c4mr1303225qtb.194.1655325530690; Wed, 15 Jun 2022 13:38:50 -0700 (PDT) Received: from [192.168.1.100] (130-44-159-43.s15913.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id y12-20020a05620a25cc00b006a6b374d8bbsm5076137qko.69.2022.06.15.13.38.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Jun 2022 13:38:50 -0700 (PDT) Message-ID: <6c78210d-f2a8-3371-5d88-74bdab399706@redhat.com> Date: Wed, 15 Jun 2022 16:38:49 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH RFA] ubsan: default to trap on unreachable at -O0 and -Og [PR104642] To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org, richard.guenther@gmail.com, jwakely.gcc@gmail.com References: <20220613195313.3240547-1-jason@redhat.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, 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: Wed, 15 Jun 2022 20:38:54 -0000 On 6/14/22 07:44, Jakub Jelinek wrote: > On Mon, Jun 13, 2022 at 03:53:13PM -0400, Jason Merrill via Gcc-patches wrote: >> When not optimizing, we can't do anything useful with unreachability in >> terms of code performance, so we might as well improve debugging by turning >> __builtin_unreachable into a trap. In the PR richi suggested introducing an >> -funreachable-traps flag for this, but this functionality is already >> implemented as -fsanitize=unreachable -fsanitize-undefined-trap-on-error, we >> just need to set those flags by default. >> >> I think it also makes sense to do this when we're explicitly optimizing for >> the debugging experience. >> >> I then needed to make options-save handle -fsanitize and >> -fsanitize-undefined-trap-on-error; since -fsanitize is has custom parsing, >> that meant handling it explicitly in the awk scripts. I also noticed we >> weren't setting it in opts_set. >> >> Do we still want -funreachable-traps as an alias (or separate flag) for this >> behavior that doesn't mention the sanitizer? > > 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=? > Your patch will e.g. silently enable the sanitization > whenever just -fsanitize-undefined-trap-on-error is passed, but that can be > passed even when users don't really expect any sanitization, just making > sure that if it is enabled (say for selected TUs or functions), it doesn't > need a runtime library to report it. I'm happy to drop that part. > 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 pass over the function to rewrite __builtin_unreachable, but that doesn't seem like much overhead. > 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. It seems to me that we want -funreachable-traps (or whatever spelling) to have exactly the same effect as the current -fsanitize=unreachable,return -fsanitize-undefined-trap-on-error, so using an entirely novel implementation strategy seems wrong. OTOH, maybe the sanitizer should also hook into fold_builtin_0 to rewrite many of the calls before any of the optimizers run, so we don't need to explicitly check SANITIZE_UNREACHABLE in e.g. optimize_unreachable. And I notice that we currently optimize away the call to f() in bool b; int f() { if (b) return 42; __builtin_unreachable (); } int main() { f(); } with -fsanitize=unreachable -O, so the test exits normally. > 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. Jason