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.
> > 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.
> > 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...
> > --- 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_
> > --- 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.
> > --- 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.
Jakub
