From patchwork Sat Jun 29 07:17:08 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Botcazou X-Patchwork-Id: 1124720 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-504011-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=adacore.com 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 45bQ251vjxz9s4Y for ; Sat, 29 Jun 2019 17:17:24 +1000 (AEST) 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:subject:date:message-id:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=TjsiWGxJdKEU38JL dSYBlmybawfuG/ks4HbmvTfH28uYNPReTYpOAR3bn52J3z/Wu9daQGcAO5wHIWPD PvJdUKWICA7JMxySXhW7z4Y25RvB4aFCh1GiqFur/42zDmQPvvScBHFGs+jL/HYZ GchquiLBrlvvj3jzw37zWjBM+a0= 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:subject:date:message-id:mime-version:content-type :content-transfer-encoding; s=default; bh=2kFfW/mbfj7pf3g4r7OrtD 92zJM=; b=qvPZJReKETGMwauESlDoSGzvHWO1vI5SZRh6ngjXCCod488u5L+sfN 1zJc7y92XZOLcMSqLpFKIcRwX92EY+xbjsbU9AodPjVFeuxmjPAsQyI50RfeE7gE Iy7Fdwr2aKxEfQMwl9jFvj26NNSHpF0Yd7FRTxDKJechLT5EqVD48= Received: (qmail 28446 invoked by alias); 29 Jun 2019 07:17:16 -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 28433 invoked by uid 89); 29 Jun 2019 07:17:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=tre, plug, XS, xs 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 ESMTP; Sat, 29 Jun 2019 07:17:12 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id 553178139C for ; Sat, 29 Jun 2019 09:17:10 +0200 (CEST) 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 kIRafaTfAf8w for ; Sat, 29 Jun 2019 09:17:10 +0200 (CEST) Received: from polaris.localnet (bon31-6-88-161-99-133.fbx.proxad.net [88.161.99.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.eu.adacore.com (Postfix) with ESMTPSA id 1433281393 for ; Sat, 29 Jun 2019 09:17:09 +0200 (CEST) From: Eric Botcazou To: gcc-patches@gcc.gnu.org Subject: [Ada] Fix wrong code with small array in packed record Date: Sat, 29 Jun 2019 09:17:08 +0200 Message-ID: <4190051.ZsBJIy5eaQ@polaris> MIME-Version: 1.0 This fixes a long-standing issue with small arrays that end up being not aligned on byte boundaries in packed record types but whose size still is a multiple of a byte. This case reveals a small loophole between the front-end and the middle-end that gigi needs to plug. There is also a trivial fixlet for expr.c to make things work on big-endian architectures too. Tested on x86-64/Linux, applied on mainline, as obvious for the expr.c hunk. 2019-06-29 Eric Botcazou * expr.c (expand_expr_real_1) : Apply the big-endian adjustment for bit-fields to all aggregate types. 2019-06-29 Eric Botcazou * gcc-interface/gigi.h (make_packable_type): Remove default value. (value_factor_p): Tweak prototype. * gcc-interface/decl.c (gnat_to_gnu_entity): Add comment. (gnat_to_gnu_component_type): Likewise. (gnat_to_gnu_field): Likewise. Fetch the position of the field earlier and simplify the condition under which the type is packed. Declare local variable is_bitfield. Pass 1 as max_align to make_packable_type if it is set to true. (copy_and_substitute_in_layout): Pass 0 to make_packable_type. * gcc-interface/utils.c (make_packable_array_type): New function. (make_packable_type): Use it to rewrite the type of array field. (maybe_pad_type): Pass align parameter to make_packable_type. (create_field_decl): Minor tweaks. (value_factor_p): Assert that FACTOR is a power of 2 and replace the modulo computation by a masking operation. 2019-06-29 Eric Botcazou * gnat.dg/array35.adb: New test. * gnat.dg/array36.adb: Likewise. Index: expr.c =================================================================== --- expr.c (revision 272633) +++ expr.c (working copy) @@ -10893,12 +10893,12 @@ expand_expr_real_1 (tree exp, rtx target if (MEM_P (op0) && REG_P (XEXP (op0, 0))) mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0)); - /* If the result has a record type and the extraction is done in + /* If the result has aggregate type and the extraction is done in an integral mode, then the field may be not aligned on a byte boundary; in this case, if it has reverse storage order, it needs to be extracted as a scalar field with reverse storage order and put back into memory order afterwards. */ - if (TREE_CODE (type) == RECORD_TYPE + if (AGGREGATE_TYPE_P (type) && GET_MODE_CLASS (ext_mode) == MODE_INT) reversep = TYPE_REVERSE_STORAGE_ORDER (type); @@ -10908,13 +10908,13 @@ expand_expr_real_1 (tree exp, rtx target ? NULL_RTX : target), ext_mode, ext_mode, reversep, alt_rtl); - /* If the result has a record type and the mode of OP0 is an + /* If the result has aggregate type and the mode of OP0 is an integral mode then, if BITSIZE is narrower than this mode and this is for big-endian data, we must put the field into the high-order bits. And we must also put it back into memory order if it has been previously reversed. */ scalar_int_mode op0_mode; - if (TREE_CODE (type) == RECORD_TYPE + if (AGGREGATE_TYPE_P (type) && is_int_mode (GET_MODE (op0), &op0_mode)) { HOST_WIDE_INT size = GET_MODE_BITSIZE (op0_mode); Index: ada/gcc-interface/decl.c =================================================================== --- ada/gcc-interface/decl.c (revision 272637) +++ ada/gcc-interface/decl.c (working copy) @@ -4481,6 +4481,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entit } } + /* Now check if the type allows atomic access. */ if (Is_Atomic_Or_VFA (gnat_entity)) check_ok_for_atomic_type (gnu_type, gnat_entity, false); @@ -5100,6 +5101,7 @@ gnat_to_gnu_component_type (Entity_Id gn } } + /* Now check if the type of the component allows atomic access. */ if (Has_Atomic_Components (gnat_array) || Is_Atomic_Or_VFA (gnat_type)) check_ok_for_atomic_type (gnu_type, gnat_array, true); @@ -6901,6 +6903,7 @@ gnat_to_gnu_field (Entity_Id gnat_field, boundaries, but that should be guaranteed by the GCC memory model. */ const bool needs_strict_alignment = (is_atomic || is_aliased || is_independent || is_strict_alignment); + bool is_bitfield; tree gnu_field_type = gnat_to_gnu_type (gnat_field_type); tree gnu_field_id = get_entity_name (gnat_field); tree gnu_field, gnu_size, gnu_pos; @@ -6915,7 +6918,7 @@ gnat_to_gnu_field (Entity_Id gnat_field, /* If a size is specified, use it. Otherwise, if the record type is packed, use the official RM size. See "Handling of Type'Size Values" in Einfo for further details. */ - if (Known_Esize (gnat_field) || Present (gnat_clause)) + if (Present (gnat_clause) || Known_Esize (gnat_field)) gnu_size = validate_size (Esize (gnat_field), gnu_field_type, gnat_field, FIELD_DECL, false, true); else if (packed == 1) @@ -6927,12 +6930,36 @@ gnat_to_gnu_field (Entity_Id gnat_field, else gnu_size = NULL_TREE; - /* If we have a specified size that is smaller than that of the field's type, - or a position is specified, and the field's type is a record that doesn't - require strict alignment, see if we can get either an integral mode form - of the type or a smaller form. If we can, show a size was specified for - the field if there wasn't one already, so we know to make this a bitfield - and avoid making things wider. + /* Likewise for the position. */ + if (Present (gnat_clause)) + { + gnu_pos = UI_To_gnu (Component_Bit_Offset (gnat_field), bitsizetype); + is_bitfield = !value_factor_p (gnu_pos, BITS_PER_UNIT); + } + + /* If the record has rep clauses and this is the tag field, make a rep + clause for it as well. */ + else if (Has_Specified_Layout (gnat_record_type) + && Chars (gnat_field) == Name_uTag) + { + gnu_pos = bitsize_zero_node; + gnu_size = TYPE_SIZE (gnu_field_type); + is_bitfield = false; + } + + else + { + gnu_pos = NULL_TREE; + is_bitfield = false; + } + + /* If the field's type is a fixed-size record that does not require strict + alignment, and the record is packed or we have a position specified for + the field that makes it a bitfield or we have a specified size that is + smaller than that of the field's type, then see if we can get either an + integral mode form of the field's type or a smaller form. If we can, + consider that a size was specified for the field if there wasn't one + already, so we know to make it a bitfield and avoid making things wider. Changing to an integral mode form is useful when the record is packed as we can then place the field at a non-byte-aligned position and so achieve @@ -6954,14 +6981,12 @@ gnat_to_gnu_field (Entity_Id gnat_field, && !TYPE_FAT_POINTER_P (gnu_field_type) && tree_fits_uhwi_p (TYPE_SIZE (gnu_field_type)) && (packed == 1 + || is_bitfield || (gnu_size - && (tree_int_cst_lt (gnu_size, TYPE_SIZE (gnu_field_type)) - || (Present (gnat_clause) - && !(UI_To_Int (Component_Bit_Offset (gnat_field)) - % BITS_PER_UNIT == 0 - && value_factor_p (gnu_size, BITS_PER_UNIT))))))) + && tree_int_cst_lt (gnu_size, TYPE_SIZE (gnu_field_type))))) { - tree gnu_packable_type = make_packable_type (gnu_field_type, true); + tree gnu_packable_type + = make_packable_type (gnu_field_type, true, is_bitfield ? 1 : 0); if (gnu_packable_type != gnu_field_type) { gnu_field_type = gnu_packable_type; @@ -6970,6 +6995,7 @@ gnat_to_gnu_field (Entity_Id gnat_field, } } + /* Now check if the type of the field allows atomic access. */ if (Is_Atomic_Or_VFA (gnat_field)) { const unsigned int align @@ -6981,12 +7007,11 @@ gnat_to_gnu_field (Entity_Id gnat_field, check_ok_for_atomic_type (gnu_field_type, gnat_field, false); } - if (Present (gnat_clause)) + /* If a position is specified, check that it is valid. */ + if (gnu_pos) { Entity_Id gnat_parent = Parent_Subtype (gnat_record_type); - gnu_pos = UI_To_gnu (Component_Bit_Offset (gnat_field), bitsizetype); - /* Ensure the position does not overlap with the parent subtype, if there is one. This test is omitted if the parent of the tagged type has a full rep clause since, in this case, component clauses are allowed to @@ -7092,19 +7117,8 @@ gnat_to_gnu_field (Entity_Id gnat_field, } } - /* If the record has rep clauses and this is the tag field, make a rep - clause for it as well. */ - else if (Has_Specified_Layout (gnat_record_type) - && Chars (gnat_field) == Name_uTag) - { - gnu_pos = bitsize_zero_node; - gnu_size = TYPE_SIZE (gnu_field_type); - } - else { - gnu_pos = NULL_TREE; - /* If we are packing the record and the field is BLKmode, round the size up to a byte boundary. */ if (packed && TYPE_MODE (gnu_field_type) == BLKmode && gnu_size) @@ -9681,7 +9695,7 @@ copy_and_substitute_in_layout (Entity_Id if (RECORD_OR_UNION_TYPE_P (gnu_field_type) && !TYPE_FAT_POINTER_P (gnu_field_type) && tree_fits_uhwi_p (TYPE_SIZE (gnu_field_type))) - gnu_field_type = make_packable_type (gnu_field_type, true); + gnu_field_type = make_packable_type (gnu_field_type, true, 0); } else Index: ada/gcc-interface/gigi.h =================================================================== --- ada/gcc-interface/gigi.h (revision 272637) +++ ada/gcc-interface/gigi.h (working copy) @@ -126,7 +126,7 @@ extern tree make_aligning_type (tree typ MAX_ALIGN alignment if the value is non-zero. If so, return the new type; if not, return the original type. */ extern tree make_packable_type (tree type, bool in_record, - unsigned int max_align = 0); + unsigned int max_align); /* Given a type TYPE, return a new type whose size is appropriate for SIZE. If TYPE is the best type, return it. Otherwise, make a new type. We @@ -837,7 +837,7 @@ extern unsigned int known_alignment (tre /* Return true if VALUE is a multiple of FACTOR. FACTOR must be a power of 2. */ -extern bool value_factor_p (tree value, HOST_WIDE_INT factor); +extern bool value_factor_p (tree value, unsigned HOST_WIDE_INT factor); /* Build an atomic load for the underlying atomic object in SRC. SYNC is true if the load requires synchronization. */ Index: ada/gcc-interface/utils.c =================================================================== --- ada/gcc-interface/utils.c (revision 272633) +++ ada/gcc-interface/utils.c (working copy) @@ -984,10 +984,45 @@ make_aligning_type (tree type, unsigned return record_type; } +/* TYPE is an ARRAY_TYPE that is being used as the type of a field in a packed + record. See if we can rewrite it as a type that has non-BLKmode, which we + can pack tighter in the packed record. If so, return the new type; if not, + return the original type. */ + +static tree +make_packable_array_type (tree type) +{ + const unsigned HOST_WIDE_INT size = tree_to_uhwi (TYPE_SIZE (type)); + unsigned HOST_WIDE_INT new_size; + unsigned int new_align; + + /* No point in doing anything if the size is either zero or too large for an + integral mode, or if the type already has non-BLKmode. */ + if (size == 0 || size > MAX_FIXED_MODE_SIZE || TYPE_MODE (type) != BLKmode) + return type; + + /* Punt if the component type is an aggregate type for now. */ + if (AGGREGATE_TYPE_P (TREE_TYPE (type))) + return type; + + tree new_type = copy_type (type); + + new_size = ceil_pow2 (size); + new_align = MIN (new_size, BIGGEST_ALIGNMENT); + SET_TYPE_ALIGN (new_type, new_align); + + TYPE_SIZE (new_type) = bitsize_int (new_size); + TYPE_SIZE_UNIT (new_type) = size_int (new_size / BITS_PER_UNIT); + + SET_TYPE_MODE (new_type, mode_for_size (new_size, MODE_INT, 1).else_blk ()); + + return new_type; +} + /* TYPE is a RECORD_TYPE, UNION_TYPE or QUAL_UNION_TYPE that is being used - as the field type of a packed record if IN_RECORD is true, or as the - component type of a packed array if IN_RECORD is false. See if we can - rewrite it either as a type that has non-BLKmode, which we can pack + as the type of a field in a packed record if IN_RECORD is true, or as + the component type of a packed array if IN_RECORD is false. See if we + can rewrite it either as a type that has non-BLKmode, which we can pack tighter in the packed record case, or as a smaller type with at most MAX_ALIGN alignment if the value is non-zero. If so, return the new type; if not, return the original type. */ @@ -995,9 +1030,9 @@ make_aligning_type (tree type, unsigned tree make_packable_type (tree type, bool in_record, unsigned int max_align) { - unsigned HOST_WIDE_INT size = tree_to_uhwi (TYPE_SIZE (type)); + const unsigned HOST_WIDE_INT size = tree_to_uhwi (TYPE_SIZE (type)); + const unsigned int align = TYPE_ALIGN (type); unsigned HOST_WIDE_INT new_size; - unsigned int align = TYPE_ALIGN (type); unsigned int new_align; /* No point in doing anything if the size is zero. */ @@ -1058,10 +1093,19 @@ make_packable_type (tree type, bool in_r tree new_field_type = TREE_TYPE (field); tree new_field, new_field_size; - if (RECORD_OR_UNION_TYPE_P (new_field_type) - && !TYPE_FAT_POINTER_P (new_field_type) + if (AGGREGATE_TYPE_P (new_field_type) && tree_fits_uhwi_p (TYPE_SIZE (new_field_type))) - new_field_type = make_packable_type (new_field_type, true, max_align); + { + if (RECORD_OR_UNION_TYPE_P (new_field_type) + && !TYPE_FAT_POINTER_P (new_field_type)) + new_field_type + = make_packable_type (new_field_type, true, max_align); + else if (in_record + && max_align > 0 + && max_align < BITS_PER_UNIT + && TREE_CODE (new_field_type) == ARRAY_TYPE) + new_field_type = make_packable_array_type (new_field_type); + } /* However, for the last field in a not already packed record type that is of an aggregate type, we need to use the RM size in the @@ -1411,7 +1455,7 @@ maybe_pad_type (tree type, tree size, un different modes, a VIEW_CONVERT_EXPR will be required for converting between them and it might be hard to overcome afterwards, including at the RTL level when the stand-alone object is accessed as a whole. */ - if (align != 0 + if (align > 0 && RECORD_OR_UNION_TYPE_P (type) && TYPE_MODE (type) == BLKmode && !TYPE_BY_REFERENCE_P (type) @@ -1422,7 +1466,7 @@ maybe_pad_type (tree type, tree size, un || (TREE_CODE (size) == INTEGER_CST && compare_tree_int (size, MAX_FIXED_MODE_SIZE) <= 0))) { - tree packable_type = make_packable_type (type, true); + tree packable_type = make_packable_type (type, true, align); if (TYPE_MODE (packable_type) != BLKmode && align >= TYPE_ALIGN (packable_type)) type = packable_type; @@ -2790,10 +2834,9 @@ create_field_decl (tree name, tree type, size = round_up (size, BITS_PER_UNIT); } - /* If we may, according to ADDRESSABLE, make a bitfield when the size is - specified for two reasons: first if the size differs from the natural - size; second, if the alignment is insufficient. There are a number of - ways the latter can be true. + /* If we may, according to ADDRESSABLE, then make a bitfield when the size + is specified for two reasons: first, when it differs from the natural + size; second, when the alignment is insufficient. We never make a bitfield if the type of the field has a nonconstant size, because no such entity requiring bitfield operations should reach here. @@ -2809,17 +2852,17 @@ create_field_decl (tree name, tree type, && size && TREE_CODE (size) == INTEGER_CST && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST - && (!tree_int_cst_equal (size, TYPE_SIZE (type)) + && (packed + || !tree_int_cst_equal (size, TYPE_SIZE (type)) || (pos && !value_factor_p (pos, TYPE_ALIGN (type))) - || packed - || (TYPE_ALIGN (record_type) != 0 + || (TYPE_ALIGN (record_type) && TYPE_ALIGN (record_type) < TYPE_ALIGN (type)))) { DECL_BIT_FIELD (field_decl) = 1; DECL_SIZE (field_decl) = size; if (!packed && !pos) { - if (TYPE_ALIGN (record_type) != 0 + if (TYPE_ALIGN (record_type) && TYPE_ALIGN (record_type) < TYPE_ALIGN (type)) SET_DECL_ALIGN (field_decl, TYPE_ALIGN (record_type)); else @@ -3001,10 +3044,12 @@ process_attributes (tree *node, struct a a power of 2. */ bool -value_factor_p (tree value, HOST_WIDE_INT factor) +value_factor_p (tree value, unsigned HOST_WIDE_INT factor) { + gcc_checking_assert (pow2p_hwi (factor)); + if (tree_fits_uhwi_p (value)) - return tree_to_uhwi (value) % factor == 0; + return (tree_to_uhwi (value) & (factor - 1)) == 0; if (TREE_CODE (value) == MULT_EXPR) return (value_factor_p (TREE_OPERAND (value, 0), factor)