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 B1661384D165 for ; Wed, 29 Jun 2022 16:42:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B1661384D165 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-532-sZSp0rOLMga6c8av46Qjww-1; Wed, 29 Jun 2022 12:42:06 -0400 X-MC-Unique: sZSp0rOLMga6c8av46Qjww-1 Received: by mail-qv1-f70.google.com with SMTP id j6-20020a05621419c600b004704e6665caso15767122qvc.6 for ; Wed, 29 Jun 2022 09:42:06 -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=tjxXTEBnZAZiyzotQahrRXHbIwyNIlul1q8drhe1nPA=; b=H1HDRhiNDaV07pm2wXmKy1HXJyE0/ZVecLfOBXhMAxwvC1qwRFiPZ0IIEgCjQRzNjU 1KaN9ancRAuyOar2eP6wcUGB1ZYZ2cX9L/eQGFkNdmAw/jjf/YpbXbhoJ+bn/pfMohJ2 FFoBowO0hhUlymGcmyGT9+Xq8cYrP8Z70VASz9Otc7zD7e4PeWYTcmZ2Bq8do+dY0jiy oWjkajgDKHUBb0NfSiZhd0jrWYfN73td6bEI7GsLQg1jnLPJ/lPhB89mIrNhTrwNgR15 J+FDOONpMx24JH+Dti6WVkskd8KBaGp+0nJLjwoqhiXj001iah5PxveXlAamj57dVsfI 4WPQ== X-Gm-Message-State: AJIora+aP1YP6OsD+WECVNPy5XGMRgQrMzmceJMON9eo6LN3bKRTqMaN dU8IBoYcP0L302M8kKHR2UCAGoSvsxTs5ZvCBOxlTx7HbTH65f+vfpk6lpfMjNLKTy8ubPuKBL8 ENsR+fRzja5Qbo2fs/A== X-Received: by 2002:a05:6214:509e:b0:470:3fdb:3ebe with SMTP id kk30-20020a056214509e00b004703fdb3ebemr7371250qvb.81.1656520925862; Wed, 29 Jun 2022 09:42:05 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sk3RVJojbPxIAlc+/nNDepXirnbUBWPpO3R9J3X9KSz+2eUBp696THiPoa3fCsIywa0DmckQ== X-Received: by 2002:a05:6214:509e:b0:470:3fdb:3ebe with SMTP id kk30-20020a056214509e00b004703fdb3ebemr7371208qvb.81.1656520925515; Wed, 29 Jun 2022 09:42:05 -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 a5-20020ac85b85000000b00307cb34aa8asm11926257qta.47.2022.06.29.09.42.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Jun 2022 09:42:05 -0700 (PDT) Message-ID: <162bc331-56a6-ed11-872a-658235611ce2@redhat.com> Date: Wed, 29 Jun 2022 12:42:04 -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: do return check with -fsanitize=unreachable To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org References: <20220617212002.3747825-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=-6.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, 29 Jun 2022 16:42:10 -0000 On 6/27/22 11:44, Jakub Jelinek wrote: > On Wed, Jun 22, 2022 at 12:04:59AM -0400, Jason Merrill wrote: >> On 6/20/22 16:16, Jason Merrill wrote: >>> On 6/20/22 07:05, Jakub Jelinek wrote: >>>> On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote: >>>>> Related to PR104642, the current situation where we get less >>>>> return checking >>>>> with just -fsanitize=unreachable than no sanitize flags seems >>>>> undesirable; I >>>>> propose that we do return checking when -fsanitize=unreachable. >>>> >>>> __builtin_unreachable itself (unless turned into trap or >>>> __ubsan_handle_builtin_unreachable) is not any kind of return >>>> checking, it >>>> is just an optimization. >>> >>> Yes, but I'm talking about "when -fsanitize=unreachable". > > The usual case is that people just use -fsanitize=undefined > and get both return and unreachable sanitization, for fall through > into end of functions returning non-void done through return sanitization. > > In the rare case people use something different like > -fsanitize=undefined -fno-sanitize=return > or > -fsanitize=unreachable > etc., they presumably don't want the fall through from end of function > diagnosed at runtime. I disagree with this assumption for the second case; it seems much more likely to me that the user just wasn't thinking about needing to also mention return. Missing return is a logical subset of unreachable; if we sanitize all the other __builtin_unreachable introduced by the compiler, why in the world would we want to leave out this one that is such a frequent error? Full -fsanitize=undefined is much higher overhead than just -fsanitize=unreachable, which introduces no extra checks. And adding return checking to unreachable is essentially zero overhead. I can't imagine any reason to want to check unreachable points EXCEPT for missing return. > I think the behavior we want is: > 1) -fsanitize=return is on -> use ubsan_instrument_return > (__ubsan_missing_return_data or __builtin_trap depending on > -fsanitize-trap=return); otherwise > 2) -funreachable-traps is on (from -O0/-Og by default or explicit), > emit __builtin_trap; otherwise > 3) -fsanitize=unreachable is on, not emit anything (__builtin_unreachable > would be just an optimization, but user asked not to instrument returns, > only unreachable, so honor user's decision and avoid confusion); otherwise > 4) -O0 is on, not emit anything (__builtin_unreachable wouldn't be much > of an optimization, just surprising and hard to debug effect); otherwise > 5) emit __builtin_unreachable > > Current trunk with your PR104642 fix in implements 1), will do 2) > only if -fsanitize=unreachable is not on, will do 3), will do 4) and 5). > > So, I'd change cp-gimplify.cc (cp_maybe_instrument_return), change: > if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) > && ((!optimize && !flag_unreachable_traps) > || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) > return; > to > if (!sanitize_flags_p (SANITIZE_RETURN, fndecl) > && !flag_unreachable_traps > && (!optimize || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl))) > return; > and > if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) > t = ubsan_instrument_return (loc); > else > t = build_builtin_unreachable (BUILTINS_LOCATION); > to > if (sanitize_flags_p (SANITIZE_RETURN, fndecl)) > t = ubsan_instrument_return (loc); > else if (flag_unreachable_traps) > t = build_call_expr_loc (BUILTINS_LOCATION, > builtin_decl_explicit (BUILT_IN_TRAP), 0); > else > t = build_builtin_unreachable (BUILTINS_LOCATION); > > Jakub >