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
