Message ID | a05d2840-f243-e6a2-36eb-f1426465f218@suse.cz |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D3896385C41C for <patchwork@sourceware.org>; Thu, 18 Nov 2021 12:35:26 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 72BB6385841C for <gcc-patches@gcc.gnu.org>; Thu, 18 Nov 2021 12:35:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 72BB6385841C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id BE9F7212C4; Thu, 18 Nov 2021 12:35:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1637238908; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bIP7HAo9AmEwOvcNKgyHaUTS56NvIyNLJgcEsKyzaPM=; b=zQVljzt+LfbalRwX24dytHxKDeEdPiirgpZ0vzEC2jlrILlp69PVVGbPZPnXX0XRTceoOS RLuO1+2WNapeeJjunL6FwXTVaTmrQIwsH0/t904ZpVP0v/qBYSSBF0WNygP/sawL2ox55H P2Pz3/mO7zL7/935u1GXI5RbzpMpQYA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1637238908; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bIP7HAo9AmEwOvcNKgyHaUTS56NvIyNLJgcEsKyzaPM=; b=Z0xrEXz2B436BWF8rb6hCcL0VrAnOlHsb+RripW6sYrR5GPscDFhaL57tlq+wlieD1oXCW 2bjM9p3YOQUAbdAw== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id A148113D21; Thu, 18 Nov 2021 12:35:08 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Vqq2JXxIlmEZSwAAMHmgww (envelope-from <mliska@suse.cz>); Thu, 18 Nov 2021 12:35:08 +0000 Message-ID: <a05d2840-f243-e6a2-36eb-f1426465f218@suse.cz> Date: Thu, 18 Nov 2021 13:35:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH] Fix IPA modref ubsan. To: gcc-patches@gcc.gnu.org Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Cc: Jan Hubicka <hubicka@ucw.cz> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Fix IPA modref ubsan.
|
|
Commit Message
Martin Liška
Nov. 18, 2021, 12:35 p.m. UTC
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(-)
Comments
> 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 >
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 >>
> 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 > > > >
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 >>>> >>
> > > 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 > > > > > > > > >
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 >>>>>> >>>> >>
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);