Re: [ARM/FDPIC 08/21] [ARM] FDPIC: Fix corner case of weak symbol

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Christophe,

On 25/05/18 09:03, Christophe Lyon wrote:
> When checking the address of a weak symbol in an executable, it is
> mandatory to use the GOTFUNCDESC scheme so that the address==NULL
> semantic is valid if the symbol is not present in the final link.
>
> 2018-XX-XX  Christophe Lyon  <christophe.lyon@st.com>
>         Mickaël Guêné <mickael.guene@st.com>
>
>         gcc/
>         * config/arm/arm.c (arm_local_funcdesc_p): New function.
>         (legitimize_pic_address): Handle weak symbols in FDPIC mode.
>         (arm_assemble_integer): Likewise.
>
> Change-Id: I3fa0b63bc0f672903f405aa72cc46052de1c0feb
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index bbf8884..025485d 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -3771,6 +3771,46 @@ arm_options_perform_arch_sanity_checks (void)
>      }
>  }
>
> +/* Test whether a local function descriptor is canonical, i.e.,
> +   whether we can use GOTOFFFUNCDESC to compute the address of the
> +   function.  */
> +static bool
> +arm_local_funcdesc_p (rtx fnx)
> +{
> +  tree fn;
> +  enum symbol_visibility vis;
> +  bool ret;
> +
> +  if (!TARGET_FDPIC)
> +    return TRUE;
> +
> +  if (! SYMBOL_REF_LOCAL_P (fnx))
> +    return FALSE;
> +
> +  fn = SYMBOL_REF_DECL (fnx);
> +
> +  if (! fn)
> +    return FALSE;
> +
> +  /* Local function declared as weak must use GOTFUNCDESC so their
> +     FUNCDESC is NULL if they are not linked in.  */
> +  if (DECL_WEAK(fn))
> +    return FALSE;
> +
> +  vis = DECL_VISIBILITY (fn);
> +
> +  if (vis == VISIBILITY_PROTECTED)
> +    /* Private function descriptors for protected functions are not
> +       canonical.  Temporarily change the visibility to global.  */
> +    vis = VISIBILITY_DEFAULT;
> +
> +  ret = default_binds_local_p_1 (fn, flag_pic);
> +
> +  DECL_VISIBILITY (fn) = vis;
> +

I'm a bit suspicious of the above few lines. Does this function end up changing the visibility
of 'fn'? If so, this function is not a pure predicate function and should be documented that it
modifies some part of its argument. Or did you mean to change the visibility before the call to
default_binds_local_p_1 and restore it afterwards?

Thanks,
Kyrill

> +  return ret;
> +}
> +
>  static void
>  arm_add_gc_roots (void)
>  {
> @@ -7488,7 +7528,9 @@ legitimize_pic_address (rtx orig, machine_mode mode, rtx reg)
>             || (GET_CODE (orig) == SYMBOL_REF
>                 && SYMBOL_REF_LOCAL_P (orig)
>                 && (SYMBOL_REF_DECL (orig)
> -                  ? !DECL_WEAK (SYMBOL_REF_DECL (orig)) : 1)))
> +                  ? !DECL_WEAK (SYMBOL_REF_DECL (orig)) : 1)
> +              && (!SYMBOL_REF_FUNCTION_P(orig)
> +                  || arm_local_funcdesc_p (orig))))
>            && NEED_GOT_RELOC
>            && arm_pic_data_is_text_relative)
>          insn = arm_pic_static_addr (orig, reg);
> @@ -23072,7 +23114,9 @@ arm_assemble_integer (rtx x, unsigned int size, int aligned_p)
>                || (GET_CODE (x) == SYMBOL_REF
>                    && (!SYMBOL_REF_LOCAL_P (x)
>                        || (SYMBOL_REF_DECL (x)
> -                         ? DECL_WEAK (SYMBOL_REF_DECL (x)) : 0))))
> +                         ? DECL_WEAK (SYMBOL_REF_DECL (x)) : 0)
> +                     || (SYMBOL_REF_FUNCTION_P (x)
> +                         && !arm_local_funcdesc_p (x)))))
>              {
>                if (TARGET_FDPIC && SYMBOL_REF_FUNCTION_P (x))
>                  fputs ("(GOTFUNCDESC)", asm_out_file);
> --
> 2.6.3