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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 45EE93858400 for ; Wed, 25 Aug 2021 20:22:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 45EE93858400 Received: from mail-qv1-f71.google.com (mail-qv1-f71.google.com [209.85.219.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-55-MYvjvcZ4P-mZ_BD_jxOTtg-1; Wed, 25 Aug 2021 16:22:44 -0400 X-MC-Unique: MYvjvcZ4P-mZ_BD_jxOTtg-1 Received: by mail-qv1-f71.google.com with SMTP id gw9-20020a0562140f0900b0035decb1dfecso698163qvb.5 for ; Wed, 25 Aug 2021 13:22:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=nErQG7FiOfmzKSr0uIrNdtIqnphURnWGH4M2PDyOZyA=; b=aBTxj12YvLUQHeYkIdAxTgolrAPl8Y5LGJF624Cyfm66LWxuoinORwzUF3/+sXYZp6 /Ccm+GbOzs5qwfGDq7y1AF5KaJG0zhEWxRn4K4RyMMGsYa6at+R1/pum0HmNuXakLIb8 LLHArWuAFmXLpeRR8wsLgJz0JCnoq3Y4H+Zeq5udI2y44An8OSFa1LkIKpSyv9wuzmRZ 021eBvWRlgvXGXmsFcKAmTq7ejepaA3te5yVVEVnmgs3kk5bZHOv5jzOwTtyazworB8e OzrhDKNhDeE8zhbCNQsJ7BUcXAJ9Su8Wb9J8FsLQvDJg0guXlO2yNwoQWILOwqnJoOCY sMvw== X-Gm-Message-State: AOAM530Pb6V13bs53Amwsj/4jpOxXIeTLSuamFUnow+QPohKV44dCsLx w/tS+16SZbf8jfqC9xx65bJS9OmpqfuRJZfzSHiH4kHHnQ4MiQohNSqoCSvrA9crr8jlSW80tjT pdJ65CTzOf+8wQrvLsHKUgfY9dLxgKrkNdalh2mKuKAIgyBPSrXmSvn/JPl05WbKMxw== X-Received: by 2002:a05:620a:1598:: with SMTP id d24mr347520qkk.484.1629922962845; Wed, 25 Aug 2021 13:22:42 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxm75jtDAehCbNKfzG66pXIKgCHsFEkKkmgsRSo0Ei//EeG3Ubaf/IUc0ddiL9UHhbGmqrE1A== X-Received: by 2002:a05:620a:1598:: with SMTP id d24mr347487qkk.484.1629922962335; Wed, 25 Aug 2021 13:22:42 -0700 (PDT) Received: from [192.168.1.148] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id x10sm504136qto.41.2021.08.25.13.22.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Aug 2021 13:22:41 -0700 (PDT) Subject: Re: [PATCH] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions To: Barrett Adair , gcc-patches@gcc.gnu.org References: From: Jason Merrill Message-ID: Date: Wed, 25 Aug 2021 16:22:41 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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: Wed, 25 Aug 2021 20:22:56 -0000 On 8/21/21 12:55 AM, Barrett Adair via Gcc-patches wrote: > This patch fixes AST comparison for trailing return types using dependent > sizeof/alignof/noexcept expressions as template value arguments. I believe > this bug is over a decade old, hailing from GCC 4.6. I found it over 5 > years ago and sat on the repro until I had time to fix it myself. Thanks! > The new test cases demonstrate redeclarations mistakenly parsed as > ambiguous overloads. These fail with gcc, but pass with clang. The last > test case concerns a GNU extension for alignof (hence the -Wno-pedantic). > After applying this patch, bootstrap succeeds, the new tests pass, and my > native "make -k check" outputs look essentially similar to these based on > the same commit: > https://gcc.gnu.org/pipermail/gcc-testresults/2021-August/715032.html > (except I only waited for a few thousand of the libstdc++ tests to pass). > > I think it makes sense to now split the switch-case blocks here since there > isn't much shared logic left, and I have a patch to do that if desired, but > I think the patch below is the cleanest for initial review. > > I studied the history and churn of the comparing_specializations hacks. > While I am still not entirely certain this is the correct change to make > there, the change seems at least superficially reasonable to me. I haven't > been able to break it yet. Perhaps someone more familiar with this area of > the compiler can weigh in. See also the existing cp_unevaluated_operand > push/pop for these EXPR codes in cp_walk_tree. > > I am awaiting a response to the copyright assignment request sent to > assign@gnu.org. Until that goes through, you can use DCO sign-off (https://gcc.gnu.org/dco.html). > commit fe64ca143dfb9021b4c47abb6382827c13e1f0cd > Author: Barrett Adair > Date: Fri Aug 20 15:37:36 2021 -0500 Please generate your patch with git format-patch for easier application. It also looks like the patch was corrupted by your email client; a simple fix for that is to attach the patch instead of pasting it in the body of your message. Or 'git help format-patch' has suggestions for ways to make including the patch inline work. I mostly use git send-email myself, or attachments when using send-email would be too complicated. > Fix cp_tree_equal for template value args using > dependent sizeof/alignof/noexcept expressions > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index 3c62dd74380..fce2a46cfa8 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -3903,40 +3903,41 @@ cp_tree_equal (tree t1, tree t2) > as being equivalent to anything. */ > if (VAR_P (o1) && DECL_NAME (o1) == NULL_TREE > && !DECL_RTL_SET_P (o1)) > /*Nop*/; > else if (VAR_P (o2) && DECL_NAME (o2) == NULL_TREE > && !DECL_RTL_SET_P (o2)) > /*Nop*/; > else if (!cp_tree_equal (o1, o2)) > return false; > > return cp_tree_equal (TREE_OPERAND (t1, 1), TREE_OPERAND (t2, 1)); > } > > case PARM_DECL: > /* For comparing uses of parameters in late-specified return types > with an out-of-class definition of the function, but can also come > up for expressions that involve 'this' in a member function > template. */ > if (comparing_specializations > + && !cp_unevaluated_operand This looks dangerous; I would expect it to mean that in template struct A { }; template void f(T t1) { A a; } template void g(T t2) { A a; } the two As are treated as equivalent; that's what the comparing_specializations code is trying to prevent. > && DECL_CONTEXT (t1) != DECL_CONTEXT (t2)) > /* When comparing hash table entries, only an exact match is > good enough; we don't want to replace 'this' with the > version from another function. But be more flexible > with parameters with identical contexts. */ > return false; > > if (same_type_p (TREE_TYPE (t1), TREE_TYPE (t2))) > { > if (DECL_ARTIFICIAL (t1) ^ DECL_ARTIFICIAL (t2)) > return false; > if (CONSTRAINT_VAR_P (t1) ^ CONSTRAINT_VAR_P (t2)) > return false; > if (DECL_ARTIFICIAL (t1) > || (DECL_PARM_LEVEL (t1) == DECL_PARM_LEVEL (t2) > && DECL_PARM_INDEX (t1) == DECL_PARM_INDEX (t2))) > return true; > } > return false; > > @@ -3974,63 +3975,68 @@ cp_tree_equal (tree t1, tree t2) > return true; > > case CONSTRAINT_INFO: > return cp_tree_equal (CI_ASSOCIATED_CONSTRAINTS (t1), > CI_ASSOCIATED_CONSTRAINTS (t2)); > > case CHECK_CONSTR: > return (CHECK_CONSTR_CONCEPT (t1) == CHECK_CONSTR_CONCEPT (t2) > && comp_template_args (CHECK_CONSTR_ARGS (t1), > CHECK_CONSTR_ARGS (t2))); > > case TREE_VEC: > /* These are template args. Really we should be getting the > caller to do this as it knows it to be true. */ > if (!comp_template_args (t1, t2, NULL, NULL, false)) > return false; > return true; > > case SIZEOF_EXPR: > case ALIGNOF_EXPR: > + case NOEXCEPT_EXPR: > { > tree o1 = TREE_OPERAND (t1, 0); > tree o2 = TREE_OPERAND (t2, 0); > > if (code1 == SIZEOF_EXPR) > { > if (SIZEOF_EXPR_TYPE_P (t1)) > o1 = TREE_TYPE (o1); > if (SIZEOF_EXPR_TYPE_P (t2)) > o2 = TREE_TYPE (o2); > } > - else if (ALIGNOF_EXPR_STD_P (t1) != ALIGNOF_EXPR_STD_P (t2)) > + else if (code1 == ALIGNOF_EXPR > + && ALIGNOF_EXPR_STD_P (t1) != ALIGNOF_EXPR_STD_P (t2)) > return false; > > if (TREE_CODE (o1) != TREE_CODE (o2)) > return false; > > - if (ARGUMENT_PACK_P (o1)) > - return template_args_equal (o1, o2); > - else if (TYPE_P (o1)) > - return same_type_p (o1, o2); > - else > - return cp_tree_equal (o1, o2); > + { > + cp_unevaluated ev; > + if (code1 == SIZEOF_EXPR && ARGUMENT_PACK_P (o1)) > + return template_args_equal (o1, o2); > + else if (code1 != NOEXCEPT_EXPR && TYPE_P (o1)) Are these code1 checks necessary? I don't mind them, but I would have expected the code to work without them. > + return same_type_p (o1, o2); > + else > + return cp_tree_equal (o1, o2); > + } > } > > case MODOP_EXPR: > { > tree t1_op1, t2_op1; > > if (!cp_tree_equal (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0))) > return false; > > t1_op1 = TREE_OPERAND (t1, 1); > t2_op1 = TREE_OPERAND (t2, 1); > if (TREE_CODE (t1_op1) != TREE_CODE (t2_op1)) > return false; > > return cp_tree_equal (TREE_OPERAND (t1, 2), TREE_OPERAND (t2, 2)); > } > > case PTRMEM_CST: > /* Two pointer-to-members are the same if they point to the same > field or function in the same class. */ > diff --git a/gcc/testsuite/g++.dg/template/canon-type-15.C > b/gcc/testsuite/g++.dg/template/canon-type-15.C > new file mode 100644 > index 00000000000..064e14c510d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/canon-type-15.C > @@ -0,0 +1,5 @@ > +// { dg-do compile { target c++11 } } > +template struct size_c{ static constexpr unsigned value = u; }; > +template auto return_size(T t) -> size_c; > +template auto return_size(T t) -> size_c; > +static_assert(decltype(return_size('a'))::value == 1u, ""); > diff --git a/gcc/testsuite/g++.dg/template/canon-type-16.C > b/gcc/testsuite/g++.dg/template/canon-type-16.C > new file mode 100644 > index 00000000000..99361cbac30 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/canon-type-16.C > @@ -0,0 +1,6 @@ > +// { dg-do compile { target c++11 } } > +template struct bool_c{ static constexpr bool value = u; }; > +template auto noexcepty(T t) -> bool_c; > +template auto noexcepty(T t) -> bool_c; > +struct foo { void operator()() noexcept; }; > +static_assert(decltype(noexcepty(foo{}))::value, ""); > diff --git a/gcc/testsuite/g++.dg/template/canon-type-17.C > b/gcc/testsuite/g++.dg/template/canon-type-17.C > new file mode 100644 > index 00000000000..0555c8d0a42 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/canon-type-17.C > @@ -0,0 +1,5 @@ > +// { dg-do compile { target c++11 } } > +template struct size_c{ static constexpr unsigned value = u; }; > +template auto return_size(T... t) -> size_c; > +template auto return_size(T... t) -> size_c; > +static_assert(decltype(return_size('a'))::value == 1u, ""); > diff --git a/gcc/testsuite/g++.dg/template/canon-type-18.C > b/gcc/testsuite/g++.dg/template/canon-type-18.C > new file mode 100644 > index 00000000000..2510181725c > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/canon-type-18.C > @@ -0,0 +1,6 @@ > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wno-pedantic" } > +template struct size_c{ static constexpr unsigned value = u; }; > +template auto get_align(T t) -> size_c; > +template auto get_align(T t) -> size_c; > +static_assert(decltype(get_align('a'))::value == 1u, ""); >