Add CLOBBER_MARKS_EOL to mark storage end-of-life clobbers

  This adds a flag to CONSTRUCTOR nodes indicating that for
clobbers this marks the end-of-life of storage as opposed to
just ending the lifetime of the object that occupied it.
The dangling pointer diagnostics uses CLOBBERs but is confused
by those emitted by the C++ frontend for example which emits
them for the second purpose at the start of CTORs.  The issue
is also appearant for aarch64 in PR104092.

Distinguishing the two cases is also necessary for the PR90348 fix.

Bootstrapped and tested on x86_64-unknown-linux-gnu.  I verified
the bogus diagnostic in PR104092 is gone with a cross.

OK for trunk?

Thanks,
Richard.

2022-02-02  Richard Biener  <rguenther@suse.de>

	PR middle-end/90348
	PR middle-end/104092
	* tree-core.h: Document protected_flag use on CONSTRUCTOR nodes.
	* tree.h (CLOBBER_MARKS_EOL): Add.
	* tree-pretty-print.cc (dump_generic_node): Mark EOL CLOBBERs.
	* gimplify.c (gimplify_bind_expr): Mark storage end-of-life clobbers
	with CLOBBER_MARKS_EOL.
	(gimplify_target_expr): Likewise.
	* tree-inline.cc (expand_call_inline): Likewise.
	* tree-ssa-ccp.cc (insert_clobber_before_stack_restore): Likewise.
	* gimple-ssa-warn-access.cc (pass_waccess::check_stmt): Only treat
	CLOBBER_MARKS_EOL clobbers as ending lifetime of storage.

	* gcc.dg/pr87052.c: Adjust.
---
 gcc/gimple-ssa-warn-access.cc  | 4 +++-
 gcc/gimplify.cc                | 2 ++
 gcc/testsuite/gcc.dg/pr87052.c | 2 +-
 gcc/tree-core.h                | 3 +++
 gcc/tree-inline.cc             | 2 ++
 gcc/tree-pretty-print.cc       | 6 +++++-
 gcc/tree-ssa-ccp.cc            | 1 +
 gcc/tree.h                     | 3 +++
 8 files changed, 20 insertions(+), 3 deletions(-)
  

On Wed, Feb 02, 2022 at 03:42:48PM +0100, Richard Biener via Gcc-patches wrote:
> This adds a flag to CONSTRUCTOR nodes indicating that for
> clobbers this marks the end-of-life of storage as opposed to
> just ending the lifetime of the object that occupied it.
> The dangling pointer diagnostics uses CLOBBERs but is confused
> by those emitted by the C++ frontend for example which emits
> them for the second purpose at the start of CTORs.  The issue
> is also appearant for aarch64 in PR104092.

I think many further build_clobber calls actually should build
those EOL clobbers, I think we'd need to go through them one by one and see
what kind of clobber it is.
E.g. I think all of omp-low.cc build_clobber calls are like that.
C++ FE indeed emits some clobbers at the start of ctors with
-flifetime-dse=2, but others e.g. at the end of dtors, dunno about those,
though maybe the object doesn't go out of scope at that point yet.
What about expansion and tree-ssa-live.cc, should those keep doing
what it does and not ignore clobbers that aren't CLOBBER_MARKS_EOL?

	Jakub
  

On Wed, 2 Feb 2022, Jakub Jelinek wrote:

> On Wed, Feb 02, 2022 at 03:42:48PM +0100, Richard Biener via Gcc-patches wrote:
> > This adds a flag to CONSTRUCTOR nodes indicating that for
> > clobbers this marks the end-of-life of storage as opposed to
> > just ending the lifetime of the object that occupied it.
> > The dangling pointer diagnostics uses CLOBBERs but is confused
> > by those emitted by the C++ frontend for example which emits
> > them for the second purpose at the start of CTORs.  The issue
> > is also appearant for aarch64 in PR104092.
> 
> I think many further build_clobber calls actually should build
> those EOL clobbers, I think we'd need to go through them one by one and see
> what kind of clobber it is.
> E.g. I think all of omp-low.cc build_clobber calls are like that.

Yeah, there may be others that also end lifetime of the storage.
It should be pretty safe since the only consumer after this patch
is the access warning.

