[PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Richard Biener]

The following fixes ranger bitmap allocation when invoked from IPA
context where the global bitmap obstack possibly isn't initialized.
Instead of trying to use one of the ranger obstacks the following
simply initializes the global bitmap obstack around an active ranger.

Bootstrapped and tested on x86_64-unknown-linux-gnu with bitmap_alloc
instrumentation (all ICEs gone, so ranger was the only offender).

OK?

Thanks,
Richard.

	PR middle-end/114604
	* gimple-range.cc (enable_ranger): Initialize the global
	bitmap obstack.
	(disable_ranger): Release it.
---
 gcc/gimple-range.cc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
index c16b776c1e3..4d3b1ce8588 100644
--- a/gcc/gimple-range.cc
+++ b/gcc/gimple-range.cc
@@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
 {
   gimple_ranger *r;
 
+  bitmap_obstack_initialize (NULL);
+
   gcc_checking_assert (!fun->x_range_query);
   r = new gimple_ranger (use_imm_uses);
   fun->x_range_query = r;
@@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
   gcc_checking_assert (fun->x_range_query);
   delete fun->x_range_query;
   fun->x_range_query = NULL;
+
+  bitmap_obstack_release (NULL);
 }
 
 // ------------------------------------------------------------------------
-- 
2.35.3

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 5180 bytes --]

On Tue, 9 Apr 2024, Aldy Hernandez wrote:

> BTW, I'm not opposed to this patch.  Thank you for tracking this down,
> and feel free to commit as is if y'all PMs agree it's OK.  I just
> wanted to know if there's a better way going forward.  I can certainly
> put it on my TODO list once stage1 opens again.
> 
> And no, there probably isn't an obstack for those classes, but I
> wonder if we should have a class local one, as we do for the rest of
> the classes.

OK, I pushed it now, it looks like the GCC 13 branch isn't affected
in obviously the same way (but I didn't try instrumenting there).

Feel free to improve next stage1.

Richard.

> Aldy
> 
> On Mon, Apr 8, 2024 at 7:47 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> >
> >
> > > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>:
> > >
> > > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote:
> > >>
> > >>
> > >>
> > >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>:
> > >>>
> > >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
> > >>>>
> > >>>> On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
> > >>>>>>       PR middle-end/114604
> > >>>>>>       * gimple-range.cc (enable_ranger): Initialize the global
> > >>>>>>       bitmap obstack.
> > >>>>>>       (disable_ranger): Release it.
> > >>>>>> ---
> > >>>>>> gcc/gimple-range.cc | 4 ++++
> > >>>>>> 1 file changed, 4 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> > >>>>>> index c16b776c1e3..4d3b1ce8588 100644
> > >>>>>> --- a/gcc/gimple-range.cc
> > >>>>>> +++ b/gcc/gimple-range.cc
> > >>>>>> @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
> > >>>>>> {
> > >>>>>>  gimple_ranger *r;
> > >>>>>>
> > >>>>>> +  bitmap_obstack_initialize (NULL);
> > >>>>>> +
> > >>>>>>  gcc_checking_assert (!fun->x_range_query);
> > >>>>>>  r = new gimple_ranger (use_imm_uses);
> > >>>>>>  fun->x_range_query = r;
> > >>>>>> @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
> > >>>>>>  gcc_checking_assert (fun->x_range_query);
> > >>>>>>  delete fun->x_range_query;
> > >>>>>>  fun->x_range_query = NULL;
> > >>>>>> +
> > >>>>>> +  bitmap_obstack_release (NULL);
> > >>>>>
> > >>>>> Are you not allowed to initialize/use obstacks unless
> > >>>>> bitmap_obstack_initialize(NULL) is called?
> > >>>>
> > >>>> You can use it with some other obstack, just not the default one.
> > >>>>
> > >>>>> If so, wouldn't it be
> > >>>>> better to lazily initialize it downstream (bitmap_alloc, or whomever
> > >>>>> needs it initialized)?
> > >>>>
> > >>>> No, you still need to decide where is the safe point to release it.
> > >>>> Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
> > >>>> the default one can nest (has associated nesting counter).  So, the above
> > >>>> patch just says that ranger starts using the default obstack in
> > >>>> enable_ranger and stops using it in disable_ranger and anything ranger
> > >>>> associated in the obstack can be freed at that point.
> > >>>
> > >>> I thought ranger never used the default one:
> > >>>
> > >>> $ grep bitmap_obstack_initialize *value* *range*
> > >>> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > >>> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > >>> gimple-range-cache.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > >>> gimple-range-gori.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > >>> gimple-range-infer.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > >>> gimple-range-phi.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > >>>
> > >>> or even:
> > >>>
> > >>> $ grep obstack.*NULL *value* *range*
> > >>> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> > >>> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> > >>> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> > >>> gimple-range-infer.cc:  obstack_free (&m_list_obstack, NULL);
> > >>> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> > >>>
> > >>> I'm obviously missing something here.
> > >>
> > >> Look for BITMAP_ALLOC (NULL) in the backtrace in the PR
> > >
> > > Ahh!  Thanks.
> > >
> > > A few default obstack uses snuck in while I wasn't looking.
> > >
> > > $ grep BITMAP_ALLOC.*NULL *range*
> > > gimple-range-cache.cc:  m_propfail = BITMAP_ALLOC (NULL);
> > > gimple-range-cache.h:  inline ssa_lazy_cache () { active_p =
> > > BITMAP_ALLOC (NULL); }
> > > gimple-range.cc:  m_pop_list = BITMAP_ALLOC (NULL);
> > >
> > > I wonder if it would be cleaner to just change these to use named obstacks.
> >
> > I didn’t find any obvious obstack to use, but sure.  This was the easiest fix ;)
> >
> > Richard
> >
> > > Andrew, is there a reason we were using the default obstack for these?
> > > For reference, they are  class update_list used in the ranger cache,
> > > ssa_lazy_cache, and dom_ranger.
> > >
> > > Aldy
> > >
> >
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Aldy Hernandez]

