* [PATCH] Fix IPA modref ubsan. @ 2021-11-18 12:35 Martin Liška 2021-11-18 12:41 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Martin Liška @ 2021-11-18 12:35 UTC (permalink / raw) To: gcc-patches; +Cc: 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 () + {} + /* 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IPA modref ubsan. 2021-11-18 12:35 [PATCH] Fix IPA modref ubsan Martin Liška @ 2021-11-18 12:41 ` Jan Hubicka 2021-11-18 15:48 ` Martin Sebor 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2021-11-18 12:41 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IPA modref ubsan. 2021-11-18 12:41 ` Jan Hubicka @ 2021-11-18 15:48 ` Martin Sebor 2021-11-18 16:22 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Martin Sebor @ 2021-11-18 15:48 UTC (permalink / raw) To: Jan Hubicka, Martin Liška; +Cc: gcc-patches 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IPA modref ubsan. 2021-11-18 15:48 ` Martin Sebor @ 2021-11-18 16:22 ` Jan Hubicka 2021-11-18 16:54 ` Martin Sebor 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2021-11-18 16:22 UTC (permalink / raw) To: Martin Sebor; +Cc: Martin Liška, gcc-patches > 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 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IPA modref ubsan. 2021-11-18 16:22 ` Jan Hubicka @ 2021-11-18 16:54 ` Martin Sebor 2021-11-18 17:10 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Martin Sebor @ 2021-11-18 16:54 UTC (permalink / raw) To: Jan Hubicka; +Cc: Martin Liška, gcc-patches 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 >>>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IPA modref ubsan. 2021-11-18 16:54 ` Martin Sebor @ 2021-11-18 17:10 ` Jan Hubicka 2021-11-19 16:10 ` Martin Sebor 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2021-11-18 17:10 UTC (permalink / raw) To: Martin Sebor; +Cc: Martin Liška, gcc-patches > > > 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 > > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix IPA modref ubsan. 2021-11-18 17:10 ` Jan Hubicka @ 2021-11-19 16:10 ` Martin Sebor 0 siblings, 0 replies; 7+ messages in thread From: Martin Sebor @ 2021-11-19 16:10 UTC (permalink / raw) To: Jan Hubicka; +Cc: Martin Liška, gcc-patches 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 >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-19 16:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-18 12:35 [PATCH] Fix IPA modref ubsan Martin Liška 2021-11-18 12:41 ` Jan Hubicka 2021-11-18 15:48 ` Martin Sebor 2021-11-18 16:22 ` Jan Hubicka 2021-11-18 16:54 ` Martin Sebor 2021-11-18 17:10 ` Jan Hubicka 2021-11-19 16:10 ` Martin Sebor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).