From patchwork Tue Dec 3 08:42:08 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 1203560 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-515027-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="EhPangU+"; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="OoDtJou9"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47RwTc2498z9sP6 for ; Tue, 3 Dec 2019 19:42:26 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:mime-version :content-type:content-transfer-encoding; q=dns; s=default; b=BPL PvjMkrn/DrhIj7CiMTB235suCOGCdGdHClqoyCMd/zgpS7tCTbNg9RkVYpDWbeRN XJrhfsO1HNLH9Nw0G/OphI3siHeB02iu0t74abEpkUa2+iPQZlDt7s3TA/kafw9W Jri/2J/LcnSc0MCOK6ruiALI11JyT1kQmcXUlfg8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:mime-version :content-type:content-transfer-encoding; s=default; bh=5Topdj1pP tX+QLzm+Wu61AfHeZY=; b=EhPangU+3LJeykOhewUXLqPk1di8SDscnqOqfwPOT 99k2rcM1/Qnl1wY7UuiHUmSpF0mKpTVTSyy5A6KpUzYbEuOe1Z7aXF/JGsPbryra 50ZYP5BqStkcGrNJFB9gwjTujkUDYfPC8Gp9h+fLc/iEQcs3PYJMg7VkOOVQ/hk3 FY= Received: (qmail 114431 invoked by alias); 3 Dec 2019 08:42:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 114414 invoked by uid 89); 3 Dec 2019 08:42:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-7.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=evaluate X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Dec 2019 08:42:16 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575362534; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=cf6p1vJv58IZsdJ4MlNYZvmUAxGYta6OfMD890KjMuE=; b=OoDtJou953fdl6CIT0dfYOS42MhSJZlZ/JbOQkQFjj3YhpSPuH52Tbc4VAhpz7W+j1xyy6 PNM2vv937FuMmAFkBXfQPBCj8zniMD4EjqSa6JultqD1+GnlmkvP8ZBgn0LLzododW5MzF 7/5ypHmvWmXFAcIub/jn4Ohfu92QwH0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-45-vQP96zhnOWS2DwQYVjikHg-1; Tue, 03 Dec 2019 03:42:13 -0500 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id 53725102C88C for ; Tue, 3 Dec 2019 08:42:12 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-117-59.ams2.redhat.com [10.36.117.59]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D9BE05DA60; Tue, 3 Dec 2019 08:42:11 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id xB38g9xw032614; Tue, 3 Dec 2019 09:42:09 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id xB38g8Fo032613; Tue, 3 Dec 2019 09:42:08 +0100 Date: Tue, 3 Dec 2019 09:42:08 +0100 From: Jakub Jelinek To: Jason Merrill Cc: gcc-patches@gcc.gnu.org Subject: [C++ PATCH] Fix up constexpr TARGET_EXPR evaluation if there are cleanups (PR c++/91369) Message-ID: <20191203084208.GB10088@tucnak> Reply-To: Jakub Jelinek MIME-Version: 1.0 User-Agent: Mutt/1.11.3 (2019-02-01) X-Mimecast-Spam-Score: 0 Content-Disposition: inline X-IsSubscribed: yes Hi! The following testcase shows that during constexpr evaluation we didn't handle TARGET_EXPR_CLEANUP at all (which was probably fine when there weren't constexpr dtors). My understanding is that TARGET_EXPR cleanups should be queued and evaluated only at the end of CLEANUP_POINT_EXPR, so that is what the patch implements. Regtesting of the first iteration revealed quite some FAILs in libstdc++, my assumption that a TARGET_EXPR with cleanups will always appear inside of some CLEANUP_POINT_EXPR is violated e.g. when cp_fold attempts to evaluate some call expressions with TARGET_EXPR in arguments, so I had to add evaluating of cleanups in cxx_eval_outermost_constant_expr too as if the outermost expression were wrapped into another CLEANUP_POINT_EXPR. There was another issue only seen in libstdc++, in particular, TRY_CATCH_EXPR created by wrap_cleanups_r can sometimes have NULL_TREE first argument. Furthermore (though just theoretical, I haven't tried to create a testcase), I believe a TARGET_EXPR shuld behave similarly to SAVE_EXPR that if the same TARGET_EXPR tree is encountered multiple times, the initializer expression is evaluated just once, not multiple times. Maybe it didn't matter before if things were evaluated multiple times, but e.g. with constexpr new/delete, evaluating new multiple times without corresponding deletes is incorrect, so in the patch I've tried to handle caching of the result and reuse of it once it has been evaluated already (with pushing into save_exprs so that e.g. when evaluating a loop second time or a function we undo the caching). Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Note, the description of CLEANUP_POINT_EXPR in tree.def makes me worry a little bit, with the: " Note that if the expression is a reference to storage, it is forced out of memory before the cleanups are run. This is necessary to handle cases where the cleanups modify the storage referenced; in the expression 't.i', if 't' is a struct with an integer member 'i' and a cleanup which modifies 'i', the value of the expression depends on whether the cleanup is run before or after 't.i' is evaluated. When expand_expr is run on 't.i', it returns a MEM. This is not good enough; the value of 't.i' must be forced out of memory." note. Does that mean the CLEANUP_POINT_EXPR handling should do unshare_constructor on the result if there are any cleanups (and similarly outermost expr handling)? Don't want on the other side to waste too much memory if it is not necessary... 2019-12-03 Jakub Jelinek PR c++/91369 * constexpr.c (struct constexpr_global_ctx): Add cleanups member, initialize it in the ctor. (cxx_eval_constant_expression) : If TARGET_EXPR_SLOT is already in the values hash_map, don't evaluate it again. Put TARGET_EXPR_SLOT into hash_map even if not lval, and push it into save_exprs too. If there is TARGET_EXPR_CLEANUP and not CLEANUP_EH_ONLY, push the cleanup to cleanups vector. : Save outer cleanups, set cleanups to local auto_vec, after evaluating the body evaluate cleanups and restore previous cleanups. : Don't crash if the first operand is NULL_TREE. (cxx_eval_outermost_constant_expr): Set cleanups to local auto_vec, after evaluating the expression evaluate cleanups. * g++.dg/cpp2a/constexpr-new8.C: New test. Jakub --- gcc/cp/constexpr.c.jj 2019-11-28 09:02:21.896897228 +0100 +++ gcc/cp/constexpr.c 2019-12-03 01:14:06.674952015 +0100 @@ -1025,8 +1025,10 @@ struct constexpr_global_ctx { /* Heap VAR_DECLs created during the evaluation of the outermost constant expression. */ auto_vec heap_vars; + /* Cleanups that need to be evaluated at the end of CLEANUP_POINT_EXPR. */ + vec *cleanups; /* Constructor. */ - constexpr_global_ctx () : constexpr_ops_count (0) {} + constexpr_global_ctx () : constexpr_ops_count (0), cleanups (NULL) {} }; /* The constexpr expansion context. CALL is the current function @@ -4984,6 +4986,14 @@ cxx_eval_constant_expression (const cons *non_constant_p = true; break; } + /* Avoid evaluating a TARGET_EXPR more than once. */ + if (tree *p = ctx->global->values.get (TARGET_EXPR_SLOT (t))) + { + if (lval) + return TARGET_EXPR_SLOT (t); + r = *p; + break; + } if ((AGGREGATE_TYPE_P (TREE_TYPE (t)) || VECTOR_TYPE_P (TREE_TYPE (t)))) { /* We're being expanded without an explicit target, so start @@ -5004,13 +5014,14 @@ cxx_eval_constant_expression (const cons if (!*non_constant_p) /* Adjust the type of the result to the type of the temporary. */ r = adjust_temp_type (TREE_TYPE (t), r); + if (TARGET_EXPR_CLEANUP (t) && !CLEANUP_EH_ONLY (t)) + ctx->global->cleanups->safe_push (TARGET_EXPR_CLEANUP (t)); + r = unshare_constructor (r); + ctx->global->values.put (TARGET_EXPR_SLOT (t), r); + if (ctx->save_exprs) + ctx->save_exprs->safe_push (TARGET_EXPR_SLOT (t)); if (lval) - { - tree slot = TARGET_EXPR_SLOT (t); - r = unshare_constructor (r); - ctx->global->values.put (slot, r); - return slot; - } + return TARGET_EXPR_SLOT (t); break; case INIT_EXPR: @@ -5060,10 +5071,15 @@ cxx_eval_constant_expression (const cons } break; - case NON_LVALUE_EXPR: case TRY_CATCH_EXPR: + if (TREE_OPERAND (t, 0) == NULL_TREE) + { + r = void_node; + break; + } + /* FALLTHRU */ + case NON_LVALUE_EXPR: case TRY_BLOCK: - case CLEANUP_POINT_EXPR: case MUST_NOT_THROW_EXPR: case EXPR_STMT: case EH_SPEC_BLOCK: @@ -5073,6 +5089,26 @@ cxx_eval_constant_expression (const cons jump_target); break; + case CLEANUP_POINT_EXPR: + { + auto_vec cleanups; + vec *prev_cleanups = ctx->global->cleanups; + ctx->global->cleanups = &cleanups; + r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), + lval, + non_constant_p, overflow_p, + jump_target); + ctx->global->cleanups = prev_cleanups; + unsigned int i; + tree cleanup; + /* Evaluate the cleanups. */ + FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup) + cxx_eval_constant_expression (ctx, cleanup, false, + non_constant_p, overflow_p, + jump_target); + } + break; + case TRY_FINALLY_EXPR: r = cxx_eval_constant_expression (ctx, TREE_OPERAND (t, 0), lval, non_constant_p, overflow_p, @@ -5883,6 +5919,9 @@ cxx_eval_outermost_constant_expr (tree t r = TARGET_EXPR_INITIAL (r); } + auto_vec cleanups; + global_ctx.cleanups = &cleanups; + instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); @@ -5892,6 +5931,13 @@ cxx_eval_outermost_constant_expr (tree t else DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (object) = true; + unsigned int i; + tree cleanup; + /* Evaluate the cleanups. */ + FOR_EACH_VEC_ELT_REVERSE (cleanups, i, cleanup) + cxx_eval_constant_expression (&ctx, cleanup, false, + &non_constant_p, &overflow_p); + /* Mutable logic is a bit tricky: we want to allow initialization of constexpr variables with mutable members, but we can't copy those members to another constexpr variable. */ --- gcc/testsuite/g++.dg/cpp2a/constexpr-new8.C.jj 2019-12-02 11:48:58.595976239 +0100 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-new8.C 2019-12-02 11:51:34.137550815 +0100 @@ -0,0 +1,18 @@ +// PR c++/91369 +// { dg-do compile { target c++2a } } + +struct A { + constexpr A () : p{new int} {} + constexpr ~A () { delete p; } + int *p; +}; + +constexpr bool +test () +{ + A{}; + return true; +} + +constexpr auto res = test (); +static_assert (res);