BTW, I'm not opposed to this patch.  Thank you for tracking this down,
and feel free to commit as is if y'all PMs agree it's OK.  I just
wanted to know if there's a better way going forward.  I can certainly
put it on my TODO list once stage1 opens again.

And no, there probably isn't an obstack for those classes, but I
wonder if we should have a class local one, as we do for the rest of
the classes.

Aldy

On Mon, Apr 8, 2024 at 7:47 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
>
>
> > Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>:
> >
> > On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote:
> >>
> >>
> >>
> >>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>:
> >>>
> >>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >>>>
> >>>> On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
> >>>>>>       PR middle-end/114604
> >>>>>>       * gimple-range.cc (enable_ranger): Initialize the global
> >>>>>>       bitmap obstack.
> >>>>>>       (disable_ranger): Release it.
> >>>>>> ---
> >>>>>> gcc/gimple-range.cc | 4 ++++
> >>>>>> 1 file changed, 4 insertions(+)
> >>>>>>
> >>>>>> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> >>>>>> index c16b776c1e3..4d3b1ce8588 100644
> >>>>>> --- a/gcc/gimple-range.cc
> >>>>>> +++ b/gcc/gimple-range.cc
> >>>>>> @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
> >>>>>> {
> >>>>>>  gimple_ranger *r;
> >>>>>>
> >>>>>> +  bitmap_obstack_initialize (NULL);
> >>>>>> +
> >>>>>>  gcc_checking_assert (!fun->x_range_query);
> >>>>>>  r = new gimple_ranger (use_imm_uses);
> >>>>>>  fun->x_range_query = r;
> >>>>>> @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
> >>>>>>  gcc_checking_assert (fun->x_range_query);
> >>>>>>  delete fun->x_range_query;
> >>>>>>  fun->x_range_query = NULL;
> >>>>>> +
> >>>>>> +  bitmap_obstack_release (NULL);
> >>>>>
> >>>>> Are you not allowed to initialize/use obstacks unless
> >>>>> bitmap_obstack_initialize(NULL) is called?
> >>>>
> >>>> You can use it with some other obstack, just not the default one.
> >>>>
> >>>>> If so, wouldn't it be
> >>>>> better to lazily initialize it downstream (bitmap_alloc, or whomever
> >>>>> needs it initialized)?
> >>>>
> >>>> No, you still need to decide where is the safe point to release it.
> >>>> Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
> >>>> the default one can nest (has associated nesting counter).  So, the above
> >>>> patch just says that ranger starts using the default obstack in
> >>>> enable_ranger and stops using it in disable_ranger and anything ranger
> >>>> associated in the obstack can be freed at that point.
> >>>
> >>> I thought ranger never used the default one:
> >>>
> >>> $ grep bitmap_obstack_initialize *value* *range*
> >>> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >>> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >>> gimple-range-cache.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >>> gimple-range-gori.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >>> gimple-range-infer.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >>> gimple-range-phi.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >>>
> >>> or even:
> >>>
> >>> $ grep obstack.*NULL *value* *range*
> >>> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> >>> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> >>> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> >>> gimple-range-infer.cc:  obstack_free (&m_list_obstack, NULL);
> >>> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> >>>
> >>> I'm obviously missing something here.
> >>
> >> Look for BITMAP_ALLOC (NULL) in the backtrace in the PR
> >
> > Ahh!  Thanks.
> >
> > A few default obstack uses snuck in while I wasn't looking.
> >
> > $ grep BITMAP_ALLOC.*NULL *range*
> > gimple-range-cache.cc:  m_propfail = BITMAP_ALLOC (NULL);
> > gimple-range-cache.h:  inline ssa_lazy_cache () { active_p =
> > BITMAP_ALLOC (NULL); }
> > gimple-range.cc:  m_pop_list = BITMAP_ALLOC (NULL);
> >
> > I wonder if it would be cleaner to just change these to use named obstacks.
>
> I didn’t find any obvious obstack to use, but sure.  This was the easiest fix ;)
>
> Richard
>
> > Andrew, is there a reason we were using the default obstack for these?
> > For reference, they are  class update_list used in the ranger cache,
> > ssa_lazy_cache, and dom_ranger.
> >
> > Aldy
> >
>

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Richard Biener]

