From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 2B0273858C66 for ; Wed, 17 Jan 2024 16:29:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2B0273858C66 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2B0273858C66 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705508946; cv=none; b=C9fNsUhuipRHz3yZemv2hglwsh0fYBUrVqxxALXLZq81BwqF+KU/eLVnUbfD9Rz9Gln/OZk6+HZiRNTVNQ0BV0QBJ82QItEb2Utso3T8pPVJsE9UP6n+najllbz980u+zIUvBH/nOcBgWGW/uorhZZEhY9+JUIf/qWzo2G7MRSE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705508946; c=relaxed/simple; bh=j5FifNTzC4pglqXAMUqYhbFt+jevTSlLCEnU2BQQXzU=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=s1xVLMOmYTxXcyLC6P1eH272/ZZLuAmDP7PIfJ8GbqYpw728KOLUCRfMKG7cgOneJMPhUJImXNAUOUwy8EXMXl5TestpAI9LUOrUv+fRil7hsgDGYFXzCaKvxMCnzK+2Ys/FgYrZPHml73BqjTrP3TVK6lgyeDUENa6wwgTLYLo= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705508943; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=B1RJ26dWDeqoy7NRXZNkgQdo1ASQnJYD5I2yOl+/YbM=; b=ZV9KIvxtX568mh/gA+Fjic/CyVEM1U3cmHLAykQbzPsW2dpPTesDxdN1hMDCa3WBcrzlDI vD1y09QcV52znqlTlROKYu3fwAZ7ikEgbO/120zdIBFSqTRnjpEflS0NkiovLQxA/s8sGG HSYTCAXrzdWKSM3fVYfAChhAxCootjA= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-64-u9UwtrrnMjumryPivRtOhg-1; Wed, 17 Jan 2024 11:28:47 -0500 X-MC-Unique: u9UwtrrnMjumryPivRtOhg-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-7831ed4bb2aso1580648985a.0 for ; Wed, 17 Jan 2024 08:28:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705508907; x=1706113707; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=B1RJ26dWDeqoy7NRXZNkgQdo1ASQnJYD5I2yOl+/YbM=; b=P27yJj75psrNS6UMxqATfE8SD1vTs/Ocuey5MbeebZLbcGttZstcn65lIatjrs1wmo 5kSGboC4Nxnc6+ksDTgwkZqbtnPDHSkov7Zz1ORZnxL+r7+ZC9bAL87GoJzdzrjjq1aE 10enrLl8aX3zu+JJWVqGlsnXjNvnZrz4VXKJjKdZjRG7JbRaGyOJTURJcNnjqruVspk9 zba8HZ8lJMVSQ3BAYUnB2tc5lL/9B7wnrJaqKgffubKAIcFI4HhY3Za8Y9Srm+ghX27r ME7xRG1z/sP3/IXFELnS/kH8Ty4h1mbXKglz/EWTTeN00K4ulAwC12k/oqcLW/Oj3QlE XJTw== X-Gm-Message-State: AOJu0YwmyW7l33hSM7ZkVMA5nVx3QUljgmf9XniXxbWJLLCZCNEyiAJD X6ruVRyQ178ILiwdd3HpfPPAofcOEVpMSPKQvbIiUAUSQtAAt0yvl8+nhJp7N6xmsBTKHZzP6BV O9OF8lntYxudVpocEvpIt8+hn4Q== X-Received: by 2002:ae9:e215:0:b0:783:2911:544b with SMTP id c21-20020ae9e215000000b007832911544bmr10062960qkc.82.1705508907292; Wed, 17 Jan 2024 08:28:27 -0800 (PST) X-Google-Smtp-Source: AGHT+IHmgkrOdfBKQxDm2Mwto2ZQDIZDmtwKy8eKd5+HUaM1HmoVzoesINaJtcscMnZ7u5/sUJVovw== X-Received: by 2002:ae9:e215:0:b0:783:2911:544b with SMTP id c21-20020ae9e215000000b007832911544bmr10062947qkc.82.1705508906959; Wed, 17 Jan 2024 08:28:26 -0800 (PST) Received: from [192.168.1.146] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id p11-20020a05620a22ab00b0078335b0cb93sm4650783qkh.93.2024.01.17.08.28.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Jan 2024 08:28:26 -0800 (PST) Message-ID: <73d3620c-74a8-4e71-86a5-7c562d4c24bd@redhat.com> Date: Wed, 17 Jan 2024 11:28:25 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] To: Patrick Palka , Nathaniel Shead Cc: Nathan Sidwell , gcc-patches@gcc.gnu.org References: <655de700.170a0220.bbd9.fe31@mx.google.com> <26aa9f6b-a015-4e6f-aa73-6912cbca50f6@acm.org> <659ab54e.630a0220.291d5.8735@mx.google.com> <8f429848-9e1a-38b7-4120-865b55780c1a@idea> From: Jason Merrill In-Reply-To: <8f429848-9e1a-38b7-4120-865b55780c1a@idea> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,RCVD_IN_SORBS_WEB,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE,URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 1/8/24 12:04, Patrick Palka wrote: > On Mon, 8 Jan 2024, Nathaniel Shead wrote: > >> On Sat, Jan 06, 2024 at 05:32:37PM -0500, Nathan Sidwell wrote: >>> I;m not sure about this, there was clearly a reason I did it the way it is, >>> but perhaps that reasoning became obsolete -- something about an existing >>> declaration and reading in a definition maybe? >> >> So I took a bit of a closer look and this is actually a regression, >> seeming to start with r13-3134-g09df0d8b14dda6. I haven't looked more >> closely at the actual change though to see whether this implies a >> different fix yet though. > > Interesting.. FWIW I applied your patch to the gcc 12 release branch, > which doesn't have r13-3134, and there were no modules testsuite > regressions there either, which at least suggests that this maybe_dup > logic isn't directly related to the optimization that r13-3134 removed. > > Your patch also seems to fix PR99244 (which AFAICT is not a regression) It seems to me we always want the DECL_ARGUMENTS corresponding to the actual definition we're using, which since "installing" is true, is the new definition. In duplicate_decls when we merge a new definition into an old declaration, we give the old declaration the new DECL_ARGUMENTS. The patch is OK. >>> On 11/22/23 06:33, Nathaniel Shead wrote: >>>> Bootstrapped and regtested on x86_64-pc-linux-gnu. I don't have write >>>> access. >>>> >>>> -- >8 -- >>>> >>>> When merging duplicate instantiations of function templates, currently >>>> read_function_def overwrites the arguments with that of the existing >>>> duplicate. This is problematic, however, since this means that the >>>> PARM_DECLs in the body of the function definition no longer match with >>>> the PARM_DECLs in the argument list, which causes issues when it comes >>>> to generating RTL. >>>> >>>> There doesn't seem to be any reason to do this replacement, so this >>>> patch removes that logic. >>>> >>>> PR c++/112588 >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> * module.cc (trees_in::read_function_def): Don't overwrite >>>> arguments. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * g++.dg/modules/merge-16.h: New test. >>>> * g++.dg/modules/merge-16_a.C: New test. >>>> * g++.dg/modules/merge-16_b.C: New test. >>>> >>>> Signed-off-by: Nathaniel Shead >>>> --- >>>> gcc/cp/module.cc | 2 -- >>>> gcc/testsuite/g++.dg/modules/merge-16.h | 10 ++++++++++ >>>> gcc/testsuite/g++.dg/modules/merge-16_a.C | 7 +++++++ >>>> gcc/testsuite/g++.dg/modules/merge-16_b.C | 5 +++++ >>>> 4 files changed, 22 insertions(+), 2 deletions(-) >>>> create mode 100644 gcc/testsuite/g++.dg/modules/merge-16.h >>>> create mode 100644 gcc/testsuite/g++.dg/modules/merge-16_a.C >>>> create mode 100644 gcc/testsuite/g++.dg/modules/merge-16_b.C >>>> >>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>> index 4f5b6e2747a..2520ab659cc 100644 >>>> --- a/gcc/cp/module.cc >>>> +++ b/gcc/cp/module.cc >>>> @@ -11665,8 +11665,6 @@ trees_in::read_function_def (tree decl, tree maybe_template) >>>> DECL_RESULT (decl) = result; >>>> DECL_INITIAL (decl) = initial; >>>> DECL_SAVED_TREE (decl) = saved; >>>> - if (maybe_dup) >>>> - DECL_ARGUMENTS (decl) = DECL_ARGUMENTS (maybe_dup); >>>> if (context) >>>> SET_DECL_FRIEND_CONTEXT (decl, context); >>>> diff --git a/gcc/testsuite/g++.dg/modules/merge-16.h b/gcc/testsuite/g++.dg/modules/merge-16.h >>>> new file mode 100644 >>>> index 00000000000..fdb38551103 >>>> --- /dev/null >>>> +++ b/gcc/testsuite/g++.dg/modules/merge-16.h >>>> @@ -0,0 +1,10 @@ >>>> +// PR c++/112588 >>>> + >>>> +void f(int*); >>>> + >>>> +template >>>> +struct S { >>>> + void g(int n) { f(&n); } >>>> +}; >>>> + >>>> +template struct S; > > If we use a partial specialization here instead (which would have disabled > the removed optimization, demonstrating how fragile/inconsistent it was) > > void f(int*); > > template > struct S { }; > > template > struct S { > void g(int n) { f(&n); } > }; > > template struct S; > > then the ICE appears earlier, since GCC 12 instead of 13. > >>>> diff --git a/gcc/testsuite/g++.dg/modules/merge-16_a.C b/gcc/testsuite/g++.dg/modules/merge-16_a.C >>>> new file mode 100644 >>>> index 00000000000..c243224c875 >>>> --- /dev/null >>>> +++ b/gcc/testsuite/g++.dg/modules/merge-16_a.C >>>> @@ -0,0 +1,7 @@ >>>> +// PR c++/112588 >>>> +// { dg-additional-options "-fmodules-ts" } >>>> +// { dg-module-cmi merge16 } >>>> + >>>> +module; >>>> +#include "merge-16.h" >>>> +export module merge16; >>>> diff --git a/gcc/testsuite/g++.dg/modules/merge-16_b.C b/gcc/testsuite/g++.dg/modules/merge-16_b.C >>>> new file mode 100644 >>>> index 00000000000..8c7b1f0511f >>>> --- /dev/null >>>> +++ b/gcc/testsuite/g++.dg/modules/merge-16_b.C >>>> @@ -0,0 +1,5 @@ >>>> +// PR c++/112588 >>>> +// { dg-additional-options "-fmodules-ts" } >>>> + >>>> +#include "merge-16.h" >>>> +import merge16; >>> >>> -- >>> Nathan Sidwell >>> >> >> >