From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x229.google.com (mail-oi1-x229.google.com [IPv6:2607:f8b0:4864:20::229]) by sourceware.org (Postfix) with ESMTPS id F1742385800C for ; Fri, 19 Nov 2021 16:10:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F1742385800C Received: by mail-oi1-x229.google.com with SMTP id bj13so22741562oib.4 for ; Fri, 19 Nov 2021 08:10:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=vmQAOnlXLwTyVCx+i7zIXPVE+0skk4mrT2qiiWUKT2U=; b=gXrQENemNfJXVuss/F/tkjQ733kiWI/6dylzyaWhSP9+5ICzppwMJCw2xgobzIkdGc AvHoT76JGg5XAXVoqFF3B2SS6DWBxN2nWk2gK+te6vuXm/GDby6HUEobVsh4ehTHj+dZ 1yGDLMjGQEdnqcoJ6qP36h6KDpy73dSITM4Zb9MtaV7MRkBmGhU8OCXz8MokR9w1DgIh nNKYed1HL3Jkq1mmt9NSLKQPJ2gr8Xf5ot95qFY8W6uVqCikp/vWmnGuqvVzW3IgWoC1 6Ltqj/xJ9zsTUXT6VNJvOUcb6Akfa+pzBq+9MjWA0HeUjDInQwnXpvcaucObJt+uzLRz T6PQ== X-Gm-Message-State: AOAM530PkgOYeotOoC0EkGRgCRmZdO5VdctGaMotlpDn32ymASlo4Blt rovTW+cWdOKzPhpna8rM6oB9qUWCDHU= X-Google-Smtp-Source: ABdhPJxriIS+nnk9pXjJDo1iEs7eHnZfuTxLFGuiq9Tuda5s4AIHEEMTcV2ce0YHVGG4oKcazJcy9w== X-Received: by 2002:aca:130e:: with SMTP id e14mr722697oii.51.1637338224197; Fri, 19 Nov 2021 08:10:24 -0800 (PST) Received: from [192.168.0.41] (184-96-250-76.hlrn.qwest.net. [184.96.250.76]) by smtp.gmail.com with ESMTPSA id d6sm54639otb.4.2021.11.19.08.10.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 19 Nov 2021 08:10:23 -0800 (PST) Subject: Re: [PATCH] Fix IPA modref ubsan. To: Jan Hubicka Cc: =?UTF-8?Q?Martin_Li=c5=a1ka?= , gcc-patches@gcc.gnu.org References: <20211118124154.GA64247@kam.mff.cuni.cz> <3969b362-f4e1-a2fe-8feb-6fda99131122@gmail.com> <20211118162203.GA76860@kam.mff.cuni.cz> <20211118171032.GB76860@kam.mff.cuni.cz> From: Martin Sebor Message-ID: <9547ca39-09ab-19b7-3068-02995bffbdd8@gmail.com> Date: Fri, 19 Nov 2021 09:10:22 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <20211118171032.GB76860@kam.mff.cuni.cz> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Nov 2021 16:10:27 -0000 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 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 >>>>>> >>>> >>