From patchwork Tue Aug 4 15:05:20 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Iain Buclaw X-Patchwork-Id: 1340896 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=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=gcc.gnu.org Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=fYYpsjT0; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BLdNZ5ywQz9sSt for ; Wed, 5 Aug 2020 01:05:33 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2746838708AF; Tue, 4 Aug 2020 15:05:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2746838708AF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1596553531; bh=ZM20zpOPc3g9fvgIblKxUC5916IXPuQu7ab4ti+KFR4=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=fYYpsjT0Fi+T4Cz+IFqVtZeC68G3K8+E174AjreXZcMaOgGCcHYgitrus/tnWFrG8 N6KscgS8kadjOdUwIgMIs32o/EBuEK5gARtgmHQHsvNckAiZW9gtMYjR/C8ha+6pVH aNn5E7Vh4SkGOHIt9c/R5/+TF081Nr1CeI1vJ82Q= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mout-p-202.mailbox.org (mout-p-202.mailbox.org [IPv6:2001:67c:2050::465:202]) by sourceware.org (Postfix) with ESMTPS id A9F4B387086C for ; Tue, 4 Aug 2020 15:05:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A9F4B387086C Received: from smtp2.mailbox.org (smtp2.mailbox.org [80.241.60.241]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by mout-p-202.mailbox.org (Postfix) with ESMTPS id 4BLdNQ24ByzQkmD; Tue, 4 Aug 2020 17:05:26 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp2.mailbox.org ([80.241.60.241]) by spamfilter01.heinlein-hosting.de (spamfilter01.heinlein-hosting.de [80.241.56.115]) (amavisd-new, port 10030) with ESMTP id 4hqg7QENjFEw; Tue, 4 Aug 2020 17:05:22 +0200 (CEST) To: gcc-patches@gcc.gnu.org Subject: [committed] d: Fix struct literals that have non-deterministic hash values (PR96153) Date: Tue, 4 Aug 2020 17:05:20 +0200 Message-Id: <20200804150520.3292579-1-ibuclaw@gdcproject.org> MIME-Version: 1.0 X-MBO-SPAM-Probability: 39 X-Rspamd-Score: 5.85 / 15.00 / 15.00 X-Rspamd-Queue-Id: F327E17A3 X-Rspamd-UID: bdbfd4 X-Spam-Status: No, score=-15.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, URIBL_ABUSE_SURBL, URIBL_RED autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Iain Buclaw via Gcc-patches From: Iain Buclaw Reply-To: Iain Buclaw Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" Hi, This patch adds code generation for generating a temporary for, and pre-filling struct and array literals with zeroes before assigning, so that alignment holes don't cause objects to produce a non-deterministic hash value. A new field has been added to the expression visitor to track whether the result is being generated for another literal, so that memset() is only called once on the top-level literal expression, and not for nesting struct or arrays. Bootstrapped and regression tested on x86_64-linux-gnu with multilib configurations -m32 and -mx32. Committed to mainline. Regards Iain --- gcc/d/ChangeLog: PR d/96153 * d-tree.h (build_expr): Add literalp argument. * expr.cc (ExprVisitor): Add literalp_ field. (ExprVisitor::ExprVisitor): Initialize literalp_. (ExprVisitor::visit (AssignExp *)): Call memset() on blits where RHS is a struct literal. Elide assignment if initializer is all zeroes. (ExprVisitor::visit (CastExp *)): Forward literalp_ to generation of subexpression. (ExprVisitor::visit (AddrExp *)): Likewise. (ExprVisitor::visit (ArrayLiteralExp *)): Use memset() to pre-fill object with zeroes. Set literalp in subexpressions. (ExprVisitor::visit (StructLiteralExp *)): Likewise. (ExprVisitor::visit (TupleExp *)): Set literalp in subexpressions. (ExprVisitor::visit (VectorExp *)): Likewise. (ExprVisitor::visit (VectorArrayExp *)): Likewise. (build_expr): Forward literal_p to ExprVisitor. gcc/testsuite/ChangeLog: PR d/96153 * gdc.dg/pr96153.d: New test. --- gcc/d/d-tree.h | 2 +- gcc/d/expr.cc | 104 ++++++++++++++++++++++----------- gcc/testsuite/gdc.dg/pr96153.d | 31 ++++++++++ 3 files changed, 101 insertions(+), 36 deletions(-) create mode 100644 gcc/testsuite/gdc.dg/pr96153.d diff --git a/gcc/d/d-tree.h b/gcc/d/d-tree.h index 2be80dd1867..072de7e6543 100644 --- a/gcc/d/d-tree.h +++ b/gcc/d/d-tree.h @@ -633,7 +633,7 @@ extern void d_comdat_linkage (tree); extern void d_linkonce_linkage (tree); /* In expr.cc. */ -extern tree build_expr (Expression *, bool = false); +extern tree build_expr (Expression *, bool = false, bool = false); extern tree build_expr_dtor (Expression *); extern tree build_return_dtor (Expression *, Type *, TypeFunction *); diff --git a/gcc/d/expr.cc b/gcc/d/expr.cc index ac3d4aaa171..85407ac7eb0 100644 --- a/gcc/d/expr.cc +++ b/gcc/d/expr.cc @@ -223,12 +223,14 @@ class ExprVisitor : public Visitor tree result_; bool constp_; + bool literalp_; public: - ExprVisitor (bool constp) + ExprVisitor (bool constp, bool literalp) { this->result_ = NULL_TREE; this->constp_ = constp; + this->literalp_ = literalp; } tree result (void) @@ -1072,7 +1074,7 @@ public: if (tb1->ty == Tstruct) { tree t1 = build_expr (e->e1); - tree t2 = convert_for_assignment (build_expr (e->e2), + tree t2 = convert_for_assignment (build_expr (e->e2, false, true), e->e2->type, e->e1->type); StructDeclaration *sd = tb1->isTypeStruct ()->sym; @@ -1101,11 +1103,22 @@ public: tree init = NULL_TREE; /* Fill any alignment holes in the struct using memset. */ - if (e->op == TOKconstruct && !identity_compare_p (sd)) - init = build_memset_call (t1); + if ((e->op == TOKconstruct + || (e->e2->op == TOKstructliteral && e->op == TOKblit)) + && (sd->isUnionDeclaration () || !identity_compare_p (sd))) + { + t1 = stabilize_reference (t1); + init = build_memset_call (t1); + } - tree result = build_assign (modifycode, t1, t2); - this->result_ = compound_expr (init, result); + /* Elide generating assignment if init is all zeroes. */ + if (init != NULL_TREE && initializer_zerop (t2)) + this->result_ = compound_expr (init, t1); + else + { + tree result = build_assign (modifycode, t1, t2); + this->result_ = compound_expr (init, result); + } } return; @@ -1135,6 +1148,7 @@ public: to call postblits, this assignment should call dtors on old assigned elements. */ if ((!postblit && !destructor) + || (e->op == TOKconstruct && e->e2->op == TOKarrayliteral) || (e->op == TOKconstruct && !lvalue && postblit) || (e->op == TOKblit || e->e1->type->size () == 0)) { @@ -1452,7 +1466,7 @@ public: { Type *ebtype = e->e1->type->toBasetype (); Type *tbtype = e->to->toBasetype (); - tree result = build_expr (e->e1, this->constp_); + tree result = build_expr (e->e1, this->constp_, this->literalp_); /* Just evaluate e1 if it has any side effects. */ if (tbtype->ty == Tvoid) @@ -1702,7 +1716,7 @@ public: exp = sle->sym; } else - exp = build_expr (e->e1, this->constp_); + exp = build_expr (e->e1, this->constp_, this->literalp_); TREE_CONSTANT (exp) = 0; this->result_ = d_convert (type, build_address (exp)); @@ -2663,12 +2677,12 @@ public: tree result = NULL_TREE; if (e->e0) - result = build_expr (e->e0); + result = build_expr (e->e0, this->constp_, true); for (size_t i = 0; i < e->exps->length; ++i) { Expression *exp = (*e->exps)[i]; - result = compound_expr (result, build_expr (exp)); + result = compound_expr (result, build_expr (exp, this->constp_, true)); } if (result == NULL_TREE) @@ -2717,7 +2731,7 @@ public: for (size_t i = 0; i < e->elements->length; i++) { Expression *expr = e->getElement (i); - tree value = build_expr (expr, this->constp_); + tree value = build_expr (expr, this->constp_, true); /* Only append nonzero values, the backend will zero out the rest of the constructor as we don't set CONSTRUCTOR_NO_CLEARING. */ @@ -2765,6 +2779,22 @@ public: if (constant_p && initializer_constant_valid_p (ctor, TREE_TYPE (ctor))) TREE_STATIC (ctor) = 1; + /* Use memset to fill any alignment holes in the array. */ + if (!this->constp_ && !this->literalp_) + { + TypeStruct *ts = etype->baseElemOf ()->isTypeStruct (); + + if (ts != NULL && (!identity_compare_p (ts->sym) + || ts->sym->isUnionDeclaration ())) + { + tree var = build_local_temp (TREE_TYPE (ctor)); + tree init = build_memset_call (var); + /* Evaluate memset() first, then any saved elements. */ + saved_elems = compound_expr (init, saved_elems); + ctor = compound_expr (modify_expr (var, ctor), var); + } + } + this->result_ = compound_expr (saved_elems, d_convert (type, ctor)); } else @@ -2885,7 +2915,8 @@ public: if (ftype->ty == Tsarray && !same_type_p (type, ftype)) { /* Initialize a static array with a single element. */ - tree elem = build_expr (exp, this->constp_); + tree elem = build_expr (exp, this->constp_, true); + saved_elems = compound_expr (saved_elems, stabilize_expr (&elem)); elem = d_save_expr (elem); if (initializer_zerop (elem)) @@ -2895,14 +2926,12 @@ public: } else { - value = convert_expr (build_expr (exp, this->constp_), + value = convert_expr (build_expr (exp, this->constp_, true), exp->type, field->type); } /* Split construction of values out of the constructor. */ - tree init = stabilize_expr (&value); - if (init != NULL_TREE) - saved_elems = compound_expr (saved_elems, init); + saved_elems = compound_expr (saved_elems, stabilize_expr (&value)); CONSTRUCTOR_APPEND_ELT (ve, get_symbol_decl (field), value); } @@ -2932,24 +2961,27 @@ public: return; } + /* Construct the struct literal for run-time. */ if (e->sym != NULL) { + /* Store the result in a symbol to initialize the literal. */ tree var = build_deref (e->sym); ctor = compound_expr (modify_expr (var, ctor), var); - this->result_ = compound_expr (saved_elems, ctor); } - else if (e->sd->isUnionDeclaration ()) + else if (!this->literalp_) { - /* For unions, use memset to fill holes in the object. */ - tree var = build_local_temp (TREE_TYPE (ctor)); - tree init = build_memset_call (var); - - init = compound_expr (init, saved_elems); - init = compound_expr (init, modify_expr (var, ctor)); - this->result_ = compound_expr (init, var); + /* Use memset to fill any alignment holes in the object. */ + if (!identity_compare_p (e->sd) || e->sd->isUnionDeclaration ()) + { + tree var = build_local_temp (TREE_TYPE (ctor)); + tree init = build_memset_call (var); + /* Evaluate memset() first, then any saved element constructors. */ + saved_elems = compound_expr (init, saved_elems); + ctor = compound_expr (modify_expr (var, ctor), var); + } } - else - this->result_ = compound_expr (saved_elems, ctor); + + this->result_ = compound_expr (saved_elems, ctor); } /* Build a null literal. */ @@ -2964,7 +2996,6 @@ public: void visit (VectorExp *e) { tree type = build_ctype (e->type); - tree etype = TREE_TYPE (type); /* First handle array literal expressions. */ if (e->e1->op == TOKarrayliteral) @@ -2977,7 +3008,8 @@ public: for (size_t i = 0; i < ale->elements->length; i++) { Expression *expr = ale->getElement (i); - tree value = d_convert (etype, build_expr (expr, this->constp_)); + tree value = d_convert (TREE_TYPE (type), + build_expr (expr, this->constp_, true)); if (!CONSTANT_CLASS_P (value)) constant_p = false; @@ -2993,8 +3025,9 @@ public: else { /* Build constructor from single value. */ - tree val = d_convert (etype, build_expr (e->e1, this->constp_)); - this->result_ = build_vector_from_val (type, val); + tree value = d_convert (TREE_TYPE (type), + build_expr (e->e1, this->constp_, true)); + this->result_ = build_vector_from_val (type, value); } } @@ -3002,7 +3035,7 @@ public: void visit (VectorArrayExp *e) { - this->result_ = convert_expr (build_expr (e->e1, this->constp_), + this->result_ = convert_expr (build_expr (e->e1, this->constp_, true), e->e1->type, e->type); } @@ -3057,12 +3090,13 @@ public: /* Main entry point for ExprVisitor interface to generate code for the Expression AST class E. If CONST_P is true, then E is a - constant expression. */ + constant expression. If LITERAL_P is true, then E is a value used + in the initialization of another literal. */ tree -build_expr (Expression *e, bool const_p) +build_expr (Expression *e, bool const_p, bool literal_p) { - ExprVisitor v = ExprVisitor (const_p); + ExprVisitor v = ExprVisitor (const_p, literal_p); location_t saved_location = input_location; input_location = make_location_t (e->loc); diff --git a/gcc/testsuite/gdc.dg/pr96153.d b/gcc/testsuite/gdc.dg/pr96153.d new file mode 100644 index 00000000000..c0e3ae024f5 --- /dev/null +++ b/gcc/testsuite/gdc.dg/pr96153.d @@ -0,0 +1,31 @@ +// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96153 +// { dg-additional-options "-fmain -funittest" } +// { dg-do run { target hw } } +// { dg-skip-if "needs gcc/config.d" { ! d_runtime } } +struct Checked(T, Hook) +{ + private T payload; + Hook hook; + + size_t toHash() const nothrow @safe + { + return hashOf(payload) ^ hashOf(hook); + } +} + +Checked!(T, Hook) checked(Hook, T)(const T value) +{ + return Checked!(T, Hook)(value); +} + +@system unittest +{ + static struct Hook3 + { + ulong var1 = ulong.max; + uint var2 = uint.max; + } + + assert(checked!Hook3(12).toHash() != checked!Hook3(13).toHash()); + assert(checked!Hook3(13).toHash() == checked!Hook3(13).toHash()); +}