From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23913 invoked by alias); 3 Sep 2019 08:40:03 -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 23866 invoked by uid 89); 3 Sep 2019 08:40:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,SPF_PASS autolearn=no version=3.3.1 spammy= X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.110.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Sep 2019 08:40:01 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 94B7528; Tue, 3 Sep 2019 01:39:59 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.62]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0288E3F246; Tue, 3 Sep 2019 01:39:58 -0700 (PDT) From: Richard Sandiford To: Christophe Lyon Mail-Followup-To: Christophe Lyon ,Christophe Lyon , gcc Patches , richard.sandiford@arm.com Cc: Christophe Lyon , gcc Patches Subject: Re: [ARM/FDPIC v5 04/21] [ARM] FDPIC: Add support for FDPIC for arm architecture References: <20190515124006.25840-1-christophe.lyon@st.com> <20190515124006.25840-5-christophe.lyon@st.com> Date: Tue, 03 Sep 2019 08:40:00 -0000 In-Reply-To: (Christophe Lyon's message of "Mon, 2 Sep 2019 22:04:36 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00098.txt.bz2 Christophe Lyon writes: > @@ -3485,6 +3485,14 @@ arm_option_override (void) > if (flag_pic && TARGET_VXWORKS_RTP) > arm_pic_register = 9; > > + /* If in FDPIC mode then force arm_pic_register to be r9. */ > + if (TARGET_FDPIC) > + { > + arm_pic_register = FDPIC_REGNUM; > + if (TARGET_THUMB1) > + sorry ("FDPIC mode is not supported in Thumb-1 mode."); Should be no "." at the end. > + } > + > if (arm_pic_register_string != NULL) > { > int pic_register = decode_reg_name (arm_pic_register_string); > [...] > @@ -7295,6 +7303,21 @@ arm_function_ok_for_sibcall (tree decl, tree exp) > if (cfun->machine->sibcall_blocked) > return false; > > + if (TARGET_FDPIC) > + { > + /* In FDPIC, never tailcall something for which we have no decl: > + the target function could be in a different module, requiring > + a different FDPIC register value. */ > + if (decl == NULL) > + return false; > + > + /* Don't tailcall if we go through the PLT since the FDPIC > + register is then corrupted and we don't restore it after > + static function calls. */ > + if (!targetm.binds_local_p (decl)) > + return false; > + } > + > /* Never tailcall something if we are generating code for Thumb-1. */ > if (TARGET_THUMB1) > return false; Is this still needed after you removed the optimisation to avoid restoring r9? (Not really a review comment, just curious.) > [...] > @@ -7780,28 +7812,132 @@ arm_load_pic_register (unsigned long saved_regs ATTRIBUTE_UNUSED, rtx pic_reg) > emit_use (pic_reg); > } > > +/* Try to determine whether an object, referenced via ORIG, will be > + placed in the text or data segment. This is used in FDPIC mode, to > + decide which relocations to use when accessing ORIG. *IS_READONLY > + is set to true if ORIG is a read-only location, false otherwise. > + Return true if we could determine the location of ORIG, false > + otherwise. *IS_READONLY is valid only when we return true. */ > +static bool > +arm_is_segment_info_known (rtx orig, bool *is_readonly) > +{ > + *is_readonly = false; > + > + if (GET_CODE (orig) == LABEL_REF) > + { > + *is_readonly = true; > + return true; > + } > + > + if (SYMBOL_REF_P (orig)) > + { > + if (CONSTANT_POOL_ADDRESS_P (orig)) > + { > + *is_readonly = true; > + return true; > + } > + else if (SYMBOL_REF_LOCAL_P (orig) > + && !SYMBOL_REF_EXTERNAL_P (orig) > + && SYMBOL_REF_DECL (orig) > + && (!DECL_P (SYMBOL_REF_DECL (orig)) > + || !DECL_COMMON (SYMBOL_REF_DECL (orig)))) This can just be an "if". > + { > + tree decl = SYMBOL_REF_DECL (orig); > + tree init = (TREE_CODE (decl) == VAR_DECL) > + ? DECL_INITIAL (decl) : (TREE_CODE (decl) == CONSTRUCTOR) > + ? decl : 0; > + int reloc = 0; > + bool named_section, readonly; > + > + if (init && init != error_mark_node) > + reloc = compute_reloc_for_constant (init); > + > + named_section = TREE_CODE (decl) == VAR_DECL > + && lookup_attribute ("section", DECL_ATTRIBUTES (decl)); > + readonly = decl_readonly_section (decl, reloc); > + > + /* We don't know where the link script will put a named > + section, so return false in such a case. */ > + if (named_section) > + return false; > + > + *is_readonly = readonly; > + return true; > + } > + else > + { > + /* We don't know. */ > + return false; > + } > + } > + else > + gcc_unreachable (); > + > + return false; Then this can end with: /* We don't know. */ return false; } gcc_unreachable (); } > +} > + > /* Generate code to load the address of a static var when flag_pic is set. */ > static rtx_insn * > arm_pic_static_addr (rtx orig, rtx reg) > { > rtx l1, labelno, offset_rtx; > + rtx_insn *insn; > > gcc_assert (flag_pic); > > - /* We use an UNSPEC rather than a LABEL_REF because this label > - never appears in the code stream. */ > - labelno = GEN_INT (pic_labelno++); > - l1 = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, labelno), UNSPEC_PIC_LABEL); > - l1 = gen_rtx_CONST (VOIDmode, l1); > + bool is_readonly = false; > + bool info_known = false; > + > + if (TARGET_FDPIC > + && SYMBOL_REF_P (orig) > + && !SYMBOL_REF_FUNCTION_P (orig)) > + info_known = arm_is_segment_info_known (orig, &is_readonly); > + > + if (TARGET_FDPIC > + && SYMBOL_REF_P (orig) > + && !SYMBOL_REF_FUNCTION_P (orig) > + && !info_known) > + { > + /* We don't know where orig is stored, so we have be > + pessimistic and use a GOT relocation. */ > + rtx pic_reg = gen_rtx_REG (Pmode, FDPIC_REGNUM); > + > + insn = calculate_pic_address_constant (reg, pic_reg, orig); > + } > + else if (TARGET_FDPIC > + && SYMBOL_REF_P (orig) > + && (SYMBOL_REF_FUNCTION_P (orig) > + || (info_known && !is_readonly))) The info_known check is redundant here. I think it's actually clearer without, since it's then more obvious that the final "else" is handling: !SYMBOL_REF_FUNCTION_P (orig) && is_readonly (Initially I misread the condition and was wondering why it was safe to drop to the "else" when "!info_known". But it doesn't do that of course.) > + { > + /* We use the GOTOFF relocation. */ > + rtx pic_reg = gen_rtx_REG (Pmode, FDPIC_REGNUM); > + > + rtx l1 = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, orig), UNSPEC_PIC_SYM); > + emit_insn (gen_movsi (reg, l1)); > + insn = emit_insn (gen_addsi3 (reg, reg, pic_reg)); > + } > + else > + { > + /* Not FDPIC, not SYMBOL_REF_P or readonly: we can use > + PC-relative access. */ > + /* We use an UNSPEC rather than a LABEL_REF because this label > + never appears in the code stream. */ > + labelno = GEN_INT (pic_labelno++); > + l1 = gen_rtx_UNSPEC (Pmode, gen_rtvec (1, labelno), UNSPEC_PIC_LABEL); > + l1 = gen_rtx_CONST (VOIDmode, l1); > + > + /* On the ARM the PC register contains 'dot + 8' at the time of the > + addition, on the Thumb it is 'dot + 4'. */ > + offset_rtx = plus_constant (Pmode, l1, TARGET_ARM ? 8 : 4); > + offset_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (2, orig, offset_rtx), > + UNSPEC_SYMBOL_OFFSET); > + offset_rtx = gen_rtx_CONST (Pmode, offset_rtx); > > - /* On the ARM the PC register contains 'dot + 8' at the time of the > - addition, on the Thumb it is 'dot + 4'. */ > - offset_rtx = plus_constant (Pmode, l1, TARGET_ARM ? 8 : 4); > - offset_rtx = gen_rtx_UNSPEC (Pmode, gen_rtvec (2, orig, offset_rtx), > - UNSPEC_SYMBOL_OFFSET); > - offset_rtx = gen_rtx_CONST (Pmode, offset_rtx); > + insn = emit_insn (gen_pic_load_addr_unified (reg, offset_rtx, > + labelno)); > + } > > - return emit_insn (gen_pic_load_addr_unified (reg, offset_rtx, labelno)); > + return insn; > } > > /* Return nonzero if X is valid as an ARM state addressing register. */ > @@ -8510,7 +8646,7 @@ load_tls_operand (rtx x, rtx reg) > static rtx_insn * > arm_call_tls_get_addr (rtx x, rtx reg, rtx *valuep, int reloc) > { > - rtx label, labelno, sum; > + rtx label, labelno = NULL_RTX, sum; > > gcc_assert (reloc != TLS_DESCSEQ); > start_sequence (); Looks like this might be a stray change (not mentioned in the changelog). > [...] > @@ -23069,9 +23234,37 @@ arm_assemble_integer (rtx x, unsigned int size, int aligned_p) > && (!SYMBOL_REF_LOCAL_P (x) > || (SYMBOL_REF_DECL (x) > ? DECL_WEAK (SYMBOL_REF_DECL (x)) : 0)))) > - fputs ("(GOT)", asm_out_file); > + { > + if (TARGET_FDPIC && SYMBOL_REF_FUNCTION_P (x)) > + fputs ("(GOTFUNCDESC)", asm_out_file); > + else > + fputs ("(GOT)", asm_out_file); > + } > else > - fputs ("(GOTOFF)", asm_out_file); > + { > + if (TARGET_FDPIC && SYMBOL_REF_FUNCTION_P (x)) > + fputs ("(GOTOFFFUNCDESC)", asm_out_file); > + else > + { > + bool is_readonly; > + > + if (arm_is_segment_info_known (x, &is_readonly)) > + fputs ("(GOTOFF)", asm_out_file); > + else > + fputs ("(GOT)", asm_out_file); > + } It looks like this changes behaviour for non-FDPIC. Is that intentional? Or should it be: if (!TARGET_FDPIC || arm_is_segment_info_known (x, &is_readonly)) ? > + } > + } > + > + /* For FDPIC we also have to mark symbol for .data section. */ > + if (TARGET_FDPIC > + && NEED_GOT_RELOC > + && flag_pic > + && !making_const_table > + && SYMBOL_REF_P (x)) > + { > + if (SYMBOL_REF_FUNCTION_P (x)) > + fputs ("(FUNCDESC)", asm_out_file); > } > fputc ('\n', asm_out_file); > return true; Given: > > Can NEED_GOT_RELOC or flag_pic be false for TARGET_FDPIC? > No. > > > Is !flag_pic TARGET_FDPIC supported? > No; flag_pic is false when we use -mno-fdpic, so we revert to the "usual" abi then the flag_pic and NEED_GOT_RELOC checks look redundant. Might as well put the SYMBOL_REF_FUNCTION_P (x) in the main "if" statement rather than split it out. > [...] > @@ -8151,10 +8156,33 @@ > pat = gen_call_internal (operands[0], operands[1], operands[2]); > arm_emit_call_insn (pat, XEXP (operands[0], 0), false); > } > + > + /* Restore FDPIC register (r9) after call. */ > + if (TARGET_FDPIC) > + { > + rtx fdpic_reg = gen_rtx_REG (Pmode, FDPIC_REGNUM); > + rtx initial_fdpic_reg = > + get_hard_reg_initial_val (Pmode, FDPIC_REGNUM); Formatting nit: "=" should be on the next line. > + > + emit_insn (gen_restore_pic_register_after_call (fdpic_reg, > + initial_fdpic_reg)); > + } > + > DONE; > }" > ) > > [...] > @@ -8240,6 +8273,18 @@ > operands[2], operands[3]); > arm_emit_call_insn (pat, XEXP (operands[1], 0), false); > } > + > + /* Restore FDPIC register (r9) after call. */ > + if (TARGET_FDPIC) > + { > + rtx fdpic_reg = gen_rtx_REG (Pmode, FDPIC_REGNUM); > + rtx initial_fdpic_reg = > + get_hard_reg_initial_val (Pmode, FDPIC_REGNUM); Same here. Looks good otherwise, thanks. Richard