From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130761 invoked by alias); 8 Jan 2018 17:02:48 -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 130748 invoked by uid 89); 8 Jan 2018 17:02:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=gains, 2152 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; Mon, 08 Jan 2018 17:02:45 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 802B5C0567A2; Mon, 8 Jan 2018 17:02:44 +0000 (UTC) Received: from ovpn-116-33.phx2.redhat.com (ovpn-116-33.phx2.redhat.com [10.3.116.33]) by smtp.corp.redhat.com (Postfix) with ESMTP id D6FD65D964; Mon, 8 Jan 2018 17:02:43 +0000 (UTC) Message-ID: <1515430963.24844.56.camel@redhat.com> Subject: Location wrappers vs decls that change type (was Re: [v2 of PATCH 03/14] C++: add location_t wrapper nodes during parsing (minimal impl)) From: David Malcolm To: Jason Merrill Cc: Nathan Sidwell , Jakub Jelinek , Richard Biener , gcc-patches List Date: Mon, 08 Jan 2018 17:10:00 -0000 In-Reply-To: <1515190808.24844.23.camel@redhat.com> References: <1514567206-13093-1-git-send-email-dmalcolm@redhat.com> <1a3db854-830d-516c-61ed-ef503b47b946@redhat.com> <1515190808.24844.23.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00506.txt.bz2 On Fri, 2018-01-05 at 17:20 -0500, David Malcolm wrote: > On Fri, 2018-01-05 at 15:29 -0500, Jason Merrill wrote: > > On 12/29/2017 12:06 PM, David Malcolm wrote: > > > One issue I ran into was that fold_for_warn doesn't eliminate > > > location wrappers when processing_template_decl, leading to > > > failures of the template-based cases in > > > g++.dg/warn/Wmemset-transposed-args-1.C. > > > > > > This is due to the early bailout when processing_template_decl > > > within cp_fold: > > > > > > 2078 if (processing_template_decl > > > 2079 || (EXPR_P (x) && (!TREE_TYPE (x) || TREE_TYPE > > > (x) == error_mark_node))) > > > 2080 return x; > > > > > > which dates back to the merger of the C++ delayed folding branch. > > > > > > I've fixed that in this version of the patch by removing that > > > "processing_template_decl ||" condition from that cp_fold early > > > bailout. > > > > Hmm, that makes me nervous. We might want to fold in templates > > when > > called from fold_for_warn, but not for other occurrences. But I > > see > > that we check processing_template_decl in cp_fully_fold and in the > > call > > to cp_fold_function, so I guess this is fine. > > (I wondered if it would be possible to add a new flag to the various > fold* calls to request folding in templates, but given that the API > is > partially shared with C doing so doesn't seem to make sense) > > > > + case VIEW_CONVERT_EXPR: > > > + case NON_LVALUE_EXPR: > > > case CAST_EXPR: > > > case REINTERPRET_CAST_EXPR: > > > case CONST_CAST_EXPR: > > > @@ -14937,6 +14940,15 @@ tsubst_copy (tree t, tree args, > > > tsubst_flags_t complain, tree in_decl) > > > case CONVERT_EXPR: > > > case NOP_EXPR: > > > { > > > + if (location_wrapper_p (t)) > > > + { > > > + /* Handle location wrappers by substituting the > > > wrapped node > > > + first, *then* reusing the resulting type. Doing > > > the type > > > + first ensures that we handle template parameters > > > and > > > + parameter pack expansions. */ > > > + tree op0 = tsubst_copy (TREE_OPERAND (t, 0), args, > > > complain, in_decl); > > > + return build1 (code, TREE_TYPE (op0), op0); > > > + } > > > > I'd rather handle location wrappers separately, and abort if > > VIEW_CONVERT_EXPR or NON_LVALUE_EXPR appear other than as wrappers. > > OK. I'm testing an updated version which does so. Doing so uncovered an issue which I'm not sure how to resolve: it's possible for a decl to change type during parsing, after location wrappers may have been created, which changes location_wrapper_p on those wrappers from true to false. Here's the most minimal reproducer I've generated so far: 1 template 2 struct basic_string { 3 static const _CharT _S_terminal; 4 static void assign(const _CharT& __c2); 5 void _M_set_length_and_sharable() { 6 assign(_S_terminal); 7 } 8 }; 9 10 template 11 const _CharT basic_string<_CharT>::_S_terminal = _CharT(); 12 13 void getline(basic_string& __str) { 14 __str._M_set_length_and_sharable(); 15 } Asserting that the only VIEW_CONVERT_EXPR or NON_LVALUE_EXPR seen in tsubst_copy and tsubst_copy_and_build are location_wrapper_p leads to an ICE on the above code. What's happening is as follows. First, in the call: 6 assign(_S_terminal); ^~~~~~~~~~~ the VAR_DECL "_S_terminal" gains a VIEW_CONVERT_EXPR location wrapper node to express the underline shown above. Later, during parsing of this init-declarator: 10 template 11 const _CharT basic_string<_CharT>::_S_terminal = _CharT(); ^~~~~~~~~~~ ...cp_parser_init_declarator calls start_decl, which calls duplicate_decls, merging the "_S_terminal" seen here: 1 template 2 struct basic_string { 3 static const _CharT _S_terminal; ^~~~~~~~~~~ with that seen here: 10 template 11 const _CharT basic_string<_CharT>::_S_terminal = _CharT(); ^~~~~~~~~~~ Both "_S_terminal" VAR_DECLs have a "_CharT" TEMPLATE_TYPE_PARM, but these types are different tree nodes. Hence the type of the first VAR_DECL changes in duplicate_decls here: 2152 TREE_TYPE (newdecl) = TREE_TYPE (olddecl) = newtype; ...changing type to the TEMPLATE_TYPE_PARM of the second VAR_DECL. At this point, the location wrapper node at the callsite still has the *old* type, and hence location_wrapper_p (wrapper) changes from true to false, as its type no longer matches that of the decl it's wrapping. Hence my rewritten code in tsubst_copy_and_build fails the assertion here: 18306 case VIEW_CONVERT_EXPR: 18307 case NON_LVALUE_EXPR: 18308 gcc_assert (location_wrapper_p (t)); 18309 RETURN (RECUR (TREE_OPERAND (t, 0))); Assuming I'm correctly understanding the above, I'm not sure what the best solution is. Some ideas: * accept that this is a rare case, and either: * don't assert, or * also allow nodes here that are "nearly" location wrappers (i.e. of type TEMPLATE_TYPE_PARM) * some kind of fixup of location wrapper node types when changing the type of a decl (ugh) * don't add location wrappers if processing a template * introduce a new tree node for location wrappers (gah) * something I haven't thought of Thoughts? Thanks Dave [...snipped discussion of the other up-thread issue, relating build_non_dependent_expr...]