Re: [ARM/FDPIC v5 04/21] [ARM] FDPIC: Add support for FDPIC for arm architecture

[Richard Sandiford <richard dot sandiford at arm dot com>]

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.

> +    }
> +
>    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