[PATCH] Fix IPA modref ubsan.

[PATCH] Fix IPA modref ubsan.

[Martin Liška]

modref_tree<tree_node*>::merge(modref_tree<tree_node*>*, vec<modref_parm_map, va_heap, vl_ptr>*, modref_parm_map*, bool)

is called with modref_parm_map chain_map;

The variable has uninitialized m.parm_offset_known and it is accessed
here:

gcc/ipa-modref-tree.h:572 a.parm_offset_known &= m.parm_offset_known;

Ready to be installed after testing?
Thanks,
Martin

	PR ipa/103230

gcc/ChangeLog:

	* ipa-modref-tree.h (struct modref_parm_map): Add default
	constructor.
	* ipa-modref.c (ipa_merge_modref_summary_after_inlining): Use it.
---
  gcc/ipa-modref-tree.h | 5 +++++
  gcc/ipa-modref.c      | 3 +--
  2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
index 0a097349ebd..6796e6ecc34 100644
--- a/gcc/ipa-modref-tree.h
+++ b/gcc/ipa-modref-tree.h
@@ -287,6 +287,11 @@ struct GTY((user)) modref_base_node
  
  struct modref_parm_map
  {
+  /* Default constructor.  */
+  modref_parm_map ()
+  : parm_index (MODREF_UNKNOWN_PARM), parm_offset_known (false), parm_offset ()
+  {}
+
    /* Index of parameter we translate to.
       Values from special_params enum are permitted too.  */
    int parm_index;
diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
index c94f0589d44..630d202d5cf 100644
--- a/gcc/ipa-modref.c
+++ b/gcc/ipa-modref.c
@@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
        auto_vec <modref_parm_map, 32> parm_map;
        modref_parm_map chain_map;
        /* TODO: Once we get jump functions for static chains we could
-	 compute this.  */
-      chain_map.parm_index = MODREF_UNKNOWN_PARM;
+	 compute parm_index.  */
  
        compute_parm_map (edge, &parm_map);
  
-- 
2.33.1

Re: [PATCH] Fix IPA modref ubsan.

[Jan Hubicka]

> modref_tree<tree_node*>::merge(modref_tree<tree_node*>*, vec<modref_parm_map, va_heap, vl_ptr>*, modref_parm_map*, bool)
> 
> is called with modref_parm_map chain_map;
> 
> The variable has uninitialized m.parm_offset_known and it is accessed
> here:
> 
> gcc/ipa-modref-tree.h:572 a.parm_offset_known &= m.parm_offset_known;
> 
> Ready to be installed after testing?
> Thanks,
> Martin
> 
> 	PR ipa/103230
> 
> gcc/ChangeLog:
> 
> 	* ipa-modref-tree.h (struct modref_parm_map): Add default
> 	constructor.
> 	* ipa-modref.c (ipa_merge_modref_summary_after_inlining): Use it.
> ---
>  gcc/ipa-modref-tree.h | 5 +++++
>  gcc/ipa-modref.c      | 3 +--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
> index 0a097349ebd..6796e6ecc34 100644
> --- a/gcc/ipa-modref-tree.h
> +++ b/gcc/ipa-modref-tree.h
> @@ -287,6 +287,11 @@ struct GTY((user)) modref_base_node
>  struct modref_parm_map
>  {
> +  /* Default constructor.  */
> +  modref_parm_map ()
> +  : parm_index (MODREF_UNKNOWN_PARM), parm_offset_known (false), parm_offset ()
> +  {}
> +
I think we are generally not supposed to put non-pods to vec<..>
The diagnostics should be from
			a.parm_offset_known &= m.parm_offset_known;
Becasue both in the parm_map (which is variable m) and access_node
(which is variable a) the parm_offset_known has no meaning when
parm_index == MODREF_UNKNOWN_PARM.

If we want to avoid computing on these, perhaps this will work?

diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
index 0a097349ebd..97736d0d8a4 100644
--- a/gcc/ipa-modref-tree.h
+++ b/gcc/ipa-modref-tree.h
@@ -568,9 +568,13 @@ struct GTY((user)) modref_tree
 				      : (*parm_map) [a.parm_index];
 			    if (m.parm_index == MODREF_LOCAL_MEMORY_PARM)
 			      continue;
-			    a.parm_offset += m.parm_offset;
-			    a.parm_offset_known &= m.parm_offset_known;
 			    a.parm_index = m.parm_index;
+			    if (a.parm_index != MODREF_UNKNOWN_PARM)
+			      {
+				a.parm_offset_known &= m.parm_offset_known;
+				if (a.parm_offset_known)
+				  a.parm_offset += m.parm_offset;
+			      }
 			  }
 		      }
 		    changed |= insert (base_node->base, ref_node->ref, a,
>    /* Index of parameter we translate to.
>       Values from special_params enum are permitted too.  */
>    int parm_index;
> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
> index c94f0589d44..630d202d5cf 100644
> --- a/gcc/ipa-modref.c
> +++ b/gcc/ipa-modref.c
> @@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
>        auto_vec <modref_parm_map, 32> parm_map;
>        modref_parm_map chain_map;
>        /* TODO: Once we get jump functions for static chains we could
> -	 compute this.  */
> -      chain_map.parm_index = MODREF_UNKNOWN_PARM;
> +	 compute parm_index.  */
>        compute_parm_map (edge, &parm_map);
> -- 
> 2.33.1
> 

Re: [PATCH] Fix IPA modref ubsan.

[Martin Sebor]

On 11/18/21 5:41 AM, Jan Hubicka via Gcc-patches wrote:
>> modref_tree<tree_node*>::merge(modref_tree<tree_node*>*, vec<modref_parm_map, va_heap, vl_ptr>*, modref_parm_map*, bool)
>>
>> is called with modref_parm_map chain_map;
>>
>> The variable has uninitialized m.parm_offset_known and it is accessed
>> here:
>>
>> gcc/ipa-modref-tree.h:572 a.parm_offset_known &= m.parm_offset_known;
>>
>> Ready to be installed after testing?
>> Thanks,
>> Martin
>>
>> 	PR ipa/103230
>>
>> gcc/ChangeLog:
>>
>> 	* ipa-modref-tree.h (struct modref_parm_map): Add default
>> 	constructor.
>> 	* ipa-modref.c (ipa_merge_modref_summary_after_inlining): Use it.
>> ---
>>   gcc/ipa-modref-tree.h | 5 +++++
>>   gcc/ipa-modref.c      | 3 +--
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
>> index 0a097349ebd..6796e6ecc34 100644
>> --- a/gcc/ipa-modref-tree.h
>> +++ b/gcc/ipa-modref-tree.h
>> @@ -287,6 +287,11 @@ struct GTY((user)) modref_base_node
>>   struct modref_parm_map
>>   {
>> +  /* Default constructor.  */
>> +  modref_parm_map ()
>> +  : parm_index (MODREF_UNKNOWN_PARM), parm_offset_known (false), parm_offset ()
>> +  {}
>> +
> I think we are generally not supposed to put non-pods to vec<..>

I don't know what the guidance is on using vec in IPA passes
but with respect to existing practice elsewhere, there are
existing uses of vec and auto_vec with non-POD types and vec
does work with them  (see the vec_default_construct and
vec_copy_construct templates, for example, whose goal is
to support nontrivial classes).

Martin

> The diagnostics should be from
> 			a.parm_offset_known &= m.parm_offset_known;
> Becasue both in the parm_map (which is variable m) and access_node
> (which is variable a) the parm_offset_known has no meaning when
> parm_index == MODREF_UNKNOWN_PARM.
> 
> If we want to avoid computing on these, perhaps this will work?
> 
> diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
> index 0a097349ebd..97736d0d8a4 100644
> --- a/gcc/ipa-modref-tree.h
> +++ b/gcc/ipa-modref-tree.h
> @@ -568,9 +568,13 @@ struct GTY((user)) modref_tree
>   				      : (*parm_map) [a.parm_index];
>   			    if (m.parm_index == MODREF_LOCAL_MEMORY_PARM)
>   			      continue;
> -			    a.parm_offset += m.parm_offset;
> -			    a.parm_offset_known &= m.parm_offset_known;
>   			    a.parm_index = m.parm_index;
> +			    if (a.parm_index != MODREF_UNKNOWN_PARM)
> +			      {
> +				a.parm_offset_known &= m.parm_offset_known;
> +				if (a.parm_offset_known)
> +				  a.parm_offset += m.parm_offset;
> +			      }
>   			  }
>   		      }
>   		    changed |= insert (base_node->base, ref_node->ref, a,
>>     /* Index of parameter we translate to.
>>        Values from special_params enum are permitted too.  */
>>     int parm_index;
>> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
>> index c94f0589d44..630d202d5cf 100644
>> --- a/gcc/ipa-modref.c
>> +++ b/gcc/ipa-modref.c
>> @@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
>>         auto_vec <modref_parm_map, 32> parm_map;
>>         modref_parm_map chain_map;
>>         /* TODO: Once we get jump functions for static chains we could
>> -	 compute this.  */
>> -      chain_map.parm_index = MODREF_UNKNOWN_PARM;
>> +	 compute parm_index.  */
>>         compute_parm_map (edge, &parm_map);
>> -- 
>> 2.33.1
>>

Re: [PATCH] Fix IPA modref ubsan.

[Jan Hubicka]

> On 11/18/21 5:41 AM, Jan Hubicka via Gcc-patches wrote:
> > > modref_tree<tree_node*>::merge(modref_tree<tree_node*>*, vec<modref_parm_map, va_heap, vl_ptr>*, modref_parm_map*, bool)
> > > 
> > > is called with modref_parm_map chain_map;
> > > 
> > > The variable has uninitialized m.parm_offset_known and it is accessed
> > > here:
> > > 
> > > gcc/ipa-modref-tree.h:572 a.parm_offset_known &= m.parm_offset_known;
> > > 
> > > Ready to be installed after testing?
> > > Thanks,
> > > Martin
> > > 
> > > 	PR ipa/103230
> > > 
> > > gcc/ChangeLog:
> > > 
> > > 	* ipa-modref-tree.h (struct modref_parm_map): Add default
> > > 	constructor.
> > > 	* ipa-modref.c (ipa_merge_modref_summary_after_inlining): Use it.
> > > ---
> > >   gcc/ipa-modref-tree.h | 5 +++++
> > >   gcc/ipa-modref.c      | 3 +--
> > >   2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
> > > index 0a097349ebd..6796e6ecc34 100644
> > > --- a/gcc/ipa-modref-tree.h
> > > +++ b/gcc/ipa-modref-tree.h
> > > @@ -287,6 +287,11 @@ struct GTY((user)) modref_base_node
> > >   struct modref_parm_map
> > >   {
> > > +  /* Default constructor.  */
> > > +  modref_parm_map ()
> > > +  : parm_index (MODREF_UNKNOWN_PARM), parm_offset_known (false), parm_offset ()
> > > +  {}
> > > +
> > I think we are generally not supposed to put non-pods to vec<..>
> 
> I don't know what the guidance is on using vec in IPA passes
> but with respect to existing practice elsewhere, there are
> existing uses of vec and auto_vec with non-POD types and vec
> does work with them  (see the vec_default_construct and
> vec_copy_construct templates, for example, whose goal is
> to support nontrivial classes).

I see, since 2017 :).  The patch is OK then.
Nontrivial destructors also behave in a sane way these days?

Honza
> 
> Martin
> 
> > The diagnostics should be from
> > 			a.parm_offset_known &= m.parm_offset_known;
> > Becasue both in the parm_map (which is variable m) and access_node
> > (which is variable a) the parm_offset_known has no meaning when
> > parm_index == MODREF_UNKNOWN_PARM.
> > 
> > If we want to avoid computing on these, perhaps this will work?
> > 
> > diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
> > index 0a097349ebd..97736d0d8a4 100644
> > --- a/gcc/ipa-modref-tree.h
> > +++ b/gcc/ipa-modref-tree.h
> > @@ -568,9 +568,13 @@ struct GTY((user)) modref_tree
> >   				      : (*parm_map) [a.parm_index];
> >   			    if (m.parm_index == MODREF_LOCAL_MEMORY_PARM)
> >   			      continue;
> > -			    a.parm_offset += m.parm_offset;
> > -			    a.parm_offset_known &= m.parm_offset_known;
> >   			    a.parm_index = m.parm_index;
> > +			    if (a.parm_index != MODREF_UNKNOWN_PARM)
> > +			      {
> > +				a.parm_offset_known &= m.parm_offset_known;
> > +				if (a.parm_offset_known)
> > +				  a.parm_offset += m.parm_offset;
> > +			      }
> >   			  }
> >   		      }
> >   		    changed |= insert (base_node->base, ref_node->ref, a,
> > >     /* Index of parameter we translate to.
> > >        Values from special_params enum are permitted too.  */
> > >     int parm_index;
> > > diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
> > > index c94f0589d44..630d202d5cf 100644
> > > --- a/gcc/ipa-modref.c
> > > +++ b/gcc/ipa-modref.c
> > > @@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
> > >         auto_vec <modref_parm_map, 32> parm_map;
> > >         modref_parm_map chain_map;
> > >         /* TODO: Once we get jump functions for static chains we could
> > > -	 compute this.  */
> > > -      chain_map.parm_index = MODREF_UNKNOWN_PARM;
> > > +	 compute parm_index.  */
> > >         compute_parm_map (edge, &parm_map);
> > > -- 
> > > 2.33.1
> > > 
> 

Re: [PATCH] Fix IPA modref ubsan.

[Martin Sebor]

On 11/18/21 9:22 AM, Jan Hubicka wrote:
>> On 11/18/21 5:41 AM, Jan Hubicka via Gcc-patches wrote:
>>>> modref_tree<tree_node*>::merge(modref_tree<tree_node*>*, vec<modref_parm_map, va_heap, vl_ptr>*, modref_parm_map*, bool)
>>>>
>>>> is called with modref_parm_map chain_map;
>>>>
>>>> The variable has uninitialized m.parm_offset_known and it is accessed
>>>> here:
>>>>
>>>> gcc/ipa-modref-tree.h:572 a.parm_offset_known &= m.parm_offset_known;
>>>>
>>>> Ready to be installed after testing?
>>>> Thanks,
>>>> Martin
>>>>
>>>> 	PR ipa/103230
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 	* ipa-modref-tree.h (struct modref_parm_map): Add default
>>>> 	constructor.
>>>> 	* ipa-modref.c (ipa_merge_modref_summary_after_inlining): Use it.
>>>> ---
>>>>    gcc/ipa-modref-tree.h | 5 +++++
>>>>    gcc/ipa-modref.c      | 3 +--
>>>>    2 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
>>>> index 0a097349ebd..6796e6ecc34 100644
>>>> --- a/gcc/ipa-modref-tree.h
>>>> +++ b/gcc/ipa-modref-tree.h
>>>> @@ -287,6 +287,11 @@ struct GTY((user)) modref_base_node
>>>>    struct modref_parm_map
>>>>    {
>>>> +  /* Default constructor.  */
>>>> +  modref_parm_map ()
>>>> +  : parm_index (MODREF_UNKNOWN_PARM), parm_offset_known (false), parm_offset ()
>>>> +  {}
>>>> +
>>> I think we are generally not supposed to put non-pods to vec<..>
>>
>> I don't know what the guidance is on using vec in IPA passes
>> but with respect to existing practice elsewhere, there are
>> existing uses of vec and auto_vec with non-POD types and vec
>> does work with them  (see the vec_default_construct and
>> vec_copy_construct templates, for example, whose goal is
>> to support nontrivial classes).
> 
> I see, since 2017 :).  The patch is OK then.
> Nontrivial destructors also behave in a sane way these days?

Good question :)

