From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 84921 invoked by alias); 31 Jul 2017 11:19:17 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 84824 invoked by uid 89); 31 Jul 2017 11:19:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=downward X-HELO: sasl.smtp.pobox.com Received: from pb-smtp2.pobox.com (HELO sasl.smtp.pobox.com) (64.147.108.71) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 31 Jul 2017 11:19:13 +0000 Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 9D2AC9ADBE; Mon, 31 Jul 2017 07:19:10 -0400 (EDT) Received: from pb-smtp2.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp2.pobox.com (Postfix) with ESMTP id 956CB9ADBD; Mon, 31 Jul 2017 07:19:10 -0400 (EDT) Received: from loudmouth.attlocal.net (unknown [76.215.41.237]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by pb-smtp2.pobox.com (Postfix) with ESMTPSA id 9F8E09ADBA; Mon, 31 Jul 2017 07:19:09 -0400 (EDT) From: Daniel Santos To: gcc-patches , Uros Bizjak , Jan Hubicka Cc: Martin Liska , "H . J . Lu" Subject: [PATCH 1/6] [i386] Correct comments, add assertions to sp_valid_at and fp_valid_at Date: Mon, 31 Jul 2017 11:19:00 -0000 Message-Id: <20170731112435.30101-1-daniel.santos@pobox.com> In-Reply-To: References: X-Pobox-Relay-ID: 12FF46AC-75E2-11E7-BE28-9D2B0D78B957-06139138!pb-smtp2.pobox.com X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg02006.txt.bz2 When we realign the stack frame (without DRAP), there may be a range of CFA offsets that should never be touched because they are alignment padding and any reference to them is almost certainly an error. Previously, only the offset of where the realigned stack frame starts was recorded and checked in sp_valid_at and fp_valid_at. This change adds sp_realigned_fp_last to struct machine_frame_state to record the last valid offset from which the frame pointer can be used when the stack pointer is realigned and modifies sp_valid_at and fp_valid_at to fail an assertion when passed an offset in the "no-man's land" between these two values. Comments for struct machine_frame_state incorrectly stated that a realigned stack pointer could be used to access offsets equal to or greater than sp_realigned_offset, but it is only valid for offsets that are greater. This was the (incorrect) behaviour of sp_valid_at and fp_valid_at prior to r250587 and this change now corrects the documentation and adds clarification of the CFA-relative calculation. Signed-off-by: Daniel Santos --- gcc/config/i386/i386.c | 45 ++++++++++++++++++++++++++++++--------------- gcc/config/i386/i386.h | 18 +++++++++++++----- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index f1486ff3750..690631dfe43 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -13102,26 +13102,36 @@ choose_baseaddr_len (unsigned int regno, HOST_WIDE_INT offset) return len; } -/* Determine if the stack pointer is valid for accessing the cfa_offset. - The register is saved at CFA - CFA_OFFSET. */ +/* Determine if the stack pointer is valid for accessing the CFA_OFFSET in + the frame save area. The register is saved at CFA - CFA_OFFSET. */ -static inline bool +static bool sp_valid_at (HOST_WIDE_INT cfa_offset) { const struct machine_frame_state &fs = cfun->machine->fs; - return fs.sp_valid && !(fs.sp_realigned - && cfa_offset <= fs.sp_realigned_offset); + if (fs.sp_realigned && cfa_offset <= fs.sp_realigned_offset) + { + /* Validate that the cfa_offset isn't in a "no-man's land". */ + gcc_assert (cfa_offset <= fs.sp_realigned_fp_last); + return false; + } + return fs.sp_valid; } -/* Determine if the frame pointer is valid for accessing the cfa_offset. - The register is saved at CFA - CFA_OFFSET. */ +/* Determine if the frame pointer is valid for accessing the CFA_OFFSET in + the frame save area. The register is saved at CFA - CFA_OFFSET. */ static inline bool fp_valid_at (HOST_WIDE_INT cfa_offset) { const struct machine_frame_state &fs = cfun->machine->fs; - return fs.fp_valid && !(fs.sp_valid && fs.sp_realigned - && cfa_offset > fs.sp_realigned_offset); + if (fs.sp_realigned && cfa_offset > fs.sp_realigned_fp_last) + { + /* Validate that the cfa_offset isn't in a "no-man's land". */ + gcc_assert (cfa_offset >= fs.sp_realigned_offset); + return false; + } + return fs.fp_valid; } /* Choose a base register based upon alignment requested, speed and/or @@ -14560,6 +14570,9 @@ ix86_expand_prologue (void) int align_bytes = crtl->stack_alignment_needed / BITS_PER_UNIT; gcc_assert (align_bytes > MIN_STACK_BOUNDARY / BITS_PER_UNIT); + /* Record last valid frame pointer offset. */ + m->fs.sp_realigned_fp_last = m->fs.sp_offset; + /* The computation of the size of the re-aligned stack frame means that we must allocate the size of the register save area before performing the actual alignment. Otherwise we cannot guarantee @@ -14573,13 +14586,15 @@ ix86_expand_prologue (void) insn = emit_insn (ix86_gen_andsp (stack_pointer_rtx, stack_pointer_rtx, GEN_INT (-align_bytes))); - /* For the purposes of register save area addressing, the stack - pointer can no longer be used to access anything in the frame - below m->fs.sp_realigned_offset and the frame pointer cannot be - used for anything at or above. */ m->fs.sp_offset = ROUND_UP (m->fs.sp_offset, align_bytes); m->fs.sp_realigned = true; m->fs.sp_realigned_offset = m->fs.sp_offset - frame.nsseregs * 16; + /* The stack pointer may no longer be equal to CFA - m->fs.sp_offset. + Beyond this point, stack access should be done via choose_baseaddr or + by using sp_valid_at and fp_valid_at to determine the correct base + register. Henceforth, any CFA offset should be thought of as logical + and not physical. */ + gcc_assert (m->fs.sp_realigned_offset >= m->fs.sp_realigned_fp_last); gcc_assert (m->fs.sp_realigned_offset == frame.stack_realign_offset); /* SEH unwind emit doesn't currently support REG_CFA_EXPRESSION, which is needed to describe where a register is saved using a realigned @@ -15269,10 +15284,10 @@ ix86_expand_epilogue (int style) if (restore_regs_via_mov || frame.nsseregs) { /* Ensure that the entire register save area is addressable via - the stack pointer, if we will restore via sp. */ + the stack pointer, if we will restore SSE regs via sp. */ if (TARGET_64BIT && m->fs.sp_offset > 0x7fffffff - && !(fp_valid_at (frame.stack_realign_offset) || m->fs.drap_valid) + && sp_valid_at (frame.stack_realign_offset) && (frame.nsseregs + frame.nregs) != 0) { pro_epilogue_adjust_stack (stack_pointer_rtx, stack_pointer_rtx, diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h index 682745ae06b..ce5bb7f6677 100644 --- a/gcc/config/i386/i386.h +++ b/gcc/config/i386/i386.h @@ -2512,7 +2512,9 @@ struct GTY(()) ix86_frame bool save_regs_using_mov; }; -/* Machine specific frame tracking during prologue/epilogue generation. */ +/* Machine specific frame tracking during prologue/epilogue generation. All + values are positive, but since the x86 stack grows downward, are subtratced + from the CFA to produce a valid address. */ struct GTY(()) machine_frame_state { @@ -2550,13 +2552,19 @@ struct GTY(()) machine_frame_state /* Indicates whether the stack pointer has been re-aligned. When set, SP/FP continue to be relative to the CFA, but the stack pointer - should only be used for offsets >= sp_realigned_offset, while - the frame pointer should be used for offsets < sp_realigned_offset. + should only be used for offsets > sp_realigned_offset, while + the frame pointer should be used for offsets <= sp_realigned_fp_last. The flags realigned and sp_realigned are mutually exclusive. */ BOOL_BITFIELD sp_realigned : 1; - /* If sp_realigned is set, this is the offset from the CFA that the - stack pointer was realigned to. */ + /* If sp_realigned is set, this is the last valid offset from the CFA + that can be used for access with the frame pointer. */ + HOST_WIDE_INT sp_realigned_fp_last; + + /* If sp_realigned is set, this is the offset from the CFA that the stack + pointer was realigned, and may or may not be equal to sp_realigned_fp_last. + Access via the stack pointer is only valid for offsets that are greater than + this value. */ HOST_WIDE_INT sp_realigned_offset; }; -- 2.13.3