> Am 08.04.2024 um 18:40 schrieb Aldy Hernandez <aldyh@redhat.com>:
> 
> On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote:
>> 
>> 
>> 
>>>> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>:
>>> 
>>> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>>> 
>>>> On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
>>>>>>       PR middle-end/114604
>>>>>>       * gimple-range.cc (enable_ranger): Initialize the global
>>>>>>       bitmap obstack.
>>>>>>       (disable_ranger): Release it.
>>>>>> ---
>>>>>> gcc/gimple-range.cc | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>> 
>>>>>> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
>>>>>> index c16b776c1e3..4d3b1ce8588 100644
>>>>>> --- a/gcc/gimple-range.cc
>>>>>> +++ b/gcc/gimple-range.cc
>>>>>> @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
>>>>>> {
>>>>>>  gimple_ranger *r;
>>>>>> 
>>>>>> +  bitmap_obstack_initialize (NULL);
>>>>>> +
>>>>>>  gcc_checking_assert (!fun->x_range_query);
>>>>>>  r = new gimple_ranger (use_imm_uses);
>>>>>>  fun->x_range_query = r;
>>>>>> @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
>>>>>>  gcc_checking_assert (fun->x_range_query);
>>>>>>  delete fun->x_range_query;
>>>>>>  fun->x_range_query = NULL;
>>>>>> +
>>>>>> +  bitmap_obstack_release (NULL);
>>>>> 
>>>>> Are you not allowed to initialize/use obstacks unless
>>>>> bitmap_obstack_initialize(NULL) is called?
>>>> 
>>>> You can use it with some other obstack, just not the default one.
>>>> 
>>>>> If so, wouldn't it be
>>>>> better to lazily initialize it downstream (bitmap_alloc, or whomever
>>>>> needs it initialized)?
>>>> 
>>>> No, you still need to decide where is the safe point to release it.
>>>> Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
>>>> the default one can nest (has associated nesting counter).  So, the above
>>>> patch just says that ranger starts using the default obstack in
>>>> enable_ranger and stops using it in disable_ranger and anything ranger
>>>> associated in the obstack can be freed at that point.
>>> 
>>> I thought ranger never used the default one:
>>> 
>>> $ grep bitmap_obstack_initialize *value* *range*
>>> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
>>> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
>>> gimple-range-cache.cc:  bitmap_obstack_initialize (&m_bitmaps);
>>> gimple-range-gori.cc:  bitmap_obstack_initialize (&m_bitmaps);
>>> gimple-range-infer.cc:  bitmap_obstack_initialize (&m_bitmaps);
>>> gimple-range-phi.cc:  bitmap_obstack_initialize (&m_bitmaps);
>>> 
>>> or even:
>>> 
>>> $ grep obstack.*NULL *value* *range*
>>> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
>>> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
>>> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
>>> gimple-range-infer.cc:  obstack_free (&m_list_obstack, NULL);
>>> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
>>> 
>>> I'm obviously missing something here.
>> 
>> Look for BITMAP_ALLOC (NULL) in the backtrace in the PR
> 
> Ahh!  Thanks.
> 
> A few default obstack uses snuck in while I wasn't looking.
> 
> $ grep BITMAP_ALLOC.*NULL *range*
> gimple-range-cache.cc:  m_propfail = BITMAP_ALLOC (NULL);
> gimple-range-cache.h:  inline ssa_lazy_cache () { active_p =
> BITMAP_ALLOC (NULL); }
> gimple-range.cc:  m_pop_list = BITMAP_ALLOC (NULL);
> 
> I wonder if it would be cleaner to just change these to use named obstacks.

I didn’t find any obvious obstack to use, but sure.  This was the easiest fix ;)

