Re: [PATCH] Add plugin interface to LD [2/4] Claim files and add symbols.

[Richard Henderson <rth at redhat dot com>]

On 09/22/2010 10:31 PM, Dave Korn wrote:
> - I'd certainly appreciate a second pair of eyes looking at the logic in
> asymbol_from_plugin_symbol by which I derive the fags etc. for a bfd asymbol
> from the ld plugin symbol struct.

It does seem to be broken.  I see no way you're going to
be able to generate cigarettes from that code.

> -	      return TRUE;
> +	      checked_ok = TRUE;

I wonder if a "goto success" wouldn't be cleaner.

> +  int fildes = bfd_check_format (entry->the_bfd, bfd_object)
> +		? open (attempt, O_RDONLY
> +# ifdef O_BINARY
> +				| O_BINARY
> +# endif /* O_BINARY */
> +			)
> +		: -1;

You already caught the c99 issue yourself.  As for O_BINARY,
better to put that in sysdeps.h with the rest of the O_FLAGS.

> +/* Don't include std headers until after config.h, sysdeps.h etc.,
> +   or you may end up with the wrong bitsize of off_t.  */
> +#include <dlfcn.h>

Better to do this in sysdeps.h, with ifdef protection.  I like
having everything system related done in the one place.


> +  asym->section = ldsym->def == LDPK_COMMON
> +	? bfd_com_section_ptr
> +	: (ldsym->def == LDPK_UNDEF || ldsym->def == LDPK_WEAKUNDEF
> +		? bfd_und_section_ptr
> +		: bfd_get_section_by_name (abfd, ".text"));

This is pretty hard to read as-is.  Better as

  flagword flags = BSF_NO_FLAGS;
  struct bfd_section *section;

  switch (ldsym->def)
    {
    case LDPK_WEAKDEF:
      flags = BSF_WEAK;
      /* FALLTHRU */
    case LDPK_DEF:
      flags |= BSF_GLOBAL;
      section = bfd_get_section_by_name (abfd, ".text");
      break;

    case LDPK_WEAKUNDEF:
      flags = BSF_WEAK;
      /* FALLTHRU */
    case LDPK_UNDEF:
      section = bfd_und_section_ptr;
      break;

    case LDPK_COMMON:
      flags = BSF_GLOBAL;
      section = bfd_com_section_ptr;
      break;

    default:
      return LDPS_ERR;
    }
  asym->flags = flags;
  asym->section = section;


> +      rv = asymbol_from_plugin_symbol (abfd, bfdsym, syms++);

Better I think just as syms + n.  I prefer not to increment
induction variables at non-obvious spots inside the loop.
In this case we can rely on the compiler producing the ++
if it really turns out to be useful.


r~