Re: [PATCH] ubsan: Separate -f{,no-}delete-null-pointer-checks from -fsanitize={null,{,returns-}nonnull-attribute} [PR104426]

[Richard Biener <rguenther@suse.de>]

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.