From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100996 invoked by alias); 24 Feb 2018 01:55:45 -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 100940 invoked by uid 89); 24 Feb 2018 01:55:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-io0-f179.google.com Received: from mail-io0-f179.google.com (HELO mail-io0-f179.google.com) (209.85.223.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 24 Feb 2018 01:55:37 +0000 Received: by mail-io0-f179.google.com with SMTP id p78so11700347iod.13 for ; Fri, 23 Feb 2018 17:55:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=MtPAEYNsyF4jCjtwjni3AOAigYxRWrDWQWYtO++ut40=; b=aZn8LHNOW2waXZ/gxdQh+ClC8OfDGHyW0Tghr92xlwRsin3cI7MLEd36RYwfJT/4dJ n16Fy3OXRkAmf6R12gzt31AHsyPTvdzy5nsOqItVNmQ3hEex4H5WyX6/xwfhf9iWdsLl c+jC9N2kJEG22x3LDBvSoqbPDMgvpEvUVovy+KUKxtC4sCkohc2bKwAATWJNhBWFXy3E MfrgsBCRBgHeNXCoJPHY+MouuzNn+7WOn7csA0wLpjUiqMqDBf8zevX5aR18x+ZCG2ND QiPR9Rlt/wK77bc4vY9oYLuhPiHiTmRVB2VN5VWqMqQBUl3QIDzYSGdz7pYQPz21f8aM BIoA== X-Gm-Message-State: APf1xPDfmt+exULkNjOaiWxJeqc+7ydBh/0mCiSEGtjidLXdu2jeAFE4 17pb6Tkr92pI8CMRaTrwywMXw3PuQ6hZH5XQ7vDgyA== X-Google-Smtp-Source: AG47ELuaRlldIa9zI9hR7DGoKmIYSUc14Ot6pnSkrVd7TKSPApXnapT0beHh6At/nEk14svXiglkG1mGKySBZ7FwC/k= X-Received: by 10.107.7.87 with SMTP id 84mr4160557ioh.216.1519437335188; Fri, 23 Feb 2018 17:55:35 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.17.200 with HTTP; Fri, 23 Feb 2018 17:55:14 -0800 (PST) In-Reply-To: <20180223142955.GB2995@redhat.com> References: <20180125211639.GA2620@redhat.com> <20180205133752.GF2608@redhat.com> <20180223142955.GB2995@redhat.com> From: Jason Merrill Date: Sat, 24 Feb 2018 01:55:00 -0000 Message-ID: Subject: Re: C++ PATCH to fix rejects-valid with constexpr ctor in C++17 (PR c++/83692) To: Marek Polacek Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg01363.txt.bz2 OK, thanks. On Fri, Feb 23, 2018 at 9:29 AM, Marek Polacek wrote: > On Fri, Feb 16, 2018 at 04:10:20PM -0500, Jason Merrill wrote: >> On Mon, Feb 5, 2018 at 1:45 PM, Jason Merrill wrote: >> > On Mon, Feb 5, 2018 at 8:37 AM, Marek Polacek wrote: >> >> On Fri, Feb 02, 2018 at 02:11:27PM -0500, Jason Merrill wrote: >> >>> On Thu, Jan 25, 2018 at 4:16 PM, Marek Polacek wrote: >> >>> > This is a similar problem to 83116: we'd cached a constexpr call, but after a >> >>> > store the result had become invalid, yet we used the wrong result again when >> >>> > encountering the same call later. This resulted in evaluating a THROW_EXPR >> >>> > which doesn't work. Details in >> >>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83692#c5 >> >>> > >> >>> > The fix for 83116 didn't work here, because when evaluating the body of the >> >>> > ctor via store_init_value -> cxx_constant_value we are in STRICT, so we do >> >>> > cache. >> >>> >> >>> > It seems that we may no longer rely on the constexpr call table when we >> >>> > do cxx_eval_store_expression, because that just rewrites *valp, i.e. the >> >>> > value of an object. Might be too big a hammer again, but I couldn't think >> >>> > of how I could guard the caching of a constexpr call. >> >>> >> >>> > This doesn't manifest in C++14 because build_special_member_call in C++17 is >> >>> > more aggressive with copy elisions (as required by P0135 which changed how we >> >>> > view prvalues). In C++14 build_special_member_call produces a CALL_EXPR, so >> >>> > expand_default_init calls maybe_constant_init, for which STRICT is false, so >> >>> > we avoid caching as per 83116. >> >>> >> >>> So it sounds like the problem is using cxx_constant_value for the >> >>> diagnostic when it has different semantics from the >> >>> maybe_constant_init that follows right after. I guess we want a >> >>> cxx_constant_init function that is a hybrid of the two. >> >> >> >> So like the following? Thanks, >> >> >> >> Bootstrapped/regtested on x86_64-linux, ok for trunk? >> >> >> >> 2018-02-04 Marek Polacek >> >> >> >> PR c++/83692 >> >> * constexpr.c (cxx_constant_init): New function. >> >> * cp-tree.h (cxx_constant_init): Declare. >> >> * typeck2.c (store_init_value): Call cxx_constant_init instead of >> >> cxx_constant_value. >> >> >> >> +/* Like cxx_constant_value, but non-strict mode. */ >> >> + >> >> +tree >> >> +cxx_constant_init (tree t, tree decl) >> >> +{ >> >> + return cxx_eval_outermost_constant_expr (t, false, false, decl); >> >> +} >> > >> > Hmm, that doesn't do the TARGET_EXPR stripping that >> > maybe_constant_init does. I was thinking of a version of >> > maybe_constant_init that passes false to allow_non_constant. Probably >> > by making "maybe_constant_init" and cxx_constant_init both call the >> > current function with an additional parameter. And then the existing >> > call to maybe_constant_init can move under an 'else' to avoid >> > redundant constexpr evaluation. >> >> Want me to take this over? > > Sorry again for the delay. > > I tried to do what you suggested. There was one twist: it regressed > constexpr-nullptr-2.C, in particular we lost diagnostics for > > constexpr int* pj0 = &((S*)0)->j; // { dg-error "not a constant expression" } > constexpr int* pj1 = &((S*)nullptr)->j; // { dg-error "not a constant expression" } > > because when maybe_constant_init_1 saw a constant: > > 5142 else if (CONSTANT_CLASS_P (t)) > 5143 /* No evaluation needed. */; > > so it didn't call cxx_eval_outermost_constant_expr which is supposed to give > the error. I fixed it by adding "&& allow_non_constant" so now it gives the > proper diagnostics. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2018-02-23 Marek Polacek > > PR c++/83692 > * constexpr.c (maybe_constant_init_1): New function. > (maybe_constant_init): Make it a wrapper around maybe_constant_init_1. > (cxx_constant_init): New function. > * cp-tree.h (cxx_constant_init): Declare. > * typeck2.c (store_init_value): Call cxx_constant_init instead of > cxx_constant_value. Move the maybe_constant_init call under an 'else'. > > * g++.dg/cpp1z/constexpr-83692.C: New test. > > diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c > index 47ff90cb055..26d0d099a05 100644 > --- gcc/cp/constexpr.c > +++ gcc/cp/constexpr.c > @@ -5123,8 +5123,8 @@ fold_non_dependent_expr (tree t) > /* Like maybe_constant_value, but returns a CONSTRUCTOR directly, rather > than wrapped in a TARGET_EXPR. */ > > -tree > -maybe_constant_init (tree t, tree decl) > +static tree > +maybe_constant_init_1 (tree t, tree decl, bool allow_non_constant) > { > if (!t) > return t; > @@ -5139,10 +5139,10 @@ maybe_constant_init (tree t, tree decl) > t = TARGET_EXPR_INITIAL (t); > if (!is_nondependent_static_init_expression (t)) > /* Don't try to evaluate it. */; > - else if (CONSTANT_CLASS_P (t)) > + else if (CONSTANT_CLASS_P (t) && allow_non_constant) > /* No evaluation needed. */; > else > - t = cxx_eval_outermost_constant_expr (t, true, false, decl); > + t = cxx_eval_outermost_constant_expr (t, allow_non_constant, false, decl); > if (TREE_CODE (t) == TARGET_EXPR) > { > tree init = TARGET_EXPR_INITIAL (t); > @@ -5152,6 +5152,22 @@ maybe_constant_init (tree t, tree decl) > return t; > } > > +/* Wrapper for maybe_constant_init_1 which permits non constants. */ > + > +tree > +maybe_constant_init (tree t, tree decl) > +{ > + return maybe_constant_init_1 (t, decl, true); > +} > + > +/* Wrapper for maybe_constant_init_1 which does not permit non constants. */ > + > +tree > +cxx_constant_init (tree t, tree decl) > +{ > + return maybe_constant_init_1 (t, decl, false); > +} > + > #if 0 > /* FIXME see ADDR_EXPR section in potential_constant_expression_1. */ > /* Return true if the object referred to by REF has automatic or thread > diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h > index 9038d677b2d..04c7b7ce3a9 100644 > --- gcc/cp/cp-tree.h > +++ gcc/cp/cp-tree.h > @@ -7411,6 +7411,7 @@ extern bool require_potential_constant_expression (tree); > extern bool require_constant_expression (tree); > extern bool require_potential_rvalue_constant_expression (tree); > extern tree cxx_constant_value (tree, tree = NULL_TREE); > +extern tree cxx_constant_init (tree, tree = NULL_TREE); > extern tree maybe_constant_value (tree, tree = NULL_TREE); > extern tree maybe_constant_init (tree, tree = NULL_TREE); > extern tree fold_non_dependent_expr (tree); > diff --git gcc/cp/typeck2.c gcc/cp/typeck2.c > index 899d60e8535..153b46cca77 100644 > --- gcc/cp/typeck2.c > +++ gcc/cp/typeck2.c > @@ -830,9 +830,10 @@ store_init_value (tree decl, tree init, vec** cleanups, int flags) > if (!require_constant_expression (value)) > value = error_mark_node; > else > - value = cxx_constant_value (value, decl); > + value = cxx_constant_init (value, decl); > } > - value = maybe_constant_init (value, decl); > + else > + value = maybe_constant_init (value, decl); > if (TREE_CODE (value) == CONSTRUCTOR && cp_has_mutable_p (type)) > /* Poison this CONSTRUCTOR so it can't be copied to another > constexpr variable. */ > diff --git gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > index e69de29bb2d..f6b61eeab85 100644 > --- gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > +++ gcc/testsuite/g++.dg/cpp1z/constexpr-83692.C > @@ -0,0 +1,21 @@ > +// PR c++/83692 > +// { dg-options -std=c++17 } > + > +struct integer { > + constexpr int value() const { return m_value; } > + int m_value; > +}; > + > +struct outer { > + integer m_x{0}; > + constexpr outer() > + { > + if (m_x.value() != 0) > + throw 0; > + m_x.m_value = integer{1}.value(); > + if (m_x.value() != 1) > + throw 0; > + } > +}; > + > +constexpr outer o{}; > > Marek