Re: [patch] Handle multiple ELF function types.

[Nick Clifton <nickc at redhat dot com>]

Hi Paul,

> 2007-04-03  Paul Brook  <paul@codesourcery.com>
>
> 	bfd/
> 	* elf.c (_bfd_elf_is_function_type): New function.
> 	* elflink.c (_bfd_elf_merge_symbol): Use bed->is_function_type.
> 	(_bfd_elf_dynamic_symbol_p, _bfd_elf_symbol_refs_local_p,
> 	is_global_data_symbol_definition, elf_link_add_object_symbols): Ditto.
> 	* elf-bfd.h (elf_backend_data): Add is_function_type.
> 	(_bfd_elf_is_function_type): Add prototype.
> 	* elfxx-target.h (elf_backend_is_function_type): Add default
> 	definition.
> 	(elfNN_bed): Add elf_backend_is_function_type.
> 	* elf32-arm.c (elf32_arm_is_function_type): New function.
> 	(elf_backend_is_function_type): Define.
>
> 	ld/testsuite/
> 	* ld-arm/preempt-app.s: New test.
> 	* ld-arm/preempt-app.sym: New.
> 	* ld-arm/arm-elf.exp: Add preempt-app.


Andreas has already mentioned a few formatting nits.  I have two other 
small corrections that I would like to see made:


> +  /* Return TRUE is type is a function symbol type.  */
> +  bfd_boolean (*is_function_type) (unsigned int type);

The comment should be "Return TRUE *if* type is a function symbol type."


> +/* Return TRUE for STT_FUNC.  */
> +
> +bfd_boolean
> +_bfd_elf_is_function_type (unsigned int type)
> +{
> +  return (type == STT_FUNC);
> +}

I believe that the comment here is misleading.  I think that it should 
say something like:

  Return TRUE for ELF symbol types that represent functions.

Possibly some extra text might be useful as well:

  This is the default version of this function which is sufficient
  for most ELF based targets.  It returns TRUE for symbols whose
  type is STT_FUNC.

The point being that the purpose of the function is not to detect types 
whose value is STT_FUNC, but to detect types whose meaning is "I am a 
function symbol".


The patch is approved with the changes pointed out by Andreas and myself.

Cheers
  Nick