From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5383 invoked by alias); 18 Dec 2018 20:40: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 5369 invoked by uid 89); 18 Dec 2018 20:40:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=Dave X-HELO: mail-qk1-f195.google.com Received: from mail-qk1-f195.google.com (HELO mail-qk1-f195.google.com) (209.85.222.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Dec 2018 20:40:09 +0000 Received: by mail-qk1-f195.google.com with SMTP id 189so10334473qkj.8 for ; Tue, 18 Dec 2018 12:40:09 -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 c12sm922397qka.42.2018.12.18.12.40.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 18 Dec 2018 12:40:06 -0800 (PST) Subject: Re: [PATCH] v6: C++: more location wrapper nodes (PR c++/43064, PR c++/43486) To: David Malcolm Cc: Jeff Law , gcc-patches@gcc.gnu.org References: <1545089414.4619.239.camel@redhat.com> <1545168126-24189-1-git-send-email-dmalcolm@redhat.com> From: Jason Merrill Message-ID: Date: Tue, 18 Dec 2018 20:40: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: <1545168126-24189-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg01333.txt.bz2 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. Jason