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 9E6A9385E03D for ; Mon, 8 Jan 2024 17:04:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9E6A9385E03D 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 9E6A9385E03D 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=1704733493; cv=none; b=QVaedLw1/fJ7CMTe1N9GkLChmKgoKlZ/NT3nMnf7hYwGrcxMD/1qTYsYrpCUQbWzqdEV8/8mxMsbwXxb8KVcvFWF0OOuzNM0iUkv6Dw1t1FXTPsU1+0zWMzAxVwRJT+vyt6JYjBBXXT5Oz1KFpUFHNRbMd4qbuVoGfkYw293O1M= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704733493; c=relaxed/simple; bh=e/jVlbGIanm9JowHPLFOkHQx7UbuQ6tzVg27DMr2drw=; h=DKIM-Signature:From:Date:To:Subject:Message-ID:MIME-Version; b=Yao0WHx0rI/EZ/qVJVcqwkoaRSPvYORLxCPNWr06qvqy7jr6H5EUCGMsNo+vxeNxxQWjA1y5bY6mOJcNw2MdIu3aQksI1kPjCOafL9yypNa83uw0WFWx8YwJMjHAIKF84/oyU0Q5aeyx3c2B3VoTIfEEQWl9Qegl7A0hLucYtME= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704733490; 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: in-reply-to:in-reply-to:references:references; bh=a/8yifuSXOobtyR7+1b2zNo/FuySNoJH7oiiH8d0Va4=; b=cZkXqqPK5zCbm5Sv8et7V8jozK/BvGnk3aQPr/ca/zt0G44m+8xhmxGuO5eccd9mW5kK9B zSgkIQec6nPgbsUf2KfUJ2gQHlKOr31NkofXbyAgoAM4vQYApf997UrCCnhIdZgTnqtcJt TRYQYJtZcewu+d6lUt1cCMImAm/Ln/M= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-320-3AX4ca07NguDkMhZzXmu2g-1; Mon, 08 Jan 2024 12:04:49 -0500 X-MC-Unique: 3AX4ca07NguDkMhZzXmu2g-1 Received: by mail-qv1-f69.google.com with SMTP id 6a1803df08f44-67fb96f8d9cso38284366d6.2 for ; Mon, 08 Jan 2024 09:04:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704733489; x=1705338289; h=mime-version:references:message-id:in-reply-to:subject:cc:to:date :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=a/8yifuSXOobtyR7+1b2zNo/FuySNoJH7oiiH8d0Va4=; b=EOHeWCm2QBWPCM6ESONm5tGJChA/p+AhX4048AI0H5Vi4LeFMBnMgMVHNUHI+3Q/6U 2LGKENU4Ey36wDyroHHW4tIqDcvNhd9wfrpIIqHJciEG6L8F36oitsV9faF9obpU4tIQ ZKKus+NX1dU1byKbnfctIgy9aUgcW1bmTQTuX1+kAvAA8IyD7CUlm7wnmtZaT3j7Ivxe WvOqRUva0DwHyzbYGmAFT0i5DMCbzri68LP3D3yLOcQwJt3LOGaBOkp5OZIWvtVmlhFc 1pZZsAW09LOEfwgCx4IpASEYrf2bDVg8fPSgXYUDwPwxecP4bP8aCo3Dp4aPV3GYFa0N lkDw== X-Gm-Message-State: AOJu0Yw8JW6yzP2c52q3+EV7SvySwUMlz3hKvkiIOXb70ZWnh/UEeoM7 lCi1xvJjMSNYG5x1HDcUzVMNDsC5jJgvhmdKh+a27uzoFFekRGVtSPRwaB5nF6hHv7Z9H7WedUK dm6pHURWUjRavJOLp7pkUi8uEAA== X-Received: by 2002:ad4:5c6e:0:b0:67f:252f:e7d with SMTP id i14-20020ad45c6e000000b0067f252f0e7dmr5766410qvh.18.1704733488871; Mon, 08 Jan 2024 09:04:48 -0800 (PST) X-Google-Smtp-Source: AGHT+IGjgQEBxgKGmTjUaXlDL24zvwIsm0bPreny7QJpI/4O+dXI+uNVqFxQsD4/g4W/if/ZiFbO2g== X-Received: by 2002:ad4:5c6e:0:b0:67f:252f:e7d with SMTP id i14-20020ad45c6e000000b0067f252f0e7dmr5766390qvh.18.1704733488553; Mon, 08 Jan 2024 09:04:48 -0800 (PST) Received: from [192.168.1.130] (ool-457670bb.dyn.optonline.net. [69.118.112.187]) by smtp.gmail.com with ESMTPSA id j17-20020a0cf9d1000000b0067f678747ffsm122908qvo.50.2024.01.08.09.04.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jan 2024 09:04:48 -0800 (PST) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Mon, 8 Jan 2024 12:04:47 -0500 (EST) To: Nathaniel Shead cc: Nathan Sidwell , gcc-patches@gcc.gnu.org, Jason Merrill , Patrick Palka Subject: Re: [PATCH] c++/modules: Prevent overwriting arguments for duplicates [PR112588] In-Reply-To: <659ab54e.630a0220.291d5.8735@mx.google.com> Message-ID: <8f429848-9e1a-38b7-4120-865b55780c1a@idea> References: <655de700.170a0220.bbd9.fe31@mx.google.com> <26aa9f6b-a015-4e6f-aa73-6912cbca50f6@acm.org> <659ab54e.630a0220.291d5.8735@mx.google.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-13.4 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 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? > > > > nathan > > 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) > > Nathaniel > > > 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 > > > >