[Bug tree-optimization/113476] New: [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[Bug tree-optimization/113476] New: [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

            Bug ID: 113476
           Summary: [14 Regression] irange::maybe_resize leaks memory via
                    IPA VRP
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

I see

==1854== 122,424 bytes in 3 blocks are possibly lost in loss record 1,365 of
1,373
==1854==    at 0x505A1DF: operator new[](unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1854==    by 0x1B8AB58: irange::maybe_resize(int) (value-range.h:645)
==1854==    by 0x1B7C7DF: irange::union_(vrange const&) (value-range.cc:1451)
==1854==    by 0x1B1D777: Value_Range::union_(vrange const&)
(value-range.h:705)
==1854==    by 0x30355A4: ipcp_vr_lattice::meet_with_1(vrange const&)
(ipa-cp.cc:1046)
==1854==    by 0x3035508: ipcp_vr_lattice::meet_with(vrange const&)
(ipa-cp.cc:1

where union_ does

  // At this point, the vector should have i ranges, none overlapping.
  // Now it simply needs to be copied, and if there are too many
  // ranges, merge some.  We wont do any analysis as to what the
  // "best" merges are, simply combine the final ranges into one.
  maybe_resize (i / 2);

and maybe_resize:

inline void
irange::maybe_resize (int needed)
{ 
  if (!m_resizable || m_max_ranges == HARD_MAX_RANGES)
    return;

  if (needed > m_max_ranges)
    {
      m_max_ranges = HARD_MAX_RANGES;
      wide_int *newmem = new wide_int[m_max_ranges * 2];
      unsigned n = num_pairs () * 2;
      for (unsigned i = 0; i < n; ++i)
        newmem[i] = m_base[i];
      m_base = newmem;
    }

that looks OK at first right but there's an irange CTOR

inline
irange::irange (wide_int *base, unsigned nranges, bool resizable)

that might end up with unexpected m_base/m_max_ranges here.

Testcase is dwarf2out.i preprocessed from the last release using C but I
guess it's easy to reproduce with any TU from GCC itself.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com
   Target Milestone|---                         |14.0
           Keywords|                            |memory-hog

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 57138
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57138&action=edit
testcase

For convenience here it is.  I checked

valgrind --leak-check=full ./cc1 -quiet -O3 -march=znver4
~/src/cc1files/dwarf2out.i -o /dev/null

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
In fact it occurs elsewhere as well:

==1854== 81,616 bytes in 2 blocks are possibly lost in loss record 1,363 of
1,373
==1854==    at 0x505A1DF: operator new[](unsigned long) (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1854==    by 0x1B8AB58: irange::maybe_resize(int) (value-range.h:645)
==1854==    by 0x1B79873: irange::operator=(irange const&) (value-range.cc:948)
==1854==    by 0x139779E: int_range<3u, true>::operator=(int_range<3u, true>
const&) (value-range.h:1061)
==1854==    by 0x2FAFBAE: phi_group::phi_group(phi_group const&)
(gimple-range-phi.cc:84)
==1854==    by 0x2FB0E7A: phi_analyzer::process_phi(gphi*)
(gimple-range-phi.cc:460)
==1854==    by 0x2FB0707: phi_analyzer::operator[](tree_node*)
(gimple-range-phi.cc:314)
==1854==    by 0x2F9E0E8: fold_using_range::range_of_phi(vrange&, gphi*,
fur_source&) (gimple-range-fold.cc:949)
==1854==    by 0x2F9C6AD: fold_using_range::fold_stmt(vrange&, gimple*,
fur_source&, tree_node*) (gimple-range-fold.cc:604)
==1854==    by 0x2F8D937: gimple_ranger::fold_range_internal(vrange&, gimple*,
tree_node*) (gimple-range.cc:265)
==1854==    by 0x2F8DC6D: gimple_ranger::range_of_stmt(vrange&, gimple*,
tree_node*) (gimple-range.cc:326)
==1854==    by 0x2F8E4D5: gimple_ranger::register_inferred_ranges(gimple*)
(gimple-range.cc:486)

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |jamborm at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Ah, so it's we have

class ipcp_vr_lattice
{ 
public:
  Value_Range m_vr;

  inline bool bottom_p () const;
  inline bool top_p () const;
  inline bool set_to_bottom ();
  bool meet_with (const vrange &p_vr);
  bool meet_with (const ipcp_vr_lattice &other);
  void init (tree type);
  void print (FILE * f);

private:
  bool meet_with_1 (const vrange &other_vr);
};

so no explicit DTOR and if we destroy the object then Value_Range doesn't
have a CTOR.  I also don't see we destroy ipcp_param_lattices?  Of course
irange doesn't have a DTOR in the first place ...

Martin?  Andrew?

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #4 from Martin Jambor <jamborm at gcc dot gnu.org> ---
The right place where to free stuff in lattices post-IPA would be in
ipa_node_params::~ipa_node_params() where we should iterate over lattices and
deinitialize them or perhaps destruct the array because since ipcp_vr_lattice
directly contains Value_Range which AFAIU directly contains int_range_max which
has a virtual destructor... does not look like a POD anymore.  This has escaped
me when I was looking at the IPA-VR changes but hopefully it should not be too
difficult to deal with.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2024-01-23
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jamborm at gcc dot gnu.org

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #4)
> The right place where to free stuff in lattices post-IPA would be in
> ipa_node_params::~ipa_node_params() where we should iterate over lattices
> and deinitialize them or perhaps destruct the array because since
> ipcp_vr_lattice directly contains Value_Range which AFAIU directly contains
> int_range_max which has a virtual destructor... does not look like a POD
> anymore.  This has escaped me when I was looking at the IPA-VR changes but
> hopefully it should not be too difficult to deal with.

OK, that might work for the IPA side.

It's quite unusual to introduce a virtual DTOR in the middle of the class
hierarchy though.  Grepping I do see quite some direct uses of 'irange'
and also 'vrange' which do not have the DTOR visible but 'irange' already
exposes and uses 'maybe_resize'.  I think those should only be introduced
in the class exposing the virtual DTOR (but why virtual?!).

Would be nice to have a picture of the range class hierarchies with
pointers on which types to use in which circumstances ...

For example:

  Value_Range vr (parm_type);
...
           irange &r = as_a <irange> (vr);
           irange_bitmask bm = r.get_bitmask ();
...

should that really use 'irange'?  Why not int_range&?

All the complication might be because of GC (irange is GTY but int_range is
not), but re-allocation would happen with 'new', not ggc_alloc, so ...

But yes, please try to fix IPA CP, I'll see if this pops up elsewhere as well
then.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #6 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I have proposed a patch on the mailing list that converts the array of lattices
to a vector:
https://inbox.sourceware.org/gcc-patches/ri6frxoxzpk.fsf@virgil.suse.cz/T/#u

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #7 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
Let me see if I can untangle things here.  Thanks for chasing
this down, BTW.

Value_Range doesn't need a CTOR because it has an int_range_max which
does have one (courtesy of int_range<>), so things get initialized
correctly.  Deleting a memory allocated container that has a
Value_Range also works, because of the ~int_range<> destructor:

+  {
+    struct container
+    {
+      Value_Range vr;
+    };
+    struct container *pointer = new container;
+    pointer->vr.set_type (integer_type_node);
+    for (int i = 0; i < 100; i += 5)
+      {
+       int_range<1> tmp (integer_type_node,
+                         wi::shwi (i, TYPE_PRECISION (integer_type_node)),
+                         wi::shwi (i+1, TYPE_PRECISION (integer_type_node)));
+       pointer->vr.union_ (tmp);
+      }
+    delete pointer;

Deleting pointer causes us to call int_range<N,
RESIZABLE>::~int_range() which clean things appropriately.

So it looks like the problem is the missing destructor in IPA.

That being said, there's a few things I've noticed thanks to you guys'
analysis.

First, the virtual marker in the int_range<> destructor is not needed.
IPA + ranges went through various changes this release, and I believe
that was left over from when value_range (int_range<2>) was being
placed in GC space directly.

We went through various incantations this release wrt IPA storage of
ranges, both in GC and in the stack.  Ultimately we ended up with
ipa_vr, which I believe is the only use of ranges in GC space, and
furthermore it uses vrange_storage not vrange nor irange nor anything
else.  For that matter, vrange_storage doesn't even have pointers, so
we shouldn't need GTY markers at all.

It seems that since IPA is the only GC user of iranges, I think we
can safely remove GTY support from the entire vrange hierarchy.

The rule of thumb should be that only int_range<> and derivatives
should be in short-term storage, and GC users should use
vrange_storage (like ipa_vr is doing, or perhaps vrange_allocator
which automates the allocation).

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #8 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> (In reply to Martin Jambor from comment #4)
> > The right place where to free stuff in lattices post-IPA would be in
> > ipa_node_params::~ipa_node_params() where we should iterate over lattices
> > and deinitialize them or perhaps destruct the array because since
> > ipcp_vr_lattice directly contains Value_Range which AFAIU directly contains
> > int_range_max which has a virtual destructor... does not look like a POD
> > anymore.  This has escaped me when I was looking at the IPA-VR changes but
> > hopefully it should not be too difficult to deal with.
> 
> OK, that might work for the IPA side.
> 
> It's quite unusual to introduce a virtual DTOR in the middle of the class
> hierarchy though.  Grepping I do see quite some direct uses of 'irange'
> and also 'vrange' which do not have the DTOR visible but 'irange' already
> exposes and uses 'maybe_resize'.  I think those should only be introduced
> in the class exposing the virtual DTOR (but why virtual?!).
> 
> Would be nice to have a picture of the range class hierarchies with
> pointers on which types to use in which circumstances ...

For reference, you should always use the most base class you can.  If
you can get the work done with the vrange API, use that.  If you're
dealing with an integer, use irange.  The int_range<> derived class
should only be used for defining a variable, so:

int_range<123> foobar; // Defined with storage.

if (is_a <irange>)
{
  irange &p = as_a <irange> (foobar);
  ...
}

// Use an irange reference here, not an int_range reference.
void somefunc (const irange &r)
{
}

Also, the reason irange does not have a destructor is because it has
no storage.  Only int_range<> has storage associated with it, so it is
the only one able to see if the range grew:

template<unsigned N, bool RESIZABLE>
inline
int_range<N, RESIZABLE>::~int_range ()
{
  if (RESIZABLE && m_base != m_ranges)
    delete[] m_base;
}

The irange superclass does not have storage, just the m_base pointer.
OTOH, m_ranges[] lives in int_range<>:

template<unsigned N, bool RESIZABLE = false>
class int_range : public irange
{
...
private:
  wide_int m_ranges[N*2];
};

This shouldn't be a problem because you should never be putting a raw irange
pointer in a container that you allocate/deallocate.

Does this help?  If so, I could definitely document it in value-range.h.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #9 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
Created attachment 57477
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57477&action=edit
Remove virtual from int_range destructor.

Bootstraps.  Tests are pending.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #10 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
Created attachment 57478
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57478&action=edit
Remove GTY support for vrange and friends

Bootstraps.  Tests are pending.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #11 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
Both patches pass test.  Up to the release maintainers to decide if they want
to include them in this release.  Otherwise, I'll queue them up for later.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Aldy Hernandez from comment #11)
> Both patches pass test.  Up to the release maintainers to decide if they
> want to include them in this release.  Otherwise, I'll queue them up for
> later.

This is an important regression, why shouldn't it be included in GCC 14?
GCC trunk is open for regression and documentation fixes...

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #13 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #12)
> (In reply to Aldy Hernandez from comment #11)
> > Both patches pass test.  Up to the release maintainers to decide if they
> > want to include them in this release.  Otherwise, I'll queue them up for
> > later.
> 
> This is an important regression, why shouldn't it be included in GCC 14?
> GCC trunk is open for regression and documentation fixes...

Martin's change to IPA fixes the leak in the PR.  My changes are just cleanups,
as no other pass is using ranges in GC space.

[Bug tree-optimization/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ah, ok, in that case it can wait for stage1.

[Bug ipa/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #15 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Jambor <jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:c8742849e22d004b6ab94b3f573639f763e42e3a

commit r14-9115-gc8742849e22d004b6ab94b3f573639f763e42e3a
Author: Martin Jambor <mjambor@suse.cz>
Date:   Wed Feb 21 15:43:13 2024 +0100

    ipa: Convert lattices from pure array to vector (PR 113476)

    In PR 113476 we have discovered that ipcp_param_lattices is no longer
    a POD and should be destructed.  In a follow-up discussion it
    transpired that their initialization done by memsetting their backing
    memory to zero is also invalid because now any write there before
    construction can be considered dead.  Plus that having them in an
    array is a little bit old-school and does not get the extra checking
    offered by vector along with automatic construction and destruction
    when necessary.

    So this patch converts the array to a vector.  That however means that
    ipcp_param_lattices cannot be just a forward declared type but must be
    known to all code that deals with ipa_node_params and thus to all code
    that includes ipa-prop.h.  Therefore I have moved ipcp_param_lattices
    and the type it depends on to a new header ipa-cp.h which now
    ipa-prop.h depends on.  Because we have the (IMHO not a very wise)
    rule that headers don't include what they need themselves, I had to
    add inclusions of ipa-cp.h and sreal.h (on which it depends) to very
    many files, which made the patch rather ugly.

    gcc/lto/ChangeLog:

    2024-02-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/113476
            * lto-common.cc: Include sreal.h and ipa-cp.h.
            * lto-partition.cc: Include ipa-cp.h, move inclusion of sreal
higher.
            * lto.cc: Include sreal.h and ipa-cp.h.

    gcc/ChangeLog:

    2024-02-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/113476
            * ipa-prop.h (ipa_node_params): Convert lattices to a vector,
adjust
            initializers in the contructor.
            (ipa_node_params::~ipa_node_params): Release lattices as a vector.
            * ipa-cp.h: New file.
            * ipa-cp.cc: Include sreal.h and ipa-cp.h.
            (ipcp_value_source): Move to ipa-cp.h.
            (ipcp_value_base): Likewise.
            (ipcp_value): Likewise.
            (ipcp_lattice): Likewise.
            (ipcp_agg_lattice): Likewise.
            (ipcp_bits_lattice): Likewise.
            (ipcp_vr_lattice): Likewise.
            (ipcp_param_lattices): Likewise.
            (ipa_get_parm_lattices): Remove assert latticess is non-NULL.
            (ipa_value_from_jfunc): Adjust a check for empty lattices.
            (ipa_context_from_jfunc): Likewise.
            (ipa_agg_value_from_jfunc): Likewise.
            (merge_agg_lats_step): Do not memset new aggregate lattices to
zero.
            (ipcp_propagate_stage): Allocate lattices in a vector as opposed to
            just in contiguous memory.
            (ipcp_store_vr_results): Adjust a check for empty lattices.
            * auto-profile.cc: Include sreal.h and ipa-cp.h.
            * cgraph.cc: Likewise.
            * cgraphclones.cc: Likewise.
            * cgraphunit.cc: Likewise.
            * config/aarch64/aarch64.cc: Likewise.
            * config/i386/i386-builtins.cc: Likewise.
            * config/i386/i386-expand.cc: Likewise.
            * config/i386/i386-features.cc: Likewise.
            * config/i386/i386-options.cc: Likewise.
            * config/i386/i386.cc: Likewise.
            * config/rs6000/rs6000.cc: Likewise.
            * config/s390/s390.cc: Likewise.
            * gengtype.cc (open_base_files): Added sreal.h and ipa-cp.h to the
            files to be included in gtype-desc.cc.
            * gimple-range-fold.cc: Include sreal.h and ipa-cp.h.
            * ipa-devirt.cc: Likewise.
            * ipa-fnsummary.cc: Likewise.
            * ipa-icf.cc: Likewise.
            * ipa-inline-analysis.cc: Likewise.
            * ipa-inline-transform.cc: Likewise.
            * ipa-inline.cc: Include ipa-cp.h, move inclusion of sreal.h
higher.
            * ipa-modref.cc: Include sreal.h and ipa-cp.h.
            * ipa-param-manipulation.cc: Likewise.
            * ipa-predicate.cc: Likewise.
            * ipa-profile.cc: Likewise.
            * ipa-prop.cc: Likewise.
            (ipa_node_params_t::duplicate): Assert new lattices remain empty
            instead of setting them to NULL.
            * ipa-pure-const.cc: Include sreal.h and ipa-cp.h.
            * ipa-split.cc: Likewise.
            * ipa-sra.cc: Likewise.
            * ipa-strub.cc: Likewise.
            * ipa-utils.cc: Likewise.
            * ipa.cc: Likewise.
            * toplev.cc: Likewise.
            * tree-ssa-ccp.cc: Likewise.
            * tree-ssa-sccvn.cc: Likewise.
            * tree-vrp.cc: Likewise.

[Bug ipa/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #16 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Fixed.

[Bug ipa/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #17 from rguenther at suse dot de <rguenther at suse dot de> ---
On Wed, 21 Feb 2024, aldyh at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476
> 
> --- Comment #8 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
> (In reply to Richard Biener from comment #5)
> > (In reply to Martin Jambor from comment #4)
> > > The right place where to free stuff in lattices post-IPA would be in
> > > ipa_node_params::~ipa_node_params() where we should iterate over lattices
> > > and deinitialize them or perhaps destruct the array because since
> > > ipcp_vr_lattice directly contains Value_Range which AFAIU directly contains
> > > int_range_max which has a virtual destructor... does not look like a POD
> > > anymore.  This has escaped me when I was looking at the IPA-VR changes but
> > > hopefully it should not be too difficult to deal with.
> > 
> > OK, that might work for the IPA side.
> > 
> > It's quite unusual to introduce a virtual DTOR in the middle of the class
> > hierarchy though.  Grepping I do see quite some direct uses of 'irange'
> > and also 'vrange' which do not have the DTOR visible but 'irange' already
> > exposes and uses 'maybe_resize'.  I think those should only be introduced
> > in the class exposing the virtual DTOR (but why virtual?!).
> > 
> > Would be nice to have a picture of the range class hierarchies with
> > pointers on which types to use in which circumstances ...
> 
> For reference, you should always use the most base class you can.  If
> you can get the work done with the vrange API, use that.  If you're
> dealing with an integer, use irange.  The int_range<> derived class
> should only be used for defining a variable, so:
> 
> int_range<123> foobar; // Defined with storage.
> 
> if (is_a <irange>)
> {
>   irange &p = as_a <irange> (foobar);
>   ...
> }
> 
> // Use an irange reference here, not an int_range reference.
> void somefunc (const irange &r)
> {
> }
> 
> Also, the reason irange does not have a destructor is because it has
> no storage.  Only int_range<> has storage associated with it, so it is
> the only one able to see if the range grew:

But when I do

 irange *r = new int_range<>;
 delete r;

then it will fail to release memory?  Are virtual DTORs not exactly
invented for this, when you delete on a reference/pointer to a base
class but the object is really a derived one?

That's what I was refering to with "introducing a virtual DTOR
in a not base-class".

I still think that's bad OO design, no?

[Bug ipa/113476] [14 Regression] irange::maybe_resize leaks memory via IPA VRP

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476

--- Comment #18 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #17)
> On Wed, 21 Feb 2024, aldyh at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113476
> > 
> > --- Comment #8 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
> > (In reply to Richard Biener from comment #5)
> > > (In reply to Martin Jambor from comment #4)
> > > > The right place where to free stuff in lattices post-IPA would be in
> > > > ipa_node_params::~ipa_node_params() where we should iterate over lattices
> > > > and deinitialize them or perhaps destruct the array because since
> > > > ipcp_vr_lattice directly contains Value_Range which AFAIU directly contains
> > > > int_range_max which has a virtual destructor... does not look like a POD
> > > > anymore.  This has escaped me when I was looking at the IPA-VR changes but
> > > > hopefully it should not be too difficult to deal with.
> > > 
> > > OK, that might work for the IPA side.
> > > 
> > > It's quite unusual to introduce a virtual DTOR in the middle of the class
> > > hierarchy though.  Grepping I do see quite some direct uses of 'irange'
> > > and also 'vrange' which do not have the DTOR visible but 'irange' already
> > > exposes and uses 'maybe_resize'.  I think those should only be introduced
> > > in the class exposing the virtual DTOR (but why virtual?!).
> > > 
> > > Would be nice to have a picture of the range class hierarchies with
> > > pointers on which types to use in which circumstances ...
> > 
> > For reference, you should always use the most base class you can.  If
> > you can get the work done with the vrange API, use that.  If you're
> > dealing with an integer, use irange.  The int_range<> derived class
> > should only be used for defining a variable, so:
> > 
> > int_range<123> foobar; // Defined with storage.
> > 
> > if (is_a <irange>)
> > {
> >   irange &p = as_a <irange> (foobar);
> >   ...
> > }
> > 
> > // Use an irange reference here, not an int_range reference.
> > void somefunc (const irange &r)
> > {
> > }
> > 
> > Also, the reason irange does not have a destructor is because it has
> > no storage.  Only int_range<> has storage associated with it, so it is
> > the only one able to see if the range grew:
> 
> But when I do
> 
>  irange *r = new int_range<>;
>  delete r;
> 
> then it will fail to release memory?  Are virtual DTORs not exactly
> invented for this, when you delete on a reference/pointer to a base
> class but the object is really a derived one?

There is no memory to release above, as int_range<> does not grow by default. 
Only int_range_max can grow, and it's meant to live on the stack by design. 
That was the whole point:

typedef int_range<3, /*RESIZABLE=*/true> int_range_max;

I suppose if you specifically wanted to shoot yourself in the foot, you could
do:

irange *r = new int_range<5, true>;

...and that would fail to release memory, but why would you do that?  If you're
allocating a lot of ranges, we have the vrange_allocator written specifically
for that, which is a lot less memory intensive.  It's what we use in the cache.

If ranges are anywhere but the stack, they should go through the allocator
which will squish down things appropriately, as iranges are very big.  The
ipa_vr class also uses the allocator in order to keep the memory footprint
down.

I guess it wouldn't hurt to have a virtual destructor in irange or even vrange
for future proofing, but it's not needed right now.  I don't have any strong
opinions, unless there's a performance penalty.