From patchwork Thu Dec 19 10:55:03 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Botcazou X-Patchwork-Id: 303285 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 140AE2C0085 for ; Thu, 19 Dec 2013 21:57:06 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; q=dns; s= default; b=O/v9SAxbRL9JHBpbQRcXxl9X3lNg5ITNwoV3YsZNdIk6GVIWCHA1U 3ByW+yVQ74FQGX68RrRMWGvD6W7D5y9YUubdma4UB2KxhbwlSMIQwpchIYnhrLgB xc3sb+ohFwgbzE+yXXr/kWG5vu2S56m/vFuVEIBqKg928TvoeGd8M4= 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:from :to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; s=default; bh=+FhVxcNRjDqZ8wIrOVWD6mc+ZXQ=; b=oMxw7VTWYz2cm+/Vdyyz2Ok9YNvC l0Avt99SO+cvuoCOil6bNFmPIYbYz8OmGWxwVMPtcg0v/WvK0DK1mAtrDeAcyJ9H BlQsx3Evmm/wCOyGR57uVbpSKgbxhyxSctuFCBtmil2yPqOkjyAGbSbVm5KuFAF5 E2L0FDazIyyrfXc= Received: (qmail 5865 invoked by alias); 19 Dec 2013 10:57:00 -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 5856 invoked by uid 89); 19 Dec 2013 10:56:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: smtp.eu.adacore.com Received: from mel.act-europe.fr (HELO smtp.eu.adacore.com) (194.98.77.210) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 19 Dec 2013 10:56:58 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id D40B226C114A; Thu, 19 Dec 2013 11:56:54 +0100 (CET) Received: from smtp.eu.adacore.com ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id a1E03LuQWLbM; Thu, 19 Dec 2013 11:56:54 +0100 (CET) Received: from polaris.localnet (bon31-6-88-161-99-133.fbx.proxad.net [88.161.99.133]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by smtp.eu.adacore.com (Postfix) with ESMTPSA id 6EF2E26C0C16; Thu, 19 Dec 2013 11:56:54 +0100 (CET) From: Eric Botcazou To: Bernd Edlinger Cc: gcc-patches@gcc.gnu.org, Richard Biener , Jakub Jelinek , Jeff Law , "Joseph S. Myers" Subject: Re: [REPOST] Invalid Code when reading from unaligned zero-sized array Date: Thu, 19 Dec 2013 11:55:03 +0100 Message-ID: <3651709.ldS2nsTtfa@polaris> User-Agent: KMail/4.7.2 (Linux/3.1.10-1.29-desktop; KDE/4.7.2; x86_64; ; ) In-Reply-To: References: <4706821.H8prJJZFhJ@polaris> MIME-Version: 1.0 > In general I like the comment, and I am open for other suggestions how > to call the parameter. I think that using EXPAND in the parameter's name is confusing because it needs to be distinguished from MODIFIER and its enumeration type. And since it would be true only after calling get_inner_reference, it would be better to have the "inner" as well. Maybe inner_reference_p or something equivalent. For the record, I have also attached a patch that implements proposal #1, but it's lightly tested for the time being. * tree-core.h (tree_type_common): Rename no_force_blk_flag into blkmode_flag. * tree.h (TYPE_NO_FORCE_BLK): Delete. (TYPE_BLKMODE_FLAG): New macro. (TYPE_NO_FORCE_BLKMODE, SET_TYPE_NO_FORCE_BLKMODE): Likewise. (TYPE_BLKMODE_FOR_ACCESS, SET_TYPE_BLKMODE_FOR_ACCESS): Likewise. * lto-streamer-out.c (hash_tree): Adjust. * tree-streamer-out.c (pack_ts_type_common_value_fields): Likewise. * tree-streamer-in.c (unpack_ts_type_common_value_fields): Likewise. * print-tree.c (print_node): Likewise. * cfgexpand.c (expand_call_stmt): Call expand_expr. * expr.h (enum expand_modifier): Enhance comment. * expr.c (expand_expr_real_1) : Do not realign if the type has TYPE_BLKMODE_FOR_ACCESS. : Likewise. : Factor out realignment code and disable it for EXPAND_{WRITE,MEMORY} or if the type has TYPE_BLKMODE_FOR_ACCESS. * stor-layout.c (compute_record_mode): Do not special-case BLKmode fields with zero size. Compute TYPE_BLKMODE_FOR_ACCESS and adjust. (layout_type): Adjust. lto/ * lto.c (compare_tree_sccs_1): Adjust. Index: tree.h =================================================================== --- tree.h (revision 206105) +++ tree.h (working copy) @@ -1589,11 +1589,18 @@ extern void protected_set_expr_location get one debug info record for them. */ #define TYPE_STUB_DECL(NODE) (TREE_CHAIN (TYPE_CHECK (NODE))) -/* In a RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE or ARRAY_TYPE, it means - the type has BLKmode only because it lacks the alignment required for - its size. */ -#define TYPE_NO_FORCE_BLK(NODE) \ - (TYPE_CHECK (NODE)->type_common.no_force_blk_flag) +/* In a RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE or ARRAY_TYPE with BLKmode, + it means the type has BLKmode only because it lacks the alignment required + for its size. In a RECORD_TYPE, UNION_TYPE, QUAL_UNION_TYPE or ARRAY_TYPE + with non-BLKmode, it means the type should have had BLKmode because out of + bounds access (but didn't because of backwards compatibility reasons). */ +#define TYPE_BLKMODE_FLAG(NODE) (TYPE_CHECK (NODE)->type_common.blkmode_flag) +#define TYPE_NO_FORCE_BLKMODE(NODE) \ + (TYPE_MODE (TYPE_CHECK (NODE)) == BLKmode && TYPE_BLKMODE_FLAG (NODE)) +#define SET_TYPE_NO_FORCE_BLKMODE(NODE, X) (TYPE_BLKMODE_FLAG (NODE) = (X)) +#define TYPE_BLKMODE_FOR_ACCESS(NODE) \ + (TYPE_MODE (TYPE_CHECK (NODE)) != BLKmode && TYPE_BLKMODE_FLAG (NODE)) +#define SET_TYPE_BLKMODE_FOR_ACCESS(NODE, X) (TYPE_BLKMODE_FLAG (NODE) = (X)) /* Nonzero in a type considered volatile as a whole. */ #define TYPE_VOLATILE(NODE) (TYPE_CHECK (NODE)->base.volatile_flag) Index: lto-streamer-out.c =================================================================== --- lto-streamer-out.c (revision 206105) +++ lto-streamer-out.c (working copy) @@ -868,7 +868,7 @@ hash_tree (struct streamer_tree_cache_d { v = iterative_hash_host_wide_int (TYPE_MODE (t), v); v = iterative_hash_host_wide_int (TYPE_STRING_FLAG (t) - | (TYPE_NO_FORCE_BLK (t) << 1) + | (TYPE_BLKMODE_FLAG (t) << 1) | (TYPE_NEEDS_CONSTRUCTING (t) << 2) | (TYPE_PACKED (t) << 3) | (TYPE_RESTRICT (t) << 4) Index: expr.c =================================================================== --- expr.c (revision 206105) +++ expr.c (working copy) @@ -9656,6 +9656,7 @@ expand_expr_real_1 (tree exp, rtx target if (modifier != EXPAND_WRITE && modifier != EXPAND_MEMORY && mode != BLKmode + && !TYPE_BLKMODE_FOR_ACCESS (type) && align < GET_MODE_ALIGNMENT (mode) /* If the target does not have special handling for unaligned loads of mode then it can use regular moves for them. */ @@ -9736,6 +9737,7 @@ expand_expr_real_1 (tree exp, rtx target if (modifier != EXPAND_WRITE && modifier != EXPAND_MEMORY && mode != BLKmode + && !TYPE_BLKMODE_FOR_ACCESS (type) && align < GET_MODE_ALIGNMENT (mode)) { if ((icode = optab_handler (movmisalign_optab, mode)) @@ -10426,50 +10428,53 @@ expand_expr_real_1 (tree exp, rtx target op0 = copy_rtx (op0); set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type))); } - else if (mode != BLKmode - && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode) - /* If the target does have special handling for unaligned - loads of mode then use them. */ - && ((icode = optab_handler (movmisalign_optab, mode)) - != CODE_FOR_nothing)) - { - rtx reg, insn; - - op0 = adjust_address (op0, mode, 0); - /* We've already validated the memory, and we're creating a - new pseudo destination. The predicates really can't - fail. */ - reg = gen_reg_rtx (mode); - - /* Nor can the insn generator. */ - insn = GEN_FCN (icode) (reg, op0); - emit_insn (insn); - return reg; - } - else if (STRICT_ALIGNMENT + else if (modifier != EXPAND_WRITE + && modifier != EXPAND_MEMORY && mode != BLKmode + && !TYPE_BLKMODE_FOR_ACCESS (type) && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)) { - tree inner_type = TREE_TYPE (treeop0); - HOST_WIDE_INT temp_size - = MAX (int_size_in_bytes (inner_type), - (HOST_WIDE_INT) GET_MODE_SIZE (mode)); - rtx new_rtx - = assign_stack_temp_for_type (mode, temp_size, type); - rtx new_with_op0_mode - = adjust_address (new_rtx, GET_MODE (op0), 0); - - gcc_assert (!TREE_ADDRESSABLE (exp)); - - if (GET_MODE (op0) == BLKmode) - emit_block_move (new_with_op0_mode, op0, - GEN_INT (GET_MODE_SIZE (mode)), - (modifier == EXPAND_STACK_PARM - ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL)); - else - emit_move_insn (new_with_op0_mode, op0); + /* If the target does have special handling for unaligned + loads of mode then use them. */ + if ((icode = optab_handler (movmisalign_optab, mode)) + != CODE_FOR_nothing) + { + rtx reg, insn; - op0 = new_rtx; + op0 = adjust_address (op0, mode, 0); + /* We've already validated the memory, and we're creating a + new pseudo destination. The predicates really can't + fail. */ + reg = gen_reg_rtx (mode); + + /* Nor can the insn generator. */ + insn = GEN_FCN (icode) (reg, op0); + emit_insn (insn); + return reg; + } + else if (STRICT_ALIGNMENT) + { + tree inner_type = TREE_TYPE (treeop0); + HOST_WIDE_INT temp_size + = MAX (int_size_in_bytes (inner_type), + (HOST_WIDE_INT) GET_MODE_SIZE (mode)); + rtx new_rtx + = assign_stack_temp_for_type (mode, temp_size, type); + rtx new_with_op0_mode + = adjust_address (new_rtx, GET_MODE (op0), 0); + + gcc_assert (!TREE_ADDRESSABLE (exp)); + + if (GET_MODE (op0) == BLKmode) + emit_block_move (new_with_op0_mode, op0, + GEN_INT (GET_MODE_SIZE (mode)), + (modifier == EXPAND_STACK_PARM + ? BLOCK_OP_CALL_PARM : BLOCK_OP_NORMAL)); + else + emit_move_insn (new_with_op0_mode, op0); + + op0 = new_rtx; + } } op0 = adjust_address (op0, mode, 0); Index: expr.h =================================================================== --- expr.h (revision 206105) +++ expr.h (working copy) @@ -41,7 +41,8 @@ along with GCC; see the file COPYING3. is a constant that is not a legitimate address. EXPAND_WRITE means we are only going to write to the resulting rtx. EXPAND_MEMORY means we are interested in a memory result, even if - the memory is constant and we could have propagated a constant value. */ + the memory is constant and we could have propagated a constant value, + or the memory is unaligned on a STRICT_ALIGNMENT target. */ enum expand_modifier {EXPAND_NORMAL = 0, EXPAND_STACK_PARM, EXPAND_SUM, EXPAND_CONST_ADDRESS, EXPAND_INITIALIZER, EXPAND_WRITE, EXPAND_MEMORY}; Index: stor-layout.c =================================================================== --- stor-layout.c (revision 206105) +++ stor-layout.c (working copy) @@ -1575,8 +1575,9 @@ finalize_record_size (record_layout_info void compute_record_mode (tree type) { - tree field; enum machine_mode mode = VOIDmode; + bool blkmode_for_access = false; + tree field; /* Most RECORD_TYPEs have BLKmode, so we start off assuming that. However, if possible, we use a mode that fits in a register @@ -1597,9 +1598,7 @@ compute_record_mode (tree type) if (TREE_CODE (TREE_TYPE (field)) == ERROR_MARK || (TYPE_MODE (TREE_TYPE (field)) == BLKmode - && ! TYPE_NO_FORCE_BLK (TREE_TYPE (field)) - && !(TYPE_SIZE (TREE_TYPE (field)) != 0 - && integer_zerop (TYPE_SIZE (TREE_TYPE (field))))) + && ! TYPE_NO_FORCE_BLKMODE (TREE_TYPE (field))) || ! tree_fits_uhwi_p (bit_position (field)) || DECL_SIZE (field) == 0 || ! tree_fits_uhwi_p (DECL_SIZE (field))) @@ -1615,6 +1614,15 @@ compute_record_mode (tree type) BLKmode structure as a scalar. */ if (targetm.member_type_forces_blk (field, mode)) return; + + /* As an extension, we support out-of-bounds access for trailing arrays. + In this case, if the element type has non-zero size, then the record + type effectively has variable size so it needs to have BLKmode. */ + blkmode_for_access + = (TREE_CODE (TREE_TYPE (field)) == ARRAY_TYPE + && !integer_zerop (TYPE_SIZE (TREE_TYPE (TREE_TYPE (field))))) + || (RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)) + && TYPE_BLKMODE_FOR_ACCESS (TREE_TYPE (field))); } /* If we only have one real field; use its mode if that mode's size @@ -1636,9 +1644,14 @@ compute_record_mode (tree type) { /* If this is the only reason this type is BLKmode, then don't force containing types to be BLKmode. */ - TYPE_NO_FORCE_BLK (type) = 1; + SET_TYPE_NO_FORCE_BLKMODE (type, 1); SET_TYPE_MODE (type, BLKmode); } + + /* If the record type needs BLKmode for access but didn't get it through + the standard layout algorithm, record it for later. */ + if (TYPE_MODE (type) != BLKmode && blkmode_for_access) + SET_TYPE_BLKMODE_FOR_ACCESS (type, 1); } /* Compute TYPE_SIZE and TYPE_ALIGN for TYPE, once it has been laid @@ -2245,7 +2258,7 @@ layout_type (tree type) /* BLKmode elements force BLKmode aggregate; else extract/store fields may lose. */ && (TYPE_MODE (TREE_TYPE (type)) != BLKmode - || TYPE_NO_FORCE_BLK (TREE_TYPE (type)))) + || TYPE_NO_FORCE_BLKMODE (TREE_TYPE (type)))) { SET_TYPE_MODE (type, mode_for_array (TREE_TYPE (type), TYPE_SIZE (type))); @@ -2253,7 +2266,7 @@ layout_type (tree type) && STRICT_ALIGNMENT && TYPE_ALIGN (type) < BIGGEST_ALIGNMENT && TYPE_ALIGN (type) < GET_MODE_ALIGNMENT (TYPE_MODE (type))) { - TYPE_NO_FORCE_BLK (type) = 1; + SET_TYPE_NO_FORCE_BLKMODE (type, 1); SET_TYPE_MODE (type, BLKmode); } } Index: cfgexpand.c =================================================================== --- cfgexpand.c (revision 206105) +++ cfgexpand.c (working copy) @@ -2253,7 +2253,7 @@ expand_call_stmt (gimple stmt) if (lhs) expand_assignment (lhs, exp, false); else - expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL); + expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL); mark_transaction_restart_calls (stmt); } Index: lto/lto.c =================================================================== --- lto/lto.c (revision 206105) +++ lto/lto.c (working copy) @@ -1349,7 +1349,7 @@ compare_tree_sccs_1 (tree t1, tree t2, t { compare_values (TYPE_MODE); compare_values (TYPE_STRING_FLAG); - compare_values (TYPE_NO_FORCE_BLK); + compare_values (TYPE_BLKMODE_FLAG); compare_values (TYPE_NEEDS_CONSTRUCTING); if (RECORD_OR_UNION_TYPE_P (t1)) { Index: tree-streamer-out.c =================================================================== --- tree-streamer-out.c (revision 206105) +++ tree-streamer-out.c (working copy) @@ -302,7 +302,7 @@ pack_ts_type_common_value_fields (struct { bp_pack_enum (bp, machine_mode, MAX_MACHINE_MODE, TYPE_MODE (expr)); bp_pack_value (bp, TYPE_STRING_FLAG (expr), 1); - bp_pack_value (bp, TYPE_NO_FORCE_BLK (expr), 1); + bp_pack_value (bp, TYPE_BLKMODE_FLAG (expr), 1); bp_pack_value (bp, TYPE_NEEDS_CONSTRUCTING (expr), 1); if (RECORD_OR_UNION_TYPE_P (expr)) { Index: print-tree.c =================================================================== --- print-tree.c (revision 206107) +++ print-tree.c (working copy) @@ -583,8 +583,10 @@ print_node (FILE *file, const char *pref if (TYPE_UNSIGNED (node)) fputs (" unsigned", file); - if (TYPE_NO_FORCE_BLK (node)) - fputs (" no-force-blk", file); + if (TYPE_NO_FORCE_BLKMODE (node)) + fputs (" no-force-blkmode", file); + else if (TYPE_BLKMODE_FOR_ACCESS (node)) + fputs (" blkmode-for-access", file); if (TYPE_STRING_FLAG (node)) fputs (" string-flag", file); Index: tree-streamer-in.c =================================================================== --- tree-streamer-in.c (revision 206105) +++ tree-streamer-in.c (working copy) @@ -352,7 +352,7 @@ unpack_ts_type_common_value_fields (stru mode = bp_unpack_enum (bp, machine_mode, MAX_MACHINE_MODE); SET_TYPE_MODE (expr, mode); TYPE_STRING_FLAG (expr) = (unsigned) bp_unpack_value (bp, 1); - TYPE_NO_FORCE_BLK (expr) = (unsigned) bp_unpack_value (bp, 1); + TYPE_BLKMODE_FLAG (expr) = (unsigned) bp_unpack_value (bp, 1); TYPE_NEEDS_CONSTRUCTING (expr) = (unsigned) bp_unpack_value (bp, 1); if (RECORD_OR_UNION_TYPE_P (expr)) { Index: tree-core.h =================================================================== --- tree-core.h (revision 206105) +++ tree-core.h (working copy) @@ -1239,7 +1239,7 @@ struct GTY(()) tree_type_common { unsigned int uid; unsigned int precision : 10; - unsigned no_force_blk_flag : 1; + unsigned blkmode_flag : 1; unsigned needs_constructing_flag : 1; unsigned transparent_aggr_flag : 1; unsigned restrict_flag : 1;