> C++ FE indeed emits some clobbers at the start of ctors with
> -flifetime-dse=2, but others e.g. at the end of dtors, dunno about those,
> though maybe the object doesn't go out of scope at that point yet.
> What about expansion and tree-ssa-live.cc, should those keep doing
> what it does and not ignore clobbers that aren't CLOBBER_MARKS_EOL?

Yes.  Basically EOL clobbers act as regular clobbers _plus_ they
also note the end of life of the underlying storage, so treating
those like regular clobbers is safe, likewise it's not a regression
to not ignore !CLOBBER_MARKS_EOL clobbers elsewhere and I'd rather
have testcases showing the current behavior is wrong for those
than changing other places.

The inliner and CCP cases I ran into when fixing PR90348, I did not
run into any others sofar.

Richard.
  

Hello,

On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:

> This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> marks the end-of-life of storage as opposed to just ending the lifetime 
> of the object that occupied it. The dangling pointer diagnostics uses 
> CLOBBERs but is confused by those emitted by the C++ frontend for 
> example which emits them for the second purpose at the start of CTORs.  
> The issue is also appearant for aarch64 in PR104092.
> 
> Distinguishing the two cases is also necessary for the PR90348 fix.

(Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
particular those for marking births)

A style nit:

>  	      tree clobber = build_clobber (TREE_TYPE (t));
> +	      CLOBBER_MARKS_EOL (clobber) = 1;

I think it really were better if build_clobber() gained an additional 
argument (non-optional!) that sets the type of it.


Ciao,
Michael.
  

On 2/2/2022 7:42 AM, Richard Biener via Gcc-patches wrote:
> This adds a flag to CONSTRUCTOR nodes indicating that for
> clobbers this marks the end-of-life of storage as opposed to
> just ending the lifetime of the object that occupied it.
> The dangling pointer diagnostics uses CLOBBERs but is confused
> by those emitted by the C++ frontend for example which emits
> them for the second purpose at the start of CTORs.  The issue
> is also appearant for aarch64 in PR104092.
>
> Distinguishing the two cases is also necessary for the PR90348 fix.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.  I verified
> the bogus diagnostic in PR104092 is gone with a cross.
>
> OK for trunk?
>
> Thanks,
> Richard.
>
> 2022-02-02  Richard Biener  <rguenther@suse.de>
>
> 	PR middle-end/90348
> 	PR middle-end/104092
> 	* tree-core.h: Document protected_flag use on CONSTRUCTOR nodes.
> 	* tree.h (CLOBBER_MARKS_EOL): Add.
> 	* tree-pretty-print.cc (dump_generic_node): Mark EOL CLOBBERs.
> 	* gimplify.c (gimplify_bind_expr): Mark storage end-of-life clobbers
> 	with CLOBBER_MARKS_EOL.
> 	(gimplify_target_expr): Likewise.
> 	* tree-inline.cc (expand_call_inline): Likewise.
> 	* tree-ssa-ccp.cc (insert_clobber_before_stack_restore): Likewise.
> 	* gimple-ssa-warn-access.cc (pass_waccess::check_stmt): Only treat
> 	CLOBBER_MARKS_EOL clobbers as ending lifetime of storage.
>
> 	* gcc.dg/pr87052.c: Adjust.
OK.   Note that I think something similar may be needed to mark EOL for 
the pointer passed to realloc to fix a related set of false positives 
for code like this

   bool something = p != q;
   whatever = realloc (p, newsize)
   if (something)

We forward propagate the p != q test generating something like this;

   whatever - realloc (p, newsize);
   if (p != q)

Which triggers a warning even though the original source was 
reasonable.  I think a well placed clobber of p should expose the 
dataflow necessary to prevent the propagation and ultimately avoid the 
false positive.  IIRC something like this came up in glibc and/or gnulib.

I realize it's not exactly the same, but they're reasonably closely related.

jeff
  

On Wed, Feb 02, 2022 at 01:23:44PM -0700, Jeff Law via Gcc-patches wrote:
> Note that I think something similar may be needed to mark EOL for the
> pointer passed to realloc to fix a related set of false positives for code
> like this
> 
>   bool something = p != q;
>   whatever = realloc (p, newsize)
>   if (something)
> 
> We forward propagate the p != q test generating something like this;
> 
>   whatever - realloc (p, newsize);
>   if (p != q)