At a minimum, element dtors should be automatically invoked by
the auto_vec dtor (there is an auto-test in vec.c to verify that).

Beyond that, since (unlike auto_vec) a plain vec isn't a container
its users are on their own when it comes to managing the memory of
their elements (i.e., they need to explicitly destroy their elements).

Having said that, as with all retrofits, they could be incomplete.
I see a few examples of where that seems to be the case here:

Calling truncate() on a vec with notrivial elements leaks, so
clients needs to explicitly release those elements.  That
should happen automatically.

Going through vec, I also see calls to memmove in functions
like quick_insert, ordered_remove, and block_remove.  So calling
those functions is not safe on a vec with nontrivial types.

Calling any of the sort functions also may not work correctly
with nontrivial elements (gcc_sort() calls memcpy).  vec should
either prevent that buy refusing to compile or use a safe
(generic) template for that.

So while basic vec uses work with nontrivial types, there are
plenty of bugs :(

Martin

> 
> Honza
>>
>> Martin
>>
>>> The diagnostics should be from
>>> 			a.parm_offset_known &= m.parm_offset_known;
>>> Becasue both in the parm_map (which is variable m) and access_node
>>> (which is variable a) the parm_offset_known has no meaning when
>>> parm_index == MODREF_UNKNOWN_PARM.
>>>
>>> If we want to avoid computing on these, perhaps this will work?
>>>
>>> diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
>>> index 0a097349ebd..97736d0d8a4 100644
>>> --- a/gcc/ipa-modref-tree.h
>>> +++ b/gcc/ipa-modref-tree.h
>>> @@ -568,9 +568,13 @@ struct GTY((user)) modref_tree
>>>    				      : (*parm_map) [a.parm_index];
>>>    			    if (m.parm_index == MODREF_LOCAL_MEMORY_PARM)
>>>    			      continue;
>>> -			    a.parm_offset += m.parm_offset;
>>> -			    a.parm_offset_known &= m.parm_offset_known;
>>>    			    a.parm_index = m.parm_index;
>>> +			    if (a.parm_index != MODREF_UNKNOWN_PARM)
>>> +			      {
>>> +				a.parm_offset_known &= m.parm_offset_known;
>>> +				if (a.parm_offset_known)
>>> +				  a.parm_offset += m.parm_offset;
>>> +			      }
>>>    			  }
>>>    		      }
>>>    		    changed |= insert (base_node->base, ref_node->ref, a,
>>>>      /* Index of parameter we translate to.
>>>>         Values from special_params enum are permitted too.  */
>>>>      int parm_index;
>>>> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
>>>> index c94f0589d44..630d202d5cf 100644
>>>> --- a/gcc/ipa-modref.c
>>>> +++ b/gcc/ipa-modref.c
>>>> @@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
>>>>          auto_vec <modref_parm_map, 32> parm_map;
>>>>          modref_parm_map chain_map;
>>>>          /* TODO: Once we get jump functions for static chains we could
>>>> -	 compute this.  */
>>>> -      chain_map.parm_index = MODREF_UNKNOWN_PARM;
>>>> +	 compute parm_index.  */
>>>>          compute_parm_map (edge, &parm_map);
>>>> -- 
>>>> 2.33.1
>>>>
>>

Re: [PATCH] Fix IPA modref ubsan.

[Jan Hubicka]

> > > I don't know what the guidance is on using vec in IPA passes
> > > but with respect to existing practice elsewhere, there are
> > > existing uses of vec and auto_vec with non-POD types and vec
> > > does work with them  (see the vec_default_construct and
> > > vec_copy_construct templates, for example, whose goal is
> > > to support nontrivial classes).
> > 
> > I see, since 2017 :).  The patch is OK then.
> > Nontrivial destructors also behave in a sane way these days?
> 
> Good question :)
> 
> At a minimum, element dtors should be automatically invoked by
> the auto_vec dtor (there is an auto-test in vec.c to verify that).
> 
> Beyond that, since (unlike auto_vec) a plain vec isn't a container
> its users are on their own when it comes to managing the memory of
> their elements (i.e., they need to explicitly destroy their elements).
> 
> Having said that, as with all retrofits, they could be incomplete.
> I see a few examples of where that seems to be the case here:
> 
> Calling truncate() on a vec with notrivial elements leaks, so
> clients needs to explicitly release those elements.  That
> should happen automatically.
> 
> Going through vec, I also see calls to memmove in functions
> like quick_insert, ordered_remove, and block_remove.  So calling
> those functions is not safe on a vec with nontrivial types.
> 
> Calling any of the sort functions also may not work correctly
> with nontrivial elements (gcc_sort() calls memcpy).  vec should
> either prevent that buy refusing to compile or use a safe
> (generic) template for that.
> 
> So while basic vec uses work with nontrivial types, there are
> plenty of bugs :(

OK, sounds bit dangerous but the use here is very simple.
I remember the non-POD rule mostly from the original David's
implementation of modref that did put non-pods to vector and he took
really long while to work out why it breaks.  And yep, it was before
2017 :)
Honza
> 
> Martin
> 
> > 
> > Honza
> > > 
> > > Martin
> > > 
> > > > The diagnostics should be from
> > > > 			a.parm_offset_known &= m.parm_offset_known;
> > > > Becasue both in the parm_map (which is variable m) and access_node
> > > > (which is variable a) the parm_offset_known has no meaning when
> > > > parm_index == MODREF_UNKNOWN_PARM.
> > > > 
> > > > If we want to avoid computing on these, perhaps this will work?
> > > > 
> > > > diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
> > > > index 0a097349ebd..97736d0d8a4 100644
> > > > --- a/gcc/ipa-modref-tree.h
> > > > +++ b/gcc/ipa-modref-tree.h
> > > > @@ -568,9 +568,13 @@ struct GTY((user)) modref_tree
> > > >    				      : (*parm_map) [a.parm_index];
> > > >    			    if (m.parm_index == MODREF_LOCAL_MEMORY_PARM)
> > > >    			      continue;
> > > > -			    a.parm_offset += m.parm_offset;
> > > > -			    a.parm_offset_known &= m.parm_offset_known;
> > > >    			    a.parm_index = m.parm_index;
> > > > +			    if (a.parm_index != MODREF_UNKNOWN_PARM)
> > > > +			      {
> > > > +				a.parm_offset_known &= m.parm_offset_known;
> > > > +				if (a.parm_offset_known)
> > > > +				  a.parm_offset += m.parm_offset;
> > > > +			      }
> > > >    			  }
> > > >    		      }
> > > >    		    changed |= insert (base_node->base, ref_node->ref, a,
> > > > >      /* Index of parameter we translate to.
> > > > >         Values from special_params enum are permitted too.  */
> > > > >      int parm_index;
> > > > > diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
> > > > > index c94f0589d44..630d202d5cf 100644
> > > > > --- a/gcc/ipa-modref.c
> > > > > +++ b/gcc/ipa-modref.c
> > > > > @@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
> > > > >          auto_vec <modref_parm_map, 32> parm_map;
> > > > >          modref_parm_map chain_map;
> > > > >          /* TODO: Once we get jump functions for static chains we could
> > > > > -	 compute this.  */
> > > > > -      chain_map.parm_index = MODREF_UNKNOWN_PARM;
> > > > > +	 compute parm_index.  */
> > > > >          compute_parm_map (edge, &parm_map);
> > > > > -- 
> > > > > 2.33.1
> > > > > 
> > > 
> 

Re: [PATCH] Fix IPA modref ubsan.

[Martin Sebor]

On 11/18/21 10:10 AM, Jan Hubicka wrote:
>>>> I don't know what the guidance is on using vec in IPA passes
>>>> but with respect to existing practice elsewhere, there are
>>>> existing uses of vec and auto_vec with non-POD types and vec
>>>> does work with them  (see the vec_default_construct and
>>>> vec_copy_construct templates, for example, whose goal is
>>>> to support nontrivial classes).
>>>
>>> I see, since 2017 :).  The patch is OK then.
>>> Nontrivial destructors also behave in a sane way these days?
>>
>> Good question :)
>>
>> At a minimum, element dtors should be automatically invoked by
>> the auto_vec dtor (there is an auto-test in vec.c to verify that).
>>
>> Beyond that, since (unlike auto_vec) a plain vec isn't a container
>> its users are on their own when it comes to managing the memory of
>> their elements (i.e., they need to explicitly destroy their elements).
>>
>> Having said that, as with all retrofits, they could be incomplete.
>> I see a few examples of where that seems to be the case here:
>>
>> Calling truncate() on a vec with notrivial elements leaks, so
>> clients needs to explicitly release those elements.  That
>> should happen automatically.
>>
>> Going through vec, I also see calls to memmove in functions
>> like quick_insert, ordered_remove, and block_remove.  So calling
>> those functions is not safe on a vec with nontrivial types.
>>
>> Calling any of the sort functions also may not work correctly
>> with nontrivial elements (gcc_sort() calls memcpy).  vec should
>> either prevent that buy refusing to compile or use a safe
>> (generic) template for that.
>>
>> So while basic vec uses work with nontrivial types, there are
>> plenty of bugs :(
> 
> OK, sounds bit dangerous but the use here is very simple.

I agree (that it's dangerous).  I expect -Wclass-memaccess will
catch the calls to memcpy/memmove if any of the vec members that
call them are instantiated on a nontrivial type.  Calling sort
will probably not trigger it because the memcpy call is buried
in a .c file (and not in a template).

Regardless, these uses should be still be fixed to work right.
I'll see if I can get to it at some point.

Martin

> I remember the non-POD rule mostly from the original David's
> implementation of modref that did put non-pods to vector and he took
> really long while to work out why it breaks.  And yep, it was before
> 2017 :)
> Honza
>>
>> Martin
>>
>>>
>>> Honza
>>>>
>>>> Martin
>>>>
>>>>> The diagnostics should be from
>>>>> 			a.parm_offset_known &= m.parm_offset_known;
>>>>> Becasue both in the parm_map (which is variable m) and access_node
>>>>> (which is variable a) the parm_offset_known has no meaning when
>>>>> parm_index == MODREF_UNKNOWN_PARM.
>>>>>
>>>>> If we want to avoid computing on these, perhaps this will work?
>>>>>
>>>>> diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
>>>>> index 0a097349ebd..97736d0d8a4 100644
>>>>> --- a/gcc/ipa-modref-tree.h
>>>>> +++ b/gcc/ipa-modref-tree.h
>>>>> @@ -568,9 +568,13 @@ struct GTY((user)) modref_tree
>>>>>     				      : (*parm_map) [a.parm_index];
>>>>>     			    if (m.parm_index == MODREF_LOCAL_MEMORY_PARM)
>>>>>     			      continue;
>>>>> -			    a.parm_offset += m.parm_offset;
>>>>> -			    a.parm_offset_known &= m.parm_offset_known;
>>>>>     			    a.parm_index = m.parm_index;
>>>>> +			    if (a.parm_index != MODREF_UNKNOWN_PARM)
>>>>> +			      {
>>>>> +				a.parm_offset_known &= m.parm_offset_known;
>>>>> +				if (a.parm_offset_known)
>>>>> +				  a.parm_offset += m.parm_offset;
>>>>> +			      }
>>>>>     			  }
>>>>>     		      }
>>>>>     		    changed |= insert (base_node->base, ref_node->ref, a,
>>>>>>       /* Index of parameter we translate to.
>>>>>>          Values from special_params enum are permitted too.  */
>>>>>>       int parm_index;
>>>>>> diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
>>>>>> index c94f0589d44..630d202d5cf 100644
>>>>>> --- a/gcc/ipa-modref.c
>>>>>> +++ b/gcc/ipa-modref.c
>>>>>> @@ -5020,8 +5020,7 @@ ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
>>>>>>           auto_vec <modref_parm_map, 32> parm_map;
>>>>>>           modref_parm_map chain_map;
>>>>>>           /* TODO: Once we get jump functions for static chains we could
>>>>>> -	 compute this.  */
>>>>>> -      chain_map.parm_index = MODREF_UNKNOWN_PARM;
>>>>>> +	 compute parm_index.  */
>>>>>>           compute_parm_map (edge, &parm_map);
>>>>>> -- 
>>>>>> 2.33.1
>>>>>>
>>>>
>>