Re: [PATCH] FT32: support for FT32B processor - part 2/2

[Nick Clifton <nickc at redhat dot com>]

Hi James,

> OK to commit?

I am worried about this patch because currently it is not possible to test it.
IE the framework to support the FT32B type has not yet gone in to the sources.
Did I miss a patch that adds this ?  (Plus I assume that there is a patch to
the top level config.sub file which needs to go into the gcc sources so that
it can be imported into the binutils sources).

As for the patch itself, I do have a few comments based upon a visual inspection:


> +	      offset -= (input_section->output_section->vma +
> +			 input_section->output_offset);
> +	      if ((offset < -1024) || (offset >= 1024))
> +		abort ();

This may be just a personal thing, but I do not like calls to abort() inside
a library.  I would much rather than an error message was generated and an
error result returned.  In this particular case for example you could return 
a bfd_reloc_outofrange value, and maybe call bfd_error_handler() to display
the invalid offset.  [There are other, similar cases elsewhere in the patch].


> @@ -345,19 +478,19 @@ ft32_elf_relocate_section (bfd *output_bfd,
>  	      break;
>  
>  	    case bfd_reloc_outofrange:
> -	      msg = _("internal error: out of range error");
> +	      msg = _ ("internal error: out of range error");
>  	      break;
>  
>  	    case bfd_reloc_notsupported:
> -	      msg = _("internal error: unsupported relocation error");
> +	      msg = _ ("internal error: unsupported relocation error");
>  	      break;
>  
>  	    case bfd_reloc_dangerous:
> -	      msg = _("internal error: dangerous relocation");
> +	      msg = _ ("internal error: dangerous relocation");
>  	      break;
>  
>  	    default:
> -	      msg = _("internal error: unknown error");
> +	      msg = _ ("internal error: unknown error");
>  	      break;
>  	    }

There is no need for this change.  The _ function is a special case that
does not follow the normal whitespace-between-function-name-and-opening-
parenthesis rule.


> +static bfd_boolean
> +ft32_reloc_shortable

> +	return 0;

bfd_boolean functions should return FALSE or TRUE rather than 0 or 1.


> +/* Reduce the diff value written in the section by count if the shrinked
> +   insn address happens to fall between the two symbols for which this
> +   diff reloc was emitted.  */
> +
> +static void
> +elf32_ft32_adjust_diff_reloc_value (bfd *abfd,
> +				   struct bfd_section *isec,
> +				   Elf_Internal_Rela *irel,
> +				   bfd_vma symval,
> +				   bfd_vma shrinked_insn_address,
> +				   int count)
> +{

This function should probably be boolean, indicating whether it has
succeeded or failed.  (plus, of course, its callers should check the
return value).

   
> +  int fixed = 0;
> +
>    /* Drop leading whitespace.  */

If fixed is a boolean variable then it really ought to have the bfd_boolean type.



One other thing - with a change like this, introducing major new functionality
to a target, it would be a really good idea to include some testsuite tests.  
You definitely want to make sure that the assembler can create these new format
objects and the disassembler can reconstruct them correctly.

Cheers
  Nick
-