IMNSHO we shouldn't warn for pointer passed to realloc being used in
equality comparisons at all, it is just unnecessarily pedantic, it works
just fine and a lot of programs use it to find out if the memory was
actually reallocated or was just extended or shrunk in place; in the former
case they often need some extra work, such as adjust something in the memory
block.
Yes, one can probably do
  uintptr_t pint = (uintptr_t) p;
  whatever = realloc (p, newsize);
  if ((uintptr_t) whatever != pint)
but I think most people don't bother.

	Jakub
  

On Wed, 2 Feb 2022, Michael Matz wrote:

> Hello,
> 
> On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:
> 
> > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> > marks the end-of-life of storage as opposed to just ending the lifetime 
> > of the object that occupied it. The dangling pointer diagnostics uses 
> > CLOBBERs but is confused by those emitted by the C++ frontend for 
> > example which emits them for the second purpose at the start of CTORs.  
> > The issue is also appearant for aarch64 in PR104092.
> > 
> > Distinguishing the two cases is also necessary for the PR90348 fix.
> 
> (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
> particular those for marking births)
> 
> A style nit:
> 
> >  	      tree clobber = build_clobber (TREE_TYPE (t));
> > +	      CLOBBER_MARKS_EOL (clobber) = 1;
> 
> I think it really were better if build_clobber() gained an additional 
> argument (non-optional!) that sets the type of it.

It indeed did occur to me as well, so as we're two now I tried to see
how it looks like.  It does like the following (didn't bother to
replace all build_clobber calls but defaulted the parameter - there
are too many).  With CLOBBER_BIRTH added for the fix for PR90348
the enum would be constructed like

#define CLOBBER_KIND(NODE) \
  ((clobber_kind) (CONSTRUCTOR_CHECK (NODE)->base.private_flag << 1
                   | CONSTRUCTOR_CHECK (NODE)->base.protected_flag))

or so (maybe different dependent on bitfield endianess to make
extracting the two adjacent bits cheap).  That would leave us space
for a fourth state.

So do you think that makes it nicer?  What do others think?

Thanks,
Richard.
  

Richard Biener <rguenther@suse.de> writes:
> On Wed, 2 Feb 2022, Michael Matz wrote:
>
>> Hello,
>> 
>> On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:
>> 
>> > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
>> > marks the end-of-life of storage as opposed to just ending the lifetime 
>> > of the object that occupied it. The dangling pointer diagnostics uses 
>> > CLOBBERs but is confused by those emitted by the C++ frontend for 
>> > example which emits them for the second purpose at the start of CTORs.  
>> > The issue is also appearant for aarch64 in PR104092.
>> > 
>> > Distinguishing the two cases is also necessary for the PR90348 fix.
>> 
>> (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
>> particular those for marking births)
>> 
>> A style nit:
>> 
>> >  	      tree clobber = build_clobber (TREE_TYPE (t));
>> > +	      CLOBBER_MARKS_EOL (clobber) = 1;
>> 
>> I think it really were better if build_clobber() gained an additional 
>> argument (non-optional!) that sets the type of it.
>
> It indeed did occur to me as well, so as we're two now I tried to see
> how it looks like.  It does like the following (didn't bother to
> replace all build_clobber calls but defaulted the parameter - there
> are too many).  With CLOBBER_BIRTH added for the fix for PR90348
> the enum would be constructed like
>
> #define CLOBBER_KIND(NODE) \
>   ((clobber_kind) (CONSTRUCTOR_CHECK (NODE)->base.private_flag << 1
>                    | CONSTRUCTOR_CHECK (NODE)->base.protected_flag))
>
> or so (maybe different dependent on bitfield endianess to make
> extracting the two adjacent bits cheap).  That would leave us space
> for a fourth state.
>
> So do you think that makes it nicer?  What do others think?

This way looks cleaner to me FWIW.

Which arm of tree_base::u do constructors use?  If we need a 2-bit
enum then maybe we can carve one out from there.

