From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 13A893858D28 for ; Thu, 10 Feb 2022 10:09:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 13A893858D28 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id D5A811F391; Thu, 10 Feb 2022 10:09:06 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (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 CD988A3B8E; Thu, 10 Feb 2022 10:09:06 +0000 (UTC) Date: Thu, 10 Feb 2022 11:09:06 +0100 (CET) From: Richard Biener To: Jakub Jelinek cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] ubsan: Separate -f{,no-}delete-null-pointer-checks from -fsanitize={null,{,returns-}nonnull-attribute} [PR104426] In-Reply-To: <20220209150800.GQ2646553@tucnak> Message-ID: <67s7p3rq-9o72-p5r4-q3o8-6510s4rp6ro5@fhfr.qr> References: <20220209084713.GK2646553@tucnak> <24n8422o-osp8-4s44-n9r6-p76sq33444ns@fhfr.qr> <20220209091814.GM2646553@tucnak> <4p686rpp-8p16-ro7-q6q7-61n80rqr2rs@fhfr.qr> <20220209140511.GP2646553@tucnak> <474rro7n-79-392-1841-r351o16sqoo0@fhfr.qr> <20220209150800.GQ2646553@tucnak> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_STOCKGEN, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 10 Feb 2022 10:09:10 -0000 On Wed, 9 Feb 2022, Jakub Jelinek wrote: > On Wed, Feb 09, 2022 at 03:41:23PM +0100, Richard Biener wrote: > > On Wed, 9 Feb 2022, Jakub Jelinek wrote: > > > > > On Wed, Feb 09, 2022 at 11:19:25AM +0100, Richard Biener wrote: > > > > That does look like bogus abstraction though - I'd rather have > > > > the target be specific w/o option checks and replace > > > > targetm.zero_addres_valid uses with a wrapper (like you do for > > > > flag_delete_null_pointer_checks), if we think that the specific > > > > query should be adjusted by sanitize flags (why?) or > > > > folding_initializer (why?). > > > > > > Based on discussions on IRC, here is a WIP patch. > > > > > > Unfortunately, there are 3 unresolved issues: > > > 1) ipa-icf.cc uses > > > && opt_for_fn (decl, flag_delete_null_pointer_checks)) > > > there is a pointer type, but I guess we'd need to adjust the > > > target hook to take a defaulted fndecl argument and use that > > > for the options > > > > Yeah, I'd use a struct function arg tho, not a decl. > > But both opts_for_fn and sanitizer_flag_p take a fndecl tree, not cfun. Hmm, ok - the go for decl. > > > 2) rtlanal.cc has: > > > case SYMBOL_REF: > > > return flag_delete_null_pointer_checks && !SYMBOL_REF_WEAK (x); > > > Is there any way how to find out address space of a SYMBOL_REF? > > > > TYPE_ADDR_SPACE (TREE_TYPE (SYMBOL_REF_DECL ())) I guess. > > And default to ADDR_SPACE_GENERIC if there is no SYMBOL_REF_DECL? > That can work. Yeah, alternatively the caller would need to pass down the MEM since the address-space is only in the MEM attrs :/ > > > Or shall it hardcode ADDR_SPACE_GENERIC? > > > 3) tree-ssa-structalias.cc has: > > > if ((TREE_CODE (t) == INTEGER_CST > > > && integer_zerop (t)) > > > /* The only valid CONSTRUCTORs in gimple with pointer typed > > > elements are zero-initializer. But in IPA mode we also > > > process global initializers, so verify at least. */ > > > || (TREE_CODE (t) == CONSTRUCTOR > > > && CONSTRUCTOR_NELTS (t) == 0)) > > > { > > > if (flag_delete_null_pointer_checks) > > > temp.var = nothing_id; > > > else > > > temp.var = nonlocal_id; > > > temp.type = ADDRESSOF; > > > temp.offset = 0; > > > results->safe_push (temp); > > > return; > > > } > > > mpt really sure where to get the address space from in that case > > > > > > And perhaps I didn't do it right in some other spots too. > > > > This case is really difficult since we track pointers through integers > > (mind the missing POINTER_TYPE_P check above). Of course we have > > no idea what address-space the integer was converted from or will > > be converted to so what the above wants to check is whether > > there is _any_ address-space that could have a zero pointer pointing > > to a valid object ... > > Ugh. So that would be ADDR_SPACE_ANY ((unsigned char) -1) and use that > in the hook? > But we'd penalize x86 through it because for the __seg_?s address spaces > we allow 0 address... Yes :/ Alternatively we can have PTA give up on non-default address-space to/from non-pointer conversions which means not to track points-to across such transitions. Might be worth filing a tracking bug for this and leave things in the current slightly broken state? In this case it would mean using ADDR_SPACE_GENERIC. Also note that the specific place can cobble up multiple fields and thus fields with pointers to _different_ address-spaces ... > > > --- gcc/targhooks.cc.jj 2022-01-18 11:58:59.919977242 +0100 > > > +++ gcc/targhooks.cc 2022-02-09 13:21:08.958835833 +0100 > > > @@ -1598,7 +1598,7 @@ default_addr_space_subset_p (addr_space_ > > > bool > > > default_addr_space_zero_address_valid (addr_space_t as ATTRIBUTE_UNUSED) > > > { > > > - return false; > > > + return !flag_delete_null_pointer_checks_; > > > > As said, I'd not do that, but check it in zero_address_valid only. > > Otherwise all targets overriding the hook have to remember to check > > this flag. I suppose we'd then do > > > > if (option_set (flag_delete_null_pointer_check)) > > use flag_delete_null_pointer_check; > > else > > use targetm.zero_address_valid; > > > > possibly only for the default address-space. > > The advantage of checking the option in the hook is that it can precisely > decide what exactly it wants for each address space. It can e.g. decide > to ignore the flag and say that in some address space 0 is always valid or 0 > is never valid, or honor it under some conditions etc. > Doing it outside of the hook means we do the decision globally, and either > we hardcode targetm.addr_space.zero_address_valid || !flag_delete_null_pointer_check_, or > targetm.addr_space.zero_address_valid && !flag_delete_null_pointer_check_ As said I'm leaning towards documenting that -f[no-]delete-null-pointer-checks only has effects on the generic address-space. It's really a mess, unfortunately :/ > > > --- gcc/config/i386/i386.cc.jj 2022-02-09 12:55:50.716774241 +0100 > > > +++ gcc/config/i386/i386.cc 2022-02-09 13:23:01.041272540 +0100 > > > @@ -23804,7 +23804,9 @@ ix86_gen_scratch_sse_rtx (machine_mode m > > > static bool > > > ix86_addr_space_zero_address_valid (addr_space_t as) > > > { > > > - return as != ADDR_SPACE_GENERIC; > > > + if (as != ADDR_SPACE_GENERIC) > > > + return true; > > > > so this makes it not possibel to use -fdelete-null-pointer-checks to > > override the non-default address space behavior (on x86) > > Yes. To some extent that is already the current behavior as > targetm.addr_space.zero_address_valid is used in some spots explicitly. > But at least it is a target's decision and without introducing further > options like -fdelete-null-pointer-check=address_space > we need one or the other choice. See above. As you say, it's the current behavior already on some targets. > > > --- gcc/config/msp430/msp430.cc.jj 2022-02-04 14:36:54.410613609 +0100 > > > +++ gcc/config/msp430/msp430.cc 2022-02-09 13:04:09.372888416 +0100 > > > @@ -161,7 +161,7 @@ msp430_option_override (void) > > > { > > > /* The MSP430 architecture can safely dereference a NULL pointer. In fact, > > > there are memory mapped registers there. */ > > > - flag_delete_null_pointer_checks = 0; > > > + flag_delete_null_pointer_checks_ = 0; > > > > I'd use the target hook to return false and let > > -fdelete-null-pointer-checks override it. > > This one is a hardcoded override, so users have no choice, so perhaps > a different hook would work. > But the nios2 case is that it just provides a different default for the > switch, so if we hardcode && or || in the generic code, one or the other > option wouldn't work. > > BTW, perhaps we should have a more nuanced function and differentiate > between > 1) address 0 can be dereferenced in a particular as > 2) variables can appear at address 0 in a particular as > 3) functions can appear at address 0 in a particular as > (perhaps just use 2+3 together). > Because e.g. for the x86 TLS segment if TLS is supported address 0 can > be dereferenced, but no variable nor function can appear there - the ABI > says that address 0 contains the generic address space pointer to that > address. Hmm, I see. That makes things more complicated but yes ... if we split it up we can as well split 2 and 3. Also whether the constant pool can appear at address 0? Or whether the stack starts at address 0? Richard.