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.