Richard 

> Andrew, is there a reason we were using the default obstack for these?
> For reference, they are  class update_list used in the ranger cache,
> ssa_lazy_cache, and dom_ranger.
> 
> Aldy
> 

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Aldy Hernandez]

On Mon, Apr 8, 2024 at 6:29 PM Richard Biener <rguenther@suse.de> wrote:
>
>
>
> > Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>:
> >
> > On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >>
> >> On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
> >>>>        PR middle-end/114604
> >>>>        * gimple-range.cc (enable_ranger): Initialize the global
> >>>>        bitmap obstack.
> >>>>        (disable_ranger): Release it.
> >>>> ---
> >>>> gcc/gimple-range.cc | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> >>>> index c16b776c1e3..4d3b1ce8588 100644
> >>>> --- a/gcc/gimple-range.cc
> >>>> +++ b/gcc/gimple-range.cc
> >>>> @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
> >>>> {
> >>>>   gimple_ranger *r;
> >>>>
> >>>> +  bitmap_obstack_initialize (NULL);
> >>>> +
> >>>>   gcc_checking_assert (!fun->x_range_query);
> >>>>   r = new gimple_ranger (use_imm_uses);
> >>>>   fun->x_range_query = r;
> >>>> @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
> >>>>   gcc_checking_assert (fun->x_range_query);
> >>>>   delete fun->x_range_query;
> >>>>   fun->x_range_query = NULL;
> >>>> +
> >>>> +  bitmap_obstack_release (NULL);
> >>>
> >>> Are you not allowed to initialize/use obstacks unless
> >>> bitmap_obstack_initialize(NULL) is called?
> >>
> >> You can use it with some other obstack, just not the default one.
> >>
> >>> If so, wouldn't it be
> >>> better to lazily initialize it downstream (bitmap_alloc, or whomever
> >>> needs it initialized)?
> >>
> >> No, you still need to decide where is the safe point to release it.
> >> Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
> >> the default one can nest (has associated nesting counter).  So, the above
> >> patch just says that ranger starts using the default obstack in
> >> enable_ranger and stops using it in disable_ranger and anything ranger
> >> associated in the obstack can be freed at that point.
> >
> > I thought ranger never used the default one:
> >
> > $ grep bitmap_obstack_initialize *value* *range*
> > value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > gimple-range-cache.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > gimple-range-gori.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > gimple-range-infer.cc:  bitmap_obstack_initialize (&m_bitmaps);
> > gimple-range-phi.cc:  bitmap_obstack_initialize (&m_bitmaps);
> >
> > or even:
> >
> > $ grep obstack.*NULL *value* *range*
> > value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> > value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> > value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> > gimple-range-infer.cc:  obstack_free (&m_list_obstack, NULL);
> > value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> >
> > I'm obviously missing something here.
>
> Look for BITMAP_ALLOC (NULL) in the backtrace in the PR

Ahh!  Thanks.

A few default obstack uses snuck in while I wasn't looking.

$ grep BITMAP_ALLOC.*NULL *range*
gimple-range-cache.cc:  m_propfail = BITMAP_ALLOC (NULL);
gimple-range-cache.h:  inline ssa_lazy_cache () { active_p =
BITMAP_ALLOC (NULL); }
gimple-range.cc:  m_pop_list = BITMAP_ALLOC (NULL);

I wonder if it would be cleaner to just change these to use named obstacks.

Andrew, is there a reason we were using the default obstack for these?
 For reference, they are  class update_list used in the ranger cache,
ssa_lazy_cache, and dom_ranger.

Aldy

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Richard Biener]

