From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91884 invoked by alias); 19 Dec 2018 15:35:12 -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 91214 invoked by uid 89); 19 Dec 2018 15:35:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Dec 2018 15:35:04 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DD30DC062EAB for ; Wed, 19 Dec 2018 15:35:02 +0000 (UTC) Received: from ovpn-117-98.phx2.redhat.com (ovpn-117-98.phx2.redhat.com [10.3.117.98]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5D1371001F3C; Wed, 19 Dec 2018 15:35:02 +0000 (UTC) Message-ID: <1545233701.4619.258.camel@redhat.com> Subject: Re: [PATCH] v6: C++: more location wrapper nodes (PR c++/43064, PR c++/43486) From: David Malcolm To: Jason Merrill Cc: Jeff Law , gcc-patches@gcc.gnu.org Date: Wed, 19 Dec 2018 15:35:00 -0000 In-Reply-To: References: <1545089414.4619.239.camel@redhat.com> <1545168126-24189-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg01398.txt.bz2 On Tue, 2018-12-18 at 15:40 -0500, Jason Merrill wrote: > On 12/18/18 4:22 PM, David Malcolm wrote: > > On Mon, 2018-12-17 at 18:30 -0500, 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? > > > > > > Dave > > > > I tried both of the above approaches, and both work. > > > > Here's v6 of the patch: > > > > I removed the strip of wrappers in > > cp_parser_late_parsing_default_args > > from earlier versions of the patch, in favor of fixing > > simple_cst_equal > > so that it treats location wrappers with unequal source locations > > as > > being unequal. This ensures that function-types with default > > arguments > > don't get merged when the default argument constants have different > > spelling locations. [I have an alternative patch which instead > > introduces a different comparator for FUNCTION_TYPE's > > TYPE_ARG_TYPES > > within type_cache_hasher::equal, almost identical to > > type_list_equal, > > but adding the requirement that location wrappers around default > > arguments have equal source location for the params to be > > considered > > equal; both patches pass bootstrap®ression testing] > > > > Doing so leads to the reported location for the bad default > > argument > > within a template in g++.dg/template/defarg6.C moving to the > > argument > > location. Previously, the callsite of the instantiation was > > identified > > due to the use of input_location in convert_like_real here: > > > > 6816 location_t loc = cp_expr_loc_or_loc (expr, > > input_location); > > > > With a location wrapper, it uses the spelling location of the > > default argument, but doesn't identify the location of the callsite > > that's instantiating the template. > > > > So I moved the note in tsubst_default_argument about which callsite > > led to a diagnostic to after the check_default_argument call, so > > that > > diagnostics within that receive notes, too. > > > > As before, this was successfully bootstrapped & regrtested on > > x86_64-pc-linux-gnu, in conjunction with the followup patch. > > > > OK for trunk? > > Ah, I hadn't seen this before my last email. Let's go with this > version. Thanks. I've now committed the v6 patch, and the follow-ups that were already approved (having rebased and retested them): r267272: "C++: more location wrapper nodes (PR c++/43064, PR c++/43486)" r267273: "C++: improvements to binary operator diagnostics (PR c++/87504)" r267276: "C++: better locations for bogus initializations (PR c++/88375)" Dave