[PATCH] document enable/disable_ranger

[PATCH] document enable/disable_ranger

[Martin Sebor]

Hey Aldy & Andrew,

I introduced a leak by calling enable_ranger() without pairing it
with one to disable_ranger() on the same function (PR 101984).
I didn't realize (or look to see) that enable_ranger() dynamically
allocates memory.

The patch below adds comments to make it clear that the calls need
to be paired.  That seems obvious now but wasn't before from just
the function names.  So I'm wondering if we might want to rename
them to make it more obvious that the former involves allocating
memory that must be explicitly deallocated.

If you agree, names along the following lines would make this
clearer (to me, anyway) but I'm open to others:

   gimple_ranger *set_new_ranger (function *);
   void release_ranger (function *);

Martin

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index 60b7d3a59cd..ef3afeacc90 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -381,6 +381,10 @@ gimple_ranger::dump (FILE *f)
    m_cache.dump (f);
  }

+/* Create a new ranger instance and associate it with function FUN.
+   Each call must be paired with a call to disable_ranger to release
+   resources.  */
+
  gimple_ranger *
  enable_ranger (struct function *fun)
  {
@@ -392,6 +396,9 @@ enable_ranger (struct function *fun)
    return r;
  }

+/* Destroy and release the ranger instance associated with function FUN
+   and replace it with the global ranger.  */
+
  void
  disable_ranger (struct function *fun)
  {
diff --git a/gcc/gimple-range.h b/gcc/gimple-range.h
index 41845b14fd6..eaebb9c5833 100644
--- a/gcc/gimple-range.h
+++ b/gcc/gimple-range.h
@@ -62,6 +62,9 @@ protected:
    range_tracer tracer;
  };

+/* Create a new ranger instance and associate it with a function.
+   Each call must be paired with a call to disable_ranger to release
+   resources.  */
  extern gimple_ranger *enable_ranger (struct function *);
  extern void disable_ranger (struct function *);

Re: [PATCH] document enable/disable_ranger

[David Malcolm]

On Thu, 2021-08-19 at 11:30 -0600, Martin Sebor via Gcc-patches wrote:
> Hey Aldy & Andrew,
> 
> I introduced a leak by calling enable_ranger() without pairing it
> with one to disable_ranger() on the same function (PR 101984).
> I didn't realize (or look to see) that enable_ranger() dynamically
> allocates memory.
> 
> The patch below adds comments to make it clear that the calls need
> to be paired.  That seems obvious now but wasn't before from just
> the function names.  So I'm wondering if we might want to rename
> them to make it more obvious that the former involves allocating
> memory that must be explicitly deallocated.
> 
> If you agree, names along the following lines would make this
> clearer (to me, anyway) but I'm open to others:
> 
>    gimple_ranger *set_new_ranger (function *);
>    void release_ranger (function *);


Could an RAII class help here, to make the aquire/release pairing more
automatic?

Dave

Re: [PATCH] document enable/disable_ranger

[Andrew MacLeod]

On 8/19/21 1:30 PM, Martin Sebor wrote:
> Hey Aldy & Andrew,
>
> I introduced a leak by calling enable_ranger() without pairing it
> with one to disable_ranger() on the same function (PR 101984).
> I didn't realize (or look to see) that enable_ranger() dynamically
> allocates memory.
>
> The patch below adds comments to make it clear that the calls need
> to be paired.  That seems obvious now but wasn't before from just
> the function names.  So I'm wondering if we might want to rename
> them to make it more obvious that the former involves allocating
> memory that must be explicitly deallocated.
>
> If you agree, names along the following lines would make this
> clearer (to me, anyway) but I'm open to others:
>
>   gimple_ranger *set_new_ranger (function *);
>   void release_ranger (function *); 


I think the missing comments alone are enough for now.

OK.

Andrew

Re: [PATCH] document enable/disable_ranger

[Andrew MacLeod]

On 8/19/21 2:00 PM, David Malcolm wrote:
> On Thu, 2021-08-19 at 11:30 -0600, Martin Sebor via Gcc-patches wrote:
>> Hey Aldy & Andrew,
>>
>> I introduced a leak by calling enable_ranger() without pairing it
>> with one to disable_ranger() on the same function (PR 101984).
>> I didn't realize (or look to see) that enable_ranger() dynamically
>> allocates memory.
>>
>> The patch below adds comments to make it clear that the calls need
>> to be paired.  That seems obvious now but wasn't before from just
>> the function names.  So I'm wondering if we might want to rename
>> them to make it more obvious that the former involves allocating
>> memory that must be explicitly deallocated.
>>
>> If you agree, names along the following lines would make this
>> clearer (to me, anyway) but I'm open to others:
>>
>>     gimple_ranger *set_new_ranger (function *);
>>     void release_ranger (function *);
>
> Could an RAII class help here, to make the aquire/release pairing more
> automatic?
>
> Dave

Well, we discussed making it a a pass property, among other things,  but 
there are also use cases we can imagine where we want to be able to have 
some alternate control.

I think for the moment its not overly difficult to create a ranger when 
your pass is ready for it, and then dispose of it when you are done.

Andrew