Thanks,
Richard
  

On Thu, 3 Feb 2022, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > On Wed, 2 Feb 2022, Michael Matz wrote:
> >
> >> Hello,
> >> 
> >> On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:
> >> 
> >> > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> >> > marks the end-of-life of storage as opposed to just ending the lifetime 
> >> > of the object that occupied it. The dangling pointer diagnostics uses 
> >> > CLOBBERs but is confused by those emitted by the C++ frontend for 
> >> > example which emits them for the second purpose at the start of CTORs.  
> >> > The issue is also appearant for aarch64 in PR104092.
> >> > 
> >> > Distinguishing the two cases is also necessary for the PR90348 fix.
> >> 
> >> (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
> >> particular those for marking births)
> >> 
> >> A style nit:
> >> 
> >> >  	      tree clobber = build_clobber (TREE_TYPE (t));
> >> > +	      CLOBBER_MARKS_EOL (clobber) = 1;
> >> 
> >> I think it really were better if build_clobber() gained an additional 
> >> argument (non-optional!) that sets the type of it.
> >
> > It indeed did occur to me as well, so as we're two now I tried to see
> > how it looks like.  It does like the following (didn't bother to
> > replace all build_clobber calls but defaulted the parameter - there
> > are too many).  With CLOBBER_BIRTH added for the fix for PR90348
> > the enum would be constructed like
> >
> > #define CLOBBER_KIND(NODE) \
> >   ((clobber_kind) (CONSTRUCTOR_CHECK (NODE)->base.private_flag << 1
> >                    | CONSTRUCTOR_CHECK (NODE)->base.protected_flag))
> >
> > or so (maybe different dependent on bitfield endianess to make
> > extracting the two adjacent bits cheap).  That would leave us space
> > for a fourth state.
> >
> > So do you think that makes it nicer?  What do others think?
> 
> This way looks cleaner to me FWIW.
> 
> Which arm of tree_base::u do constructors use?  If we need a 2-bit
> enum then maybe we can carve one out from there.

constructors are tcc_exceptional but since they are used where
tree operands reside they need to be compatible to EXPR_P and
thus various things like TREE_SIDE_EFFECTS need to work.

Which means, we can't.

Richard.
  

Richard Biener <rguenther@suse.de> writes:
> On Thu, 3 Feb 2022, Richard Sandiford wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> > On Wed, 2 Feb 2022, Michael Matz wrote:
>> >
>> >> Hello,
>> >> 
>> >> On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:
>> >> 
>> >> > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
>> >> > marks the end-of-life of storage as opposed to just ending the lifetime 
>> >> > of the object that occupied it. The dangling pointer diagnostics uses 
>> >> > CLOBBERs but is confused by those emitted by the C++ frontend for 
>> >> > example which emits them for the second purpose at the start of CTORs.  
>> >> > The issue is also appearant for aarch64 in PR104092.
>> >> > 
>> >> > Distinguishing the two cases is also necessary for the PR90348 fix.
>> >> 
>> >> (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
>> >> particular those for marking births)
>> >> 
>> >> A style nit:
>> >> 
>> >> >  	      tree clobber = build_clobber (TREE_TYPE (t));
>> >> > +	      CLOBBER_MARKS_EOL (clobber) = 1;
>> >> 
>> >> I think it really were better if build_clobber() gained an additional 
>> >> argument (non-optional!) that sets the type of it.
>> >
>> > It indeed did occur to me as well, so as we're two now I tried to see
>> > how it looks like.  It does like the following (didn't bother to
>> > replace all build_clobber calls but defaulted the parameter - there
>> > are too many).  With CLOBBER_BIRTH added for the fix for PR90348
>> > the enum would be constructed like
>> >
>> > #define CLOBBER_KIND(NODE) \
>> >   ((clobber_kind) (CONSTRUCTOR_CHECK (NODE)->base.private_flag << 1
>> >                    | CONSTRUCTOR_CHECK (NODE)->base.protected_flag))
>> >
>> > or so (maybe different dependent on bitfield endianess to make
>> > extracting the two adjacent bits cheap).  That would leave us space
>> > for a fourth state.
>> >
>> > So do you think that makes it nicer?  What do others think?
>> 
>> This way looks cleaner to me FWIW.
>> 
>> Which arm of tree_base::u do constructors use?  If we need a 2-bit
>> enum then maybe we can carve one out from there.
>
> constructors are tcc_exceptional but since they are used where
> tree operands reside they need to be compatible to EXPR_P and
> thus various things like TREE_SIDE_EFFECTS need to work.
>
> Which means, we can't.

