RE: [PATCH} Add aarch64-pe support to LD and GAS; refine support in BFD

[Tamar Christina <Tamar.Christina@arm.com>]

Hi Jedidiah,

Thanks for respinning the patch, a couple more comments below:

> 
> From: Jedidiah Thompson <wej22007@outlook.com> 
> Sent: Monday, December 27, 2021 5:07 PM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH} Add aarch64-pe support to LD and GAS; refine support in BFD
> 
> Here is the updated patch:
> 
> --- a/bfd/config.bfd
> +++ b/bfd/config.bfd
> @@ -249,7 +249,12 @@ case "${targ}" in
>      ;;
>    aarch64-*-elf | aarch64-*-rtems* | aarch64-*-genode*)
>      targ_defvec=aarch64_elf64_le_vec
> -    targ_selvecs="aarch64_elf64_be_vec aarch64_elf32_le_vec aarch64_elf32_be_vec arm_elf32_le_vec arm_elf32_be_vec aarch64_pei_vec"
> +    targ_selvecs="aarch64_elf64_be_vec aarch64_elf32_le_vec aarch64_elf32_be_vec arm_elf32_le_vec arm_elf32_be_vec aarch64_pei_le_vec"
> +    want64=true
> +    ;;
> +  aarch64-*-pe)
> +    targ_defvec=aarch64_pei_le_vec
> +    targ_selvecs="aarch64_pe_le_vec aarch64_elf64_le_vec arm_pe_le_vec arm_pei_le_vec"
>      want64=true

64-bit PE is not an underscore platform, so you want targ_underscore=no here too.
Also you want the executable format not the image format as the default here. So
targ_defvec=aarch64_pe_le_vec instead of pei.

>      ;;
>    aarch64_be-*-elf)
> @@ -279,7 +284,7 @@ case "${targ}" in
>      ;;
>    aarch64-*-linux* | aarch64-*-netbsd*)
>      targ_defvec=aarch64_elf64_le_vec
> -    targ_selvecs="aarch64_elf64_be_vec aarch64_elf32_le_vec aarch64_elf32_be_vec arm_elf32_le_vec arm_elf32_be_vec aarch64_pei_vec"
> +    targ_selvecs="aarch64_elf64_be_vec aarch64_elf32_le_vec aarch64_elf32_be_vec arm_elf32_le_vec arm_elf32_be_vec aarch64_pei_le_vec"
>      want64=true
>      ;;
>    aarch64_be-*-linux* | aarch64_be-*-netbsd*)
> @@ -1534,3 +1539,9 @@ case "${targ_defvec} ${targ_selvecs}" in
>      targ_archs="$targ_archs bfd_k1om_arch"
>      ;;
>  esac
> +
> +if test x"$targ_defvec" = x"aarch64-pe"; then
> +  # Not currently complete (and probably not stable), warn user
> +  echo "*** WARNING BFD aarch64-pe support not complete nor stable"
> +  echo "*** Do not rely on this for production purposes"
> +fi

> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index cc7725483aa..811f3eedbaa 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -1475,7 +1475,7 @@ s_unreq (int a ATTRIBUTE_UNUSED)
>  
>  /* Directives: Instruction set selection.  */
>  
> -#ifdef OBJ_ELF
> +#if defined (OBJ_ELF) || defined (OBJ_COFF)
>  /* This code is to handle mapping symbols as defined in the ARM AArch64 ELF
>     spec.  (See "Mapping symbols", section 4.5.4, ARM AAELF64 version 0.05).
>     Note that previously, $a and $t has type STT_FUNC (BSF_OBJECT flag),
> @@ -2029,6 +2029,7 @@ s_aarch64_inst (int ignored ATTRIBUTE_UNUSED)
>    demand_empty_rest_of_line ();
>  }
>  
> +#ifdef OBJ_ELF
>  static void
>  s_aarch64_cfi_b_key_frame (int ignored ATTRIBUTE_UNUSED)
>  {
> @@ -2037,7 +2038,6 @@ s_aarch64_cfi_b_key_frame (int ignored ATTRIBUTE_UNUSED)
>    fde->pauth_key = AARCH64_PAUTH_KEY_B;
>  }
>  
> -#ifdef OBJ_ELF
>  /* Emit BFD_RELOC_AARCH64_TLSDESC_ADD on the next ADD instruction.  */
>  
>  static void
> @@ -2111,8 +2111,8 @@ const pseudo_typeS md_pseudo_table[] = {
>    {"arch", s_aarch64_arch, 0},
>    {"arch_extension", s_aarch64_arch_extension, 0},
>    {"inst", s_aarch64_inst, 0},
> -  {"cfi_b_key_frame", s_aarch64_cfi_b_key_frame, 0},
>  #ifdef OBJ_ELF
> +  {"cfi_b_key_frame", s_aarch64_cfi_b_key_frame, 0},
>    {"tlsdescadd", s_tlsdescadd, 0},
>    {"tlsdesccall", s_tlsdesccall, 0},
>    {"tlsdescldr", s_tlsdescldr, 0},
> @@ -8302,7 +8302,7 @@ aarch64_handle_align (fragS * fragP)
>    fix = bytes & (noop_size - 1);
>    if (fix)
>      {
> -#ifdef OBJ_ELF
> +#if defined (OBJ_ELF) || defined (OBJ_COFF)
>        insert_data_mapping_symbol (MAP_INSN, fragP->fr_fix, fragP, fix);
>  #endif
>        memset (p, 0, fix);
> @@ -9044,9 +9044,13 @@ md_apply_fix (fixS * fixP, valueT * valP, segT seg)
>        break;
>  
>      case BFD_RELOC_AARCH64_TLSIE_LD_GOTTPREL_LO12_NC:
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>        fixP->fx_r_type = (ilp32_p
>           ? BFD_RELOC_AARCH64_TLSIE_LD32_GOTTPREL_LO12_NC
>           : BFD_RELOC_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC);
> +#else
> +      fixP->fx_r_type = BFD_RELOC_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC;
> +#endif

not sure I understand why these changes are needed. ilp32_p is based on the ABI and since
-mabi isn't defined for aarch64-*pe you can't ever really set it. So all of these and the below
should already be correct without the ifdefs.

That said the default ABI looks wrong, The default ABI will be set to AARCH64_ABI_LP64.
Since PE/Windos is IL32P64 you'll want a new ABI, AARCH64_ABI_IL32P64 and make that the default in
aarch64_after_parse_args moving that ouf of the ifdef OBJ_ELF.

You'll also want to change TARGET_FORMAT to set the default format for the BFD target.

>        S_SET_THREAD_LOCAL (fixP->fx_addsy);
>        /* Should always be exported to object file, see
>     aarch64_force_relocation().  */
> @@ -9055,10 +9059,14 @@ md_apply_fix (fixS * fixP, valueT * valP, segT seg)
>        break;
>  
>      case BFD_RELOC_AARCH64_TLSDESC_LD_LO12_NC:
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>        fixP->fx_r_type = (ilp32_p
>           ? BFD_RELOC_AARCH64_TLSDESC_LD32_LO12_NC
>           : BFD_RELOC_AARCH64_TLSDESC_LD64_LO12);
> +#else
> +      fixP->fx_r_type = BFD_RELOC_AARCH64_TLSDESC_LD64_LO12;
>        S_SET_THREAD_LOCAL (fixP->fx_addsy);
> +#endif
>        /* Should always be exported to object file, see
>     aarch64_force_relocation().  */
>        gas_assert (!fixP->fx_done);
> @@ -9127,9 +9135,13 @@ md_apply_fix (fixS * fixP, valueT * valP, segT seg)
>      case BFD_RELOC_AARCH64_LD_GOT_LO12_NC:
>        /* Should always be exported to object file, see
>     aarch64_force_relocation().  */
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>        fixP->fx_r_type = (ilp32_p
>           ? BFD_RELOC_AARCH64_LD32_GOT_LO12_NC
>           : BFD_RELOC_AARCH64_LD64_GOT_LO12_NC);
> +#else
> +      fixP->fx_r_type = BFD_RELOC_AARCH64_LD64_GOT_LO12_NC;
> +#endif
>        gas_assert (!fixP->fx_done);
>        gas_assert (seg->use_rela_p);
>        break;
> @@ -9627,7 +9639,11 @@ md_begin (void)
>    cpu_variant = *mcpu_cpu_opt;
>  
>    /* Record the CPU type.  */
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>    mach = ilp32_p ? bfd_mach_aarch64_ilp32 : bfd_mach_aarch64;
> +#else
> +  mach = bfd_mach_aarch64;
> +#endif
>  
>    bfd_set_arch_mach (stdoutput, TARGET_ARCH, mach);
>  }
> @@ -10188,6 +10204,7 @@ aarch64_parse_arch (const char *str)
>  }
>  
>  /* ABIs.  */
> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF)
>  struct aarch64_option_abi_value_table
>  {
>    const char *name;
> @@ -10220,6 +10237,7 @@ aarch64_parse_abi (const char *str)
>    as_bad (_("unknown abi `%s'\n"), str);
>    return 0;
>  }
> +#endif /* OBJ_ELF || OBJ_MAYBE_ELF */
>  
>  static struct aarch64_long_option_table aarch64_long_opts[] = {
>  #ifdef OBJ_ELF
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 78bff0a1b56..ccc4a4c3f0c 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -169,7 +169,7 @@ void aarch64_elf_copy_symbol_attributes (symbolS *, symbolS *);
>  struct aarch64_frag_type
>  {
>    int recorded;
> -#ifdef OBJ_ELF
> +#if defined (OBJ_ELF) || defined (OBJ_COFF)
>    /* If there is a mapping symbol at offset 0 in this frag,
>       it will be saved in FIRST_MAP.  If there are any mapping
>       symbols in this frag, the last one will be saved in
> @@ -242,6 +242,34 @@ struct aarch64_segment_info_type
>  extern void aarch64_after_parse_args (void);
>  #define md_after_parse_args() aarch64_after_parse_args ()
>  
> +#elif defined(TE_PEP)
> +# define GLOBAL_OFFSET_TABLE_NAME "__GLOBAL_OFFSET_TABLE_"
> +# define TC_SEGMENT_INFO_TYPE          struct aarch64_segment_info_type
> +
> +/* This is not really an alignment operation, but it's something we
> +   need to do at the same time: whenever we are figuring out the
> +   alignment for data, we should check whether a $d symbol is
> +   necessary.  */
> +# define md_cons_align(nbytes)         mapping_state (MAP_DATA)
> +
> +enum mstate
> +{
> +  MAP_UNDEFINED = 0, /* Must be zero, for seginfo in new sections.  */
> +  MAP_DATA,
> +  MAP_INSN,
> +};
> +
> +void mapping_state (enum mstate);
> +
> +struct aarch64_segment_info_type
> +{
> +  const char *last_file;
> +  unsigned last_line;
> +  enum mstate mapstate;
> +  unsigned int marked_pr_dependency;
> +  aarch64_instr_sequence insn_sequence;
> +};
> +

Any reason you can't just remove the OBJ_ELF or include OBJ_COFF here?
This looks exactly the same code as the OBJ_ELF block and in principle
CFIs should be handled by BFD already.

>  #else /* Not OBJ_ELF.  */
>  #define GLOBAL_OFFSET_TABLE_NAME "__GLOBAL_OFFSET_TABLE_"
>  #endif
> @@ -274,13 +302,13 @@ extern void aarch64_handle_align (struct frag *);
>  extern int tc_aarch64_regname_to_dw2regnum (char *regname);
>  extern void tc_aarch64_frame_initial_instructions (void);
>  
> -#ifdef TE_PE
> +#if 0
>  
>  #define O_secrel O_md1
>  
>  #define TC_DWARF2_EMIT_OFFSET  tc_pe_dwarf2_emit_offset
>  void tc_pe_dwarf2_emit_offset (symbolS *, unsigned int);
>  
> -#endif /* TE_PE */
> +#endif

This looks like copied left over code from the Arm implementation, just delete the hunk.

Thanks,
Tamar