From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82749 invoked by alias); 8 Jan 2018 21:01:53 -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 82739 invoked by uid 89); 8 Jan 2018 21:01:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,KAM_SHORT,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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; Mon, 08 Jan 2018 21:01:51 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1BE4E8B112; Mon, 8 Jan 2018 21:01:50 +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 517C360C91; Mon, 8 Jan 2018 21:01:49 +0000 (UTC) Message-ID: <1515445308.24844.84.camel@redhat.com> Subject: 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 21:03:00 -0000 In-Reply-To: <1a3db854-830d-516c-61ed-ef503b47b946@redhat.com> References: <1514567206-13093-1-git-send-email-dmalcolm@redhat.com> <1a3db854-830d-516c-61ed-ef503b47b946@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/msg00538.txt.bz2 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. > > > + 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. Once I fixed the issue with location_wrapper_p with decls changing type, it turns out that trunk is already passing VIEW_CONVERT_EXPR to tsubst_copy_and_build for non-wrapper nodes (and from there to tsubst_copy), where the default case currently handles them. Adding an assert turns this into an ICE. g++.dg/delayedfold/builtin1.C is the only instance of it I found in our test suite, where it's used here: class RegionLock { template void m_fn1(); int spinlock; } acquire_zero; int acquire_one; template void RegionLock::m_fn1() { __atomic_compare_exchange(&spinlock, &acquire_zero, &acquire_one, false, 2, 2); ^~~~~~~~~~~~~ } (gdb) call debug_tree (t) unit-size align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff18c9690 precision:32 min max pointer_to_this > unsigned type_6 DI size unit-size align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical- type 0x7ffff18d93f0> arg:0 unsigned type_6 DI size unit-size align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff1a18150> arg:0 arg:0 ../../src/gcc/testsuite/g++.dg/delayedfold/builtin1.C:10:40 start: ../../src/gcc/testsuite/g++.dg/delayedfold/builtin1.C:10:40 finish: ../../src/gcc/testsuite/g++.dg/delayedfold/builtin1.C:10:52>>> (This one is just for VIEW_CONVERT_EXPR; I don't yet know of any existing places where NON_LVALUE_EXPR can be passed to tsubst_*). Given that, is it OK to go with the approach in this (v2) patch? (presumably requiring the fix to location_wrapper_p to use a flag rather than a matching type). > > @@ -24998,6 +25018,8 @@ build_non_dependent_expr (tree expr) > > && !expanding_concept ()) > > fold_non_dependent_expr (expr); > > > > + STRIP_ANY_LOCATION_WRAPPER (expr); > > Why is this needed? https://gcc.gnu.org/ml/gcc-patches/2018-01/msg00349.html > Jason Thanks Dave