From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2049) id 5A4953857C47; Mon, 28 Feb 2022 12:08:40 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5A4953857C47 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Matthew Malcomson To: gcc-cvs@gcc.gnu.org Subject: [gcc(refs/vendors/ARM/heads/morello)] Fix bugs around automatic capability alignment X-Act-Checkin: gcc X-Git-Author: Matthew Malcomson X-Git-Refname: refs/vendors/ARM/heads/morello X-Git-Oldrev: 39294e2fe8abb2b2a06c2d196cd05107b26b37cd X-Git-Newrev: 4b7272b8c06eed8d3667412c063e61b8c5b9f292 Message-Id: <20220228120840.5A4953857C47@sourceware.org> Date: Mon, 28 Feb 2022 12:08:40 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Feb 2022 12:08:40 -0000 https://gcc.gnu.org/g:4b7272b8c06eed8d3667412c063e61b8c5b9f292 commit 4b7272b8c06eed8d3667412c063e61b8c5b9f292 Author: Matthew Malcomson Date: Thu Feb 3 17:37:16 2022 +0000 Fix bugs around automatic capability alignment In commit a77f6664310 we always aligned things that were being written out as capabilities. This triggered some failures in the testsuite, which pointed out that the idea was mistaken. The idea was based on the fact that loading an unaligned capability triggers a fault hence thinking that we would only ever want to write out aligned capabilities. The choice to allow unaligned capabilities being written out had already been made the other way, and that makes more sense. Allowing unaligned capabilities means that packed structures with pointers in them are allowed (while we have a warning on the unaligned capability). This patch reverts that part of the commit, and also ensures we never use `capinit` or `chericap` to emit capabilities that we want unaligned. This adjustment is necessary since the assembler adds alignment when it sees either of these directives. Hence when trying to emit an unaligned capability we need to emit it without these directives. That change to emit unaligned capabilities with a different directive uncovered an existing bug from the "precisely bounded capabilities" implementation (see 2167d4d7c41). A few places mistakenly passed the alignment to the new hook in bits, while the hook takes alignment in bytes, while others used the return value from the new hook as if it were bits when it was in fact bytes. We do not change the definition of the hook, since the units of the corresponding hook are also bytes and we feel that matching these units still seems like the least error-prone approach. Here we simply use the helper function `alignment_pad_from_bits` that uses bits for this alignment argument. This patch does not come with a testsuite change, it is tested using the existing testcases which emit unaligned capabilities and/or those which emit aligned capabiilies going through those codepaths which have been modified. (tests in the testsuite which have been seen to be affected by broken implementations of this patch are: - gcc.c-torture/execute/20010915-1.c - gcc.c-torture/execute/920501-5.c - gcc.c-torture/execute/980223.c - gcc.c-torture/execute/builtins/strpbrk.c - gcc.dg/20000906-1.c - gcc.dg/torture/pr52530.c - gcc.dg/torture/pr53908.c - gcc.target/aarch64/aapcs64/morello-test_8.c - gcc.target/aarch64/morello/label-addressing-includes-lsb.c) For ease of review, a description of the changes: - assemble_noswitch_variable takes its alignment in bits hence we should pass the alignment argument of targetm.data_padding_size correspondingly adjusted. (can see this alignment is given in bits by the rest of the function). - assemble_constant_contents mentions that it takes alignment in bits in the comment above the function. - output_constant_def_contents passes its alignment to assemble_constant_contents hence its internal align variable should stay represented as bits. - output_constant_pool_1 passes its `align_used` internal variable to `assemble_align` hence that should be in bits. - place_block_symbol already did its calculations in bits, but we have a helper function which handles the case when there was a bitwise alignment and targetm.data_alignment did not change the alignment. Hence we change the code to use that helper function. Diff: --- gcc/config/aarch64/aarch64.c | 22 ++++++++++++++-------- gcc/varasm.c | 35 +++++++++++++++++------------------ 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 1a684a2b66f..52d86e47924 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -20256,16 +20256,12 @@ cap_offset_known_outside_bounds (rtx label, rtx maybe_constant, tree *declp, bool aarch64_asm_output_capability (rtx x, unsigned int size, int aligned_p) { - /* Capability loads must be aligned on Morello, so we must emit an aligned - capability. This is automatically done by the assembler if we're using - `capinit` or `chericap`, but when we emit an integer we need to manually - do that. */ - aligned_p = aligned_p || !TARGET_CAPABILITY_FAKE; if (CONST_NULL_P (x)) { if (size == 16) { - fputs ("\t.p2align\t4\n", asm_out_file); + if (aligned_p) + fputs ("\t.p2align\t4\n", asm_out_file); fputs ("\t.zero\t16\n", asm_out_file); return true; } @@ -20306,7 +20302,8 @@ aarch64_asm_output_capability (rtx x, unsigned int size, int aligned_p) && CONST_NULL_P (XEXP (x, 0)) && size == 16) { - fputs ("\t.p2align\t4\n", asm_out_file); + if (aligned_p) + fputs ("\t.p2align\t4\n", asm_out_file); ret = targetm.asm_out.integer (XEXP (x, 1), 8, 0); return ret && targetm.asm_out.integer (const0_rtx, 8, 0); } @@ -20334,9 +20331,18 @@ aarch64_asm_output_capability (rtx x, unsigned int size, int aligned_p) requesting a capability for it. This would cause a linker error since the linker refuses to generate such invalid-by-definition capabilities. */ - fputs ("\t.p2align\t4\n", asm_out_file); + if (aligned_p) + fputs ("\t.p2align\t4\n", asm_out_file); ret = targetm.asm_out.integer (drop_capability (x), offset_size, aligned_p); } + else if (! aligned_p) + { + /* If we want to emit a capability but it is not aligned, we can't use + the `capinit` or `chericap` directives since they include alignment to + a capability boundary. */ + ret = targetm.asm_out.integer (drop_capability (x), offset_size, + aligned_p); + } else { fputs ("\t.capinit\t", asm_out_file); diff --git a/gcc/varasm.c b/gcc/varasm.c index c42519a316f..e36d9197c9a 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -2098,7 +2098,7 @@ assemble_noswitch_variable (tree decl, const char *name, section *sect, unsigned HOST_WIDE_INT size, rounded; size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); - size += targetm.data_padding_size (size, align, decl); + size += targetm.data_padding_size (size, align / BITS_PER_UNIT, decl); rounded = size; if ((flag_sanitize & SANITIZE_ADDRESS) && asan_protect_global (decl)) @@ -3620,7 +3620,7 @@ assemble_constant_contents (tree exp, const char *label, unsigned int align, HOST_WIDE_INT size; size = get_constant_size (exp); - size += targetm.data_padding_size (size, align, decl); + size += targetm.data_padding_size (size, align / BITS_PER_UNIT, decl); /* Do any machine/system dependent processing of the constant. */ targetm.asm_out.declare_constant_name (asm_out_file, label, exp, size); @@ -3669,11 +3669,9 @@ output_constant_def_contents (rtx symbol) : symtab_node::get (decl)->definition_alignment ()); section *sect = get_constant_section (exp, align); switch_to_section (sect); - align = targetm.data_alignment (get_constant_size (exp), - align / BITS_PER_UNIT, - decl); + align = alignment_pad_from_bits (get_constant_size (exp), align, decl); if (align) - ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align)); + ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align/BITS_PER_UNIT)); assemble_constant_contents (exp, XSTR (symbol, 0), align, (sect->common.flags & SECTION_MERGE) && (sect->common.flags & SECTION_STRINGS), @@ -4169,8 +4167,10 @@ output_constant_pool_1 (class constant_descriptor_rtx *desc, } uint64_t size = GET_MODE_SIZE (desc->mode); - uint64_t align_used = targetm.data_alignment (size, align, NULL_TREE); - uint64_t padding = targetm.data_padding_size (size, align_used, NULL_TREE); + uint64_t align_used = alignment_pad_from_bits (size, align, NULL_TREE); + uint64_t padding = targetm.data_padding_size (size, + align_used / BITS_PER_UNIT, + NULL_TREE); #ifdef ASM_OUTPUT_SPECIAL_POOL_ENTRY ASM_OUTPUT_SPECIAL_POOL_ENTRY (asm_out_file, x, desc->mode, @@ -7746,10 +7746,9 @@ place_block_symbol (rtx symbol) desc = SYMBOL_REF_CONSTANT (symbol); alignment = desc->align; size = GET_MODE_SIZE (desc->mode); - alignment = targetm.data_alignment (size, alignment / BITS_PER_UNIT, - NULL_TREE); - size += targetm.data_padding_size (size, alignment, NULL_TREE); - alignment *= BITS_PER_UNIT; + alignment = alignment_pad_from_bits (size, alignment, NULL_TREE); + size += targetm.data_padding_size (size, alignment / BITS_PER_UNIT, + NULL_TREE); } else if (TREE_CONSTANT_POOL_ADDRESS_P (symbol)) { @@ -7757,9 +7756,9 @@ place_block_symbol (rtx symbol) gcc_checking_assert (DECL_IN_CONSTANT_POOL (decl)); alignment = DECL_ALIGN (decl); size = get_constant_size (DECL_INITIAL (decl)); - alignment = targetm.data_alignment (size, alignment / BITS_PER_UNIT, decl); - size += targetm.data_padding_size (size, alignment, decl); - alignment *= BITS_PER_UNIT; + alignment = alignment_pad_from_bits (size, alignment, decl); + size += targetm.data_padding_size (size, alignment / BITS_PER_UNIT, + decl); if ((flag_sanitize & SANITIZE_ADDRESS) && TREE_CODE (DECL_INITIAL (decl)) == STRING_CST && asan_protect_global (DECL_INITIAL (decl))) @@ -7789,9 +7788,9 @@ place_block_symbol (rtx symbol) } alignment = get_variable_align (decl); size = tree_to_uhwi (DECL_SIZE_UNIT (decl)); - alignment = targetm.data_alignment (size, alignment / BITS_PER_UNIT, decl); - size += targetm.data_padding_size (size, alignment, decl); - alignment *= BITS_PER_UNIT; + alignment = alignment_pad_from_bits (size, alignment, decl); + size += targetm.data_padding_size (size, alignment / BITS_PER_UNIT, + decl); if ((flag_sanitize & SANITIZE_ADDRESS) && asan_protect_global (decl)) {