Re: [PATCH] Fix IPA modref ubsan.

[Jan Hubicka <hubicka@kam.mff.cuni.cz>]

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