From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 103944 invoked by alias); 18 Dec 2018 20:23:55 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 103890 invoked by uid 89); 18 Dec 2018 20:23:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=7316, 7066 X-HELO: mail-qt1-f193.google.com Received: from mail-qt1-f193.google.com (HELO mail-qt1-f193.google.com) (209.85.160.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Dec 2018 20:23:53 +0000 Received: by mail-qt1-f193.google.com with SMTP id i7so19696664qtj.10 for ; Tue, 18 Dec 2018 12:23:52 -0800 (PST) Return-Path: Received: from [192.168.1.149] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id r67sm791428qkr.28.2018.12.18.12.23.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 18 Dec 2018 12:23:50 -0800 (PST) Subject: Re: [PATCH] v5: C++: more location wrapper nodes (PR c++/43064, PR c++/43486) To: David Malcolm Cc: Jeff Law , gcc-patches@gcc.gnu.org References: <1544833064-29934-1-git-send-email-dmalcolm@redhat.com> <4a0448bb-8f3e-92cb-0e13-f8928632cc0c@redhat.com> <1545089414.4619.239.camel@redhat.com> From: Jason Merrill Message-ID: <7ec66fa2-ffcd-3a22-3ad1-a52848882a8d@redhat.com> Date: Tue, 18 Dec 2018 20:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <1545089414.4619.239.camel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg01330.txt.bz2 On 12/17/18 6:30 PM, David Malcolm wrote: > On Mon, 2018-12-17 at 14:33 -0500, Jason Merrill wrote: >> On 12/14/18 7:17 PM, David Malcolm wrote: >>> + /* Since default args are effectively part of the function >>> type, >>> + strip location wrappers here, since otherwise the >>> location of >>> + one function's default arguments is arbitrarily chosen >>> for >>> + all functions with similar signature (due to >>> canonicalization >>> + of function types). */ >> >> Hmm, looking at this again, why would this happen? I see that >> type_list_equal uses == to compare default arguments, so two >> function >> types with the same default argument but different location wrappers >> shouldn't be combined. >> >> Jason > > Thanks. > > I did some digging into this. I added this strip to fix > g++.dg/template/defarg6.C > but it looks like I was overzealous (the comment is correct, but it's > papering over a problem). > > It turns out that type_list_equal is doing more than just pointer > equality; it's hitting the simple_cst_equal part of the && at line > 7071: > > 7063 bool > 7064 type_list_equal (const_tree l1, const_tree l2) > 7065 { > 7066 const_tree t1, t2; > 7067 > 7068 for (t1 = l1, t2 = l2; t1 && t2; t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2)) > 7069 if (TREE_VALUE (t1) != TREE_VALUE (t2) > 7070 || (TREE_PURPOSE (t1) != TREE_PURPOSE (t2) > 7071 && ! (1 == simple_cst_equal (TREE_PURPOSE (t1), TREE_PURPOSE (t2)) > 7072 && (TREE_TYPE (TREE_PURPOSE (t1)) > 7073 == TREE_TYPE (TREE_PURPOSE (t2)))))) > 7074 return false; > 7075 > 7076 return t1 == t2; > 7077 } > > What's happening is that there are two different functions with > identical types apart from the locations of their (equal) default > arguments: both of the TREE_PURPOSEs are NON_LVALUE_EXPR wrappers > around a CONST_DECL enum value (at different source locations). > > simple_cst_equal is stripping the location wrappers here: > > 7311 if (CONVERT_EXPR_CODE_P (code1) || code1 == NON_LVALUE_EXPR) > 7312 { > 7313 if (CONVERT_EXPR_CODE_P (code2) > 7314 || code2 == NON_LVALUE_EXPR) > 7315 return simple_cst_equal (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0)); > 7316 else > 7317 return simple_cst_equal (TREE_OPERAND (t1, 0), t2); > 7318 } > > and thus finds them to be equal; the iteration in type_list_equal > continues, and runs out of parameters with t1 == t2 == NULL, and thus > returns true, and thus the two function types hash to the same slot, > and the two function types get treated as being the same. > > It's not clear to me yet what the best solution to this is: > - should simple_cst_equal regard different source locations as being > different? > - should function-type hashing use a custom version of type_list_equal > when comparing params, and make different source locations of default > args be different? > - something else? I'd experiment with removing the simple_cst_equal bit in type_list_equal. But I think that can wait, and you can go ahead and commit the v5 patch. Jason