[ARM/FDPIC v5 04/21] [ARM] FDPIC: Add support for FDPIC for arm architecture
Christophe Lyon
christophe.lyon@linaro.org
Wed Sep 4 19:59:00 GMT 2019
On Tue, 3 Sep 2019 at 10:40, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon <christophe.lyon@linaro.org> 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.
Done
>
> > + }
> > +
> > 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.)
You are right: it's not needed anymore, I forgot to remove it.
>
> > [...]
> > @@ -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".
>
Done
> > + {
> > + 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 ();
> }
>
OK
> > +}
> > +
> > /* 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.)
>
OK, done
> > + {
> > + /* 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).
Right, it belongs to patch 10/21 "Implement TLS support".
The initialization avoids a warning about possibly non-initialized variable.
>
> > [...]
> > @@ -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))
>
> ?
You are right, it was not intentional.
Unfortunately, it does not introduce a regression on
arm-none-linux-gnueabi[hf] with the GCC testsuite.
Fixed.
>
> > + }
> > + }
> > +
> > + /* 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.
>
OK
> > [...]
> > @@ -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.
OK
>
> > +
> > + 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.
OK
>
> Looks good otherwise, thanks.
>
Here is an updated version, thanks!
> Richard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ARM-FDPIC-Add-support-for-FDPIC-for-arm-architecture.patch
Type: text/x-patch
Size: 19039 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190904/dabed8dd/attachment.bin>
More information about the Gcc-patches
mailing list