Re: [PATCH] Fix IPA modref ubsan.

[Martin Sebor <msebor@gmail.com>]

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
>>>>
>>