But TREE_SIDE_EFFECTS is part of the main set of flags.  For tree_base::u
we have:

  union {
    /* The bits in the following structure should only be used with
       accessor macros that constrain inputs with tree checking.  */
    struct {
      unsigned lang_flag_0 : 1;
      unsigned lang_flag_1 : 1;
      unsigned lang_flag_2 : 1;
      unsigned lang_flag_3 : 1;
      unsigned lang_flag_4 : 1;
      unsigned lang_flag_5 : 1;
      unsigned lang_flag_6 : 1;
      unsigned saturating_flag : 1;

      unsigned unsigned_flag : 1;
      unsigned packed_flag : 1;
      unsigned user_align : 1;
      unsigned nameless_flag : 1;
      unsigned atomic_flag : 1;
      unsigned unavailable_flag : 1;
      unsigned spare0 : 2;

      unsigned spare1 : 8;

      /* This field is only used with TREE_TYPE nodes; the only reason it is
	 present in tree_base instead of tree_type is to save space.  The size
	 of the field must be large enough to hold addr_space_t values.  */
      unsigned address_space : 8;
    } bits;

    /* The following fields are present in tree_base to save space.  The
       nodes using them do not require any of the flags above and so can
       make better use of the 4-byte sized word.  */

    /* The number of HOST_WIDE_INTs in an INTEGER_CST.  */
    struct {
      /* The number of HOST_WIDE_INTs if the INTEGER_CST is accessed in
	 its native precision.  */
      unsigned char unextended;

      /* The number of HOST_WIDE_INTs if the INTEGER_CST is extended to
	 wider precisions based on its TYPE_SIGN.  */
      unsigned char extended;

      /* The number of HOST_WIDE_INTs if the INTEGER_CST is accessed in
	 offset_int precision, with smaller integers being extended
	 according to their TYPE_SIGN.  This is equal to one of the two
	 fields above but is cached for speed.  */
      unsigned char offset;
    } int_length;

    /* VEC length.  This field is only used with TREE_VEC.  */
    int length;

    /* This field is only used with VECTOR_CST.  */
    struct {
      /* The value of VECTOR_CST_LOG2_NPATTERNS.  */
      unsigned int log2_npatterns : 8;

      /* The value of VECTOR_CST_NELTS_PER_PATTERN.  */
      unsigned int nelts_per_pattern : 8;

      /* For future expansion.  */
      unsigned int unused : 16;
    } vector_cst;

    /* SSA version number.  This field is only used with SSA_NAME.  */
    unsigned int version;

    /* CHREC_VARIABLE.  This field is only used with POLYNOMIAL_CHREC.  */
    unsigned int chrec_var;

    /* Internal function code.  */
    enum internal_fn ifn;

    /* OMP_ATOMIC* memory order.  */
    enum omp_memory_order omp_atomic_memory_order;

    /* The following two fields are used for MEM_REF and TARGET_MEM_REF
       expression trees and specify known data non-dependences.  For
       two memory references in a function they are known to not
       alias if dependence_info.clique are equal and dependence_info.base
       are distinct.  Clique number zero means there is no information,
       clique number one is populated from function global information
       and thus needs no remapping on transforms like loop unrolling.  */
    struct {
      unsigned short clique;
      unsigned short base;
    } dependence_info;
  } GTY((skip(""))) u;

so there's spare room even if constructors use the general “bits” arm.

Thanks,
Richard
  

On Thu, 3 Feb 2022, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > On Thu, 3 Feb 2022, Richard Sandiford wrote:
> >
> >> Richard Biener <rguenther@suse.de> writes:
> >> > On Wed, 2 Feb 2022, Michael Matz wrote:
> >> >
> >> >> Hello,
> >> >> 
> >> >> On Wed, 2 Feb 2022, Richard Biener via Gcc-patches wrote:
> >> >> 
> >> >> > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> >> >> > marks the end-of-life of storage as opposed to just ending the lifetime 
> >> >> > of the object that occupied it. The dangling pointer diagnostics uses 
> >> >> > CLOBBERs but is confused by those emitted by the C++ frontend for 
> >> >> > example which emits them for the second purpose at the start of CTORs.  
> >> >> > The issue is also appearant for aarch64 in PR104092.
> >> >> > 
> >> >> > Distinguishing the two cases is also necessary for the PR90348 fix.
> >> >> 
> >> >> (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
> >> >> particular those for marking births)
> >> >> 
> >> >> A style nit:
> >> >> 
> >> >> >  	      tree clobber = build_clobber (TREE_TYPE (t));
> >> >> > +	      CLOBBER_MARKS_EOL (clobber) = 1;
> >> >> 
> >> >> I think it really were better if build_clobber() gained an additional 
> >> >> argument (non-optional!) that sets the type of it.
> >> >
> >> > It indeed did occur to me as well, so as we're two now I tried to see
> >> > how it looks like.  It does like the following (didn't bother to
> >> > replace all build_clobber calls but defaulted the parameter - there
> >> > are too many).  With CLOBBER_BIRTH added for the fix for PR90348
> >> > the enum would be constructed like
> >> >
> >> > #define CLOBBER_KIND(NODE) \
> >> >   ((clobber_kind) (CONSTRUCTOR_CHECK (NODE)->base.private_flag << 1
> >> >                    | CONSTRUCTOR_CHECK (NODE)->base.protected_flag))
> >> >
> >> > or so (maybe different dependent on bitfield endianess to make
> >> > extracting the two adjacent bits cheap).  That would leave us space
> >> > for a fourth state.
> >> >
> >> > So do you think that makes it nicer?  What do others think?
> >> 
> >> This way looks cleaner to me FWIW.
> >> 
> >> Which arm of tree_base::u do constructors use?  If we need a 2-bit
> >> enum then maybe we can carve one out from there.
> >
> > constructors are tcc_exceptional but since they are used where
> > tree operands reside they need to be compatible to EXPR_P and
> > thus various things like TREE_SIDE_EFFECTS need to work.
> >
> > Which means, we can't.
> 
> But TREE_SIDE_EFFECTS is part of the main set of flags.  For tree_base::u
> we have:
> 
>   union {
>     /* The bits in the following structure should only be used with
>        accessor macros that constrain inputs with tree checking.  */
>     struct {
>       unsigned lang_flag_0 : 1;
>       unsigned lang_flag_1 : 1;
>       unsigned lang_flag_2 : 1;
>       unsigned lang_flag_3 : 1;
>       unsigned lang_flag_4 : 1;
>       unsigned lang_flag_5 : 1;
>       unsigned lang_flag_6 : 1;
>       unsigned saturating_flag : 1;
> 
>       unsigned unsigned_flag : 1;
>       unsigned packed_flag : 1;
>       unsigned user_align : 1;
>       unsigned nameless_flag : 1;
>       unsigned atomic_flag : 1;
>       unsigned unavailable_flag : 1;
>       unsigned spare0 : 2;
> 
>       unsigned spare1 : 8;
> 
>       /* This field is only used with TREE_TYPE nodes; the only reason it is
> 	 present in tree_base instead of tree_type is to save space.  The size
> 	 of the field must be large enough to hold addr_space_t values.  */
>       unsigned address_space : 8;
>     } bits;
> 
>     /* The following fields are present in tree_base to save space.  The
>        nodes using them do not require any of the flags above and so can
>        make better use of the 4-byte sized word.  */
> 
>     /* The number of HOST_WIDE_INTs in an INTEGER_CST.  */
>     struct {
>       /* The number of HOST_WIDE_INTs if the INTEGER_CST is accessed in
> 	 its native precision.  */
>       unsigned char unextended;
> 
>       /* The number of HOST_WIDE_INTs if the INTEGER_CST is extended to
> 	 wider precisions based on its TYPE_SIGN.  */
>       unsigned char extended;
> 
>       /* The number of HOST_WIDE_INTs if the INTEGER_CST is accessed in
> 	 offset_int precision, with smaller integers being extended
> 	 according to their TYPE_SIGN.  This is equal to one of the two
> 	 fields above but is cached for speed.  */
>       unsigned char offset;
>     } int_length;
> 
>     /* VEC length.  This field is only used with TREE_VEC.  */
>     int length;
> 
>     /* This field is only used with VECTOR_CST.  */
>     struct {
>       /* The value of VECTOR_CST_LOG2_NPATTERNS.  */
>       unsigned int log2_npatterns : 8;
> 
>       /* The value of VECTOR_CST_NELTS_PER_PATTERN.  */
>       unsigned int nelts_per_pattern : 8;
> 
>       /* For future expansion.  */
>       unsigned int unused : 16;
>     } vector_cst;
> 
>     /* SSA version number.  This field is only used with SSA_NAME.  */
>     unsigned int version;
> 
>     /* CHREC_VARIABLE.  This field is only used with POLYNOMIAL_CHREC.  */
>     unsigned int chrec_var;
> 
>     /* Internal function code.  */
>     enum internal_fn ifn;
> 
>     /* OMP_ATOMIC* memory order.  */
>     enum omp_memory_order omp_atomic_memory_order;
> 
>     /* The following two fields are used for MEM_REF and TARGET_MEM_REF
>        expression trees and specify known data non-dependences.  For
>        two memory references in a function they are known to not
>        alias if dependence_info.clique are equal and dependence_info.base
>        are distinct.  Clique number zero means there is no information,
>        clique number one is populated from function global information
>        and thus needs no remapping on transforms like loop unrolling.  */
>     struct {
>       unsigned short clique;
>       unsigned short base;
>     } dependence_info;
>   } GTY((skip(""))) u;
> 
> so there's spare room even if constructors use the general “bits” arm.

Ah, yes - I thought CTORs are using that for the number of elements
but they don't, they use a pointer to a vec<>.  So yes, for convenience
we could use bits in that section (with a bit more churn as they need to
be streamed for LTO, etc.)

I'll see to simply stick the plain enum there for CTORs.

Richard.
  

Hello,

On Thu, 3 Feb 2022, Richard Biener wrote:

> > > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> > > marks the end-of-life of storage as opposed to just ending the lifetime 
> > > of the object that occupied it. The dangling pointer diagnostics uses 
> > > CLOBBERs but is confused by those emitted by the C++ frontend for 
> > > example which emits them for the second purpose at the start of CTORs.  
> > > The issue is also appearant for aarch64 in PR104092.
> > > 
> > > Distinguishing the two cases is also necessary for the PR90348 fix.
> > 
> > (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
> > particular those for marking births)
> > 
> > A style nit:
> > 
> > >  	      tree clobber = build_clobber (TREE_TYPE (t));
> > > +	      CLOBBER_MARKS_EOL (clobber) = 1;
> > 
> > I think it really were better if build_clobber() gained an additional 
> > argument (non-optional!) that sets the type of it.
> 
> It indeed did occur to me as well, so as we're two now I tried to see
> how it looks like.  It does like the following (didn't bother to
> replace all build_clobber calls but defaulted the parameter - there
> are too many).

Except for this part I like it (I wouldn't call ca. 25 calls too 
many).  I often find optional arguments to be a long term maintenance 
burden when reading code.

(And yeah, enum good, putting the enum to tree_base.u, if it works, 
otherwise use your bit shuffling)


Ciao,
Michael.
  

On Thu, 3 Feb 2022, Michael Matz wrote:

> Hello,
> 
> On Thu, 3 Feb 2022, Richard Biener wrote:
> 
> > > > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> > > > marks the end-of-life of storage as opposed to just ending the lifetime 
> > > > of the object that occupied it. The dangling pointer diagnostics uses 
> > > > CLOBBERs but is confused by those emitted by the C++ frontend for 
> > > > example which emits them for the second purpose at the start of CTORs.  
> > > > The issue is also appearant for aarch64 in PR104092.
> > > > 
> > > > Distinguishing the two cases is also necessary for the PR90348 fix.
> > > 
> > > (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
> > > particular those for marking births)
> > > 
> > > A style nit:
> > > 
> > > >  	      tree clobber = build_clobber (TREE_TYPE (t));
> > > > +	      CLOBBER_MARKS_EOL (clobber) = 1;
> > > 
> > > I think it really were better if build_clobber() gained an additional 
> > > argument (non-optional!) that sets the type of it.
> > 
> > It indeed did occur to me as well, so as we're two now I tried to see
> > how it looks like.  It does like the following (didn't bother to
> > replace all build_clobber calls but defaulted the parameter - there
> > are too many).
> 
> Except for this part I like it (I wouldn't call ca. 25 calls too 
> many).  I often find optional arguments to be a long term maintenance 
> burden when reading code.

But I think in this case it makes clear that the remaining callers
are un-audited - as Jakub said some of them may want to use a
specific kind.  I also wanted to make the patch small just in case
we're considering the fix for GCC 12 ...

> (And yeah, enum good, putting the enum to tree_base.u, if it works, 
> otherwise use your bit shuffling)

It seems to.  Updated patch in testing.

Richard.
  

On Thu, 3 Feb 2022, Richard Biener wrote:

> On Thu, 3 Feb 2022, Michael Matz wrote:
> 
> > Hello,
> > 
> > On Thu, 3 Feb 2022, Richard Biener wrote:
> > 
> > > > > This adds a flag to CONSTRUCTOR nodes indicating that for clobbers this 
> > > > > marks the end-of-life of storage as opposed to just ending the lifetime 
> > > > > of the object that occupied it. The dangling pointer diagnostics uses 
> > > > > CLOBBERs but is confused by those emitted by the C++ frontend for 
> > > > > example which emits them for the second purpose at the start of CTORs.  
> > > > > The issue is also appearant for aarch64 in PR104092.
> > > > > 
> > > > > Distinguishing the two cases is also necessary for the PR90348 fix.
> > > > 
> > > > (Just FWIW: I agree with the plan to have different types of CLOBBERs, in 
> > > > particular those for marking births)
> > > > 
> > > > A style nit:
> > > > 
> > > > >  	      tree clobber = build_clobber (TREE_TYPE (t));
> > > > > +	      CLOBBER_MARKS_EOL (clobber) = 1;
> > > > 
> > > > I think it really were better if build_clobber() gained an additional 
> > > > argument (non-optional!) that sets the type of it.
> > > 
> > > It indeed did occur to me as well, so as we're two now I tried to see
> > > how it looks like.  It does like the following (didn't bother to
> > > replace all build_clobber calls but defaulted the parameter - there
> > > are too many).
> > 
> > Except for this part I like it (I wouldn't call ca. 25 calls too 
> > many).  I often find optional arguments to be a long term maintenance 
> > burden when reading code.
> 
> But I think in this case it makes clear that the remaining callers
> are un-audited - as Jakub said some of them may want to use a
> specific kind.  I also wanted to make the patch small just in case
> we're considering the fix for GCC 12 ...
> 
> > (And yeah, enum good, putting the enum to tree_base.u, if it works, 
> > otherwise use your bit shuffling)
> 
> It seems to.  Updated patch in testing.

Meh.  C++ uses LANG_FLAG_3 on CTORs at least, so I'm switching to
u.bits.address_space.

Richard.
  

Hello,

On Thu, 3 Feb 2022, Richard Biener via Gcc-patches wrote:

> > > It indeed did occur to me as well, so as we're two now I tried to 
> > > see how it looks like.  It does like the following (didn't bother to 
> > > replace all build_clobber calls but defaulted the parameter - there 
> > > are too many).
> > 
> > Except for this part I like it (I wouldn't call ca. 25 calls too 
> > many).  I often find optional arguments to be a long term maintenance 
> > burden when reading code.
> 
> But I think in this case it makes clear that the remaining callers are 
> un-audited

My pedantic answer to that would be that to make something clear you want 
to be explicit, and being explicit means writing something, not 
leaving something out, so you'd add another "CLOBBER_DONTKNOW" and use 
that for the unaudited calls.  Pedantic, as I said :)

But, of course, you built the shed, so paint it green, all right :)


Ciao,
Michael.