> Am 08.04.2024 um 18:09 schrieb Aldy Hernandez <aldyh@redhat.com>:
> 
> On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
>> 
>> On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
>>>>        PR middle-end/114604
>>>>        * gimple-range.cc (enable_ranger): Initialize the global
>>>>        bitmap obstack.
>>>>        (disable_ranger): Release it.
>>>> ---
>>>> gcc/gimple-range.cc | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
>>>> index c16b776c1e3..4d3b1ce8588 100644
>>>> --- a/gcc/gimple-range.cc
>>>> +++ b/gcc/gimple-range.cc
>>>> @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
>>>> {
>>>>   gimple_ranger *r;
>>>> 
>>>> +  bitmap_obstack_initialize (NULL);
>>>> +
>>>>   gcc_checking_assert (!fun->x_range_query);
>>>>   r = new gimple_ranger (use_imm_uses);
>>>>   fun->x_range_query = r;
>>>> @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
>>>>   gcc_checking_assert (fun->x_range_query);
>>>>   delete fun->x_range_query;
>>>>   fun->x_range_query = NULL;
>>>> +
>>>> +  bitmap_obstack_release (NULL);
>>> 
>>> Are you not allowed to initialize/use obstacks unless
>>> bitmap_obstack_initialize(NULL) is called?
>> 
>> You can use it with some other obstack, just not the default one.
>> 
>>> If so, wouldn't it be
>>> better to lazily initialize it downstream (bitmap_alloc, or whomever
>>> needs it initialized)?
>> 
>> No, you still need to decide where is the safe point to release it.
>> Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
>> the default one can nest (has associated nesting counter).  So, the above
>> patch just says that ranger starts using the default obstack in
>> enable_ranger and stops using it in disable_ranger and anything ranger
>> associated in the obstack can be freed at that point.
> 
> I thought ranger never used the default one:
> 
> $ grep bitmap_obstack_initialize *value* *range*
> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
> gimple-range-cache.cc:  bitmap_obstack_initialize (&m_bitmaps);
> gimple-range-gori.cc:  bitmap_obstack_initialize (&m_bitmaps);
> gimple-range-infer.cc:  bitmap_obstack_initialize (&m_bitmaps);
> gimple-range-phi.cc:  bitmap_obstack_initialize (&m_bitmaps);
> 
> or even:
> 
> $ grep obstack.*NULL *value* *range*
> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
> gimple-range-infer.cc:  obstack_free (&m_list_obstack, NULL);
> value-range-storage.cc:    obstack_free (&m_obstack, NULL);
> 
> I'm obviously missing something here.

Look for BITMAP_ALLOC (NULL) in the backtrace in the PR

Richard 


> Aldy
> 

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Aldy Hernandez]

On Mon, Apr 8, 2024 at 5:54 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
> > >         PR middle-end/114604
> > >         * gimple-range.cc (enable_ranger): Initialize the global
> > >         bitmap obstack.
> > >         (disable_ranger): Release it.
> > > ---
> > >  gcc/gimple-range.cc | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> > > index c16b776c1e3..4d3b1ce8588 100644
> > > --- a/gcc/gimple-range.cc
> > > +++ b/gcc/gimple-range.cc
> > > @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
> > >  {
> > >    gimple_ranger *r;
> > >
> > > +  bitmap_obstack_initialize (NULL);
> > > +
> > >    gcc_checking_assert (!fun->x_range_query);
> > >    r = new gimple_ranger (use_imm_uses);
> > >    fun->x_range_query = r;
> > > @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
> > >    gcc_checking_assert (fun->x_range_query);
> > >    delete fun->x_range_query;
> > >    fun->x_range_query = NULL;
> > > +
> > > +  bitmap_obstack_release (NULL);
> >
> > Are you not allowed to initialize/use obstacks unless
> > bitmap_obstack_initialize(NULL) is called?
>
> You can use it with some other obstack, just not the default one.
>
> > If so, wouldn't it be
> > better to lazily initialize it downstream (bitmap_alloc, or whomever
> > needs it initialized)?
>
> No, you still need to decide where is the safe point to release it.
> Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
> the default one can nest (has associated nesting counter).  So, the above
> patch just says that ranger starts using the default obstack in
> enable_ranger and stops using it in disable_ranger and anything ranger
> associated in the obstack can be freed at that point.

I thought ranger never used the default one:

$ grep bitmap_obstack_initialize *value* *range*
value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
value-relation.cc:  bitmap_obstack_initialize (&m_bitmaps);
gimple-range-cache.cc:  bitmap_obstack_initialize (&m_bitmaps);
gimple-range-gori.cc:  bitmap_obstack_initialize (&m_bitmaps);
gimple-range-infer.cc:  bitmap_obstack_initialize (&m_bitmaps);
gimple-range-phi.cc:  bitmap_obstack_initialize (&m_bitmaps);

or even:

$ grep obstack.*NULL *value* *range*
value-range-storage.cc:    obstack_free (&m_obstack, NULL);
value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
value-relation.cc:  obstack_free (&m_chain_obstack, NULL);
gimple-range-infer.cc:  obstack_free (&m_list_obstack, NULL);
value-range-storage.cc:    obstack_free (&m_obstack, NULL);

I'm obviously missing something here.

Aldy

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Jakub Jelinek]

On Mon, Apr 08, 2024 at 05:40:23PM +0200, Aldy Hernandez wrote:
> >         PR middle-end/114604
> >         * gimple-range.cc (enable_ranger): Initialize the global
> >         bitmap obstack.
> >         (disable_ranger): Release it.
> > ---
> >  gcc/gimple-range.cc | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> > index c16b776c1e3..4d3b1ce8588 100644
> > --- a/gcc/gimple-range.cc
> > +++ b/gcc/gimple-range.cc
> > @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
> >  {
> >    gimple_ranger *r;
> >
> > +  bitmap_obstack_initialize (NULL);
> > +
> >    gcc_checking_assert (!fun->x_range_query);
> >    r = new gimple_ranger (use_imm_uses);
> >    fun->x_range_query = r;
> > @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
> >    gcc_checking_assert (fun->x_range_query);
> >    delete fun->x_range_query;
> >    fun->x_range_query = NULL;
> > +
> > +  bitmap_obstack_release (NULL);
> 
> Are you not allowed to initialize/use obstacks unless
> bitmap_obstack_initialize(NULL) is called?

You can use it with some other obstack, just not the default one.

> If so, wouldn't it be
> better to lazily initialize it downstream (bitmap_alloc, or whomever
> needs it initialized)?

No, you still need to decide where is the safe point to release it.
Unlike the non-default bitmap_obstack_initialize/bitmap_obstack_release,
the default one can nest (has associated nesting counter).  So, the above
patch just says that ranger starts using the default obstack in
enable_ranger and stops using it in disable_ranger and anything ranger
associated in the obstack can be freed at that point.
If one uses this from a pass which has its own
bitmap_obstack_{initializer,release} (NULL) around this, then the above will
not do anything, just bump the nesting counter and decrease it later.
But if some pass doesn't, i.e. the pass itself doesn't use the default
obstack, only ranger does, then the above seems like the right change to me.

> Unless I'm misunderstanding something, it
> doesn't seem like ranger is the correct place to fix this.

	Jakub

Re: [PATCH] middle-end/114604 - ranger allocates bitmap without initialized obstack

[Aldy Hernandez]

On Mon, Apr 8, 2024 at 11:50 AM Richard Biener <rguenther@suse.de> wrote:
>
> The following fixes ranger bitmap allocation when invoked from IPA
> context where the global bitmap obstack possibly isn't initialized.
> Instead of trying to use one of the ranger obstacks the following
> simply initializes the global bitmap obstack around an active ranger.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu with bitmap_alloc
> instrumentation (all ICEs gone, so ranger was the only offender).
>
> OK?
>
> Thanks,
> Richard.
>
>         PR middle-end/114604
>         * gimple-range.cc (enable_ranger): Initialize the global
>         bitmap obstack.
>         (disable_ranger): Release it.
> ---
>  gcc/gimple-range.cc | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc
> index c16b776c1e3..4d3b1ce8588 100644
> --- a/gcc/gimple-range.cc
> +++ b/gcc/gimple-range.cc
> @@ -689,6 +689,8 @@ enable_ranger (struct function *fun, bool use_imm_uses)
>  {
>    gimple_ranger *r;
>
> +  bitmap_obstack_initialize (NULL);
> +
>    gcc_checking_assert (!fun->x_range_query);
>    r = new gimple_ranger (use_imm_uses);
>    fun->x_range_query = r;
> @@ -705,6 +707,8 @@ disable_ranger (struct function *fun)
>    gcc_checking_assert (fun->x_range_query);
>    delete fun->x_range_query;
>    fun->x_range_query = NULL;
> +
> +  bitmap_obstack_release (NULL);

Are you not allowed to initialize/use obstacks unless
bitmap_obstack_initialize(NULL) is called?  If so, wouldn't it be
better to lazily initialize it downstream (bitmap_alloc, or whomever
needs it initialized)?  Unless I'm misunderstanding something, it
doesn't seem like ranger is the correct place to fix this.

Aldy