Re: [PATCH, FT32] initial support (2nd attempt)

[Alan Modra <amodra at gmail dot com>]

On Sat, Jan 03, 2015 at 05:16:34PM +0000, James Bowman wrote:
> This patch adds FT32 support to binutils. Please can someone review it, and if appropriate
> commit it, as I do not have write access to the tree.

I don't see any gas testsuite for your processor.  You'd be wise to
add one that at least checks all opcodes.

> +  HOWTO (R_FT32_32,		/* type */
> +	 0,			/* rightshift */
> +	 2,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 32,			/* bitsize */
> +	 FALSE,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_bitfield, /* complain_on_overflow */
> +	 bfd_elf_generic_reloc,	/* special_function */
> +	 "R_FT32_32",		/* name */
> +	 FALSE,			/* partial_inplace */
> +	 0x00000000,		/* src_mask */
> +	 0x000fffff,		/* dst_mask */
> +	 FALSE),		/* pcrel_offset */

dst_mask is wrong for a 32-bit reloc.

> +/* Perform a single relocation.  By default we use the standard BFD
> +   routines, but a few relocs, we have to do them ourselves.  */

Which few relocs?  Please omit ft32_final_link_relocate.

> +/* Return the section that should be marked against GC for a given
> +   relocation.  */
> +
> +static asection *
> +ft32_elf_gc_mark_hook (asection *sec,
> +			struct bfd_link_info *info,
> +			Elf_Internal_Rela *rel,
> +			struct elf_link_hash_entry *h,
> +			Elf_Internal_Sym *sym)
> +{
> +  return _bfd_elf_gc_mark_hook (sec, info, rel, h, sym);
> +}

Likewise, omit this function.

> +
> +/* Look through the relocs for a section during the first phase.
> +   Since we don't do .gots or .plts, we just need to consider the
> +   virtual table relocs for gc.  */
> +
> +static bfd_boolean
> +ft32_elf_check_relocs (bfd *abfd,
> +			struct bfd_link_info *info,
> +			asection *sec,
> +			const Elf_Internal_Rela *relocs)

And this one too, since it does nothing.

> +void
> +md_operand (expressionS *op __attribute__((unused)))
> +{
> +  /* Empty for now. */
> +}

Rather than this,
#define md_operand(x)
in tc-ft32.h

> +void
> +md_apply_fix (fixS *fixP ATTRIBUTE_UNUSED,
> +	      valueT * valP ATTRIBUTE_UNUSED, segT seg ATTRIBUTE_UNUSED)
> +{
> +  char *buf = fixP->fx_where + fixP->fx_frag->fr_literal;
> +  long val = *valP;
> +  long max, min;

Please remove min, max since you don't use them in any meaningful way.

> +  long newval;
> +
> +  max = min = 0;
> +  switch (fixP->fx_r_type)
> +    {
> +    case BFD_RELOC_32:
> +      buf[3] = val >> 24;
> +      buf[2] = val >> 16;
> +      buf[1] = val >> 8;
> +      buf[0] = val >> 0;
> +      buf += 4;

Also, incrementing buf is useless.  Please remove here and elsewhere.

> +#if 0
> +arelent *
> +tc_gen_reloc (asection *section ATTRIBUTE_UNUSED, fixS *fixp)

Please remove all #if 0 code.

> +  /* This is the standard place for KLUDGEs to work around bugs in
> +     bfd_install_relocation (first such note in the documentation
> +     appears with binutils-2.8).
> +
> +     That function bfd_install_relocation does the wrong thing with
> +     putting stuff into the addend of a reloc (it should stay out) for a
> +     weak symbol.  The really bad thing is that it adds the
> +     "segment-relative offset" of the symbol into the reloc.  In this
> +     case, the reloc should instead be relative to the symbol with no
> +     other offset than the assembly code shows; and since the symbol is
> +     weak, any local definition should be ignored until link time (or
> +     thereafter).
> +     To wit:  weaksym+42  should be weaksym+42 in the reloc,
> +     not weaksym+(offset_from_segment_of_local_weaksym_definition)
> +
> +     To "work around" this, we subtract the segment-relative offset of
> +     "known" weak symbols.  This evens out the extra offset.
> +
> +     That happens for a.out but not for ELF, since for ELF,
> +     bfd_install_relocation uses the "special function" field of the
> +     howto, and does not execute the code that needs to be undone.  */
> +
> +  if (OUTPUT_FLAVOR == bfd_target_aout_flavour
> +      && fixP->fx_addsy && S_IS_WEAK (fixP->fx_addsy)
> +      && ! bfd_is_und_section (S_GET_SEGMENT (fixP->fx_addsy)))
> +    {
> +      relP->addend -= S_GET_VALUE (fixP->fx_addsy);
> +    }

Why this code, when you don't support AOUT?

> -emoxiebox.c: $(srcdir)/emulparams/moxiebox.sh \
> -  $(ELF_GEN_DEPS) $(srcdir)/scripttempl/elf.sc ${GEN_DEPENDS}
> -

You seem to have taken a dislike to this target??

-- 
Alan Modra
Australia Development Lab, IBM