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

Dave Korn dave.korn.cygwin@gmail.com
Thu Oct 7 21:42:00 GMT 2010


On 06/10/2010 23:14, Richard Henderson wrote:
> 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.

  LOL, only thing broken there was my brain... I meant flags of course!

>> -	      return TRUE;
>> +	      checked_ok = TRUE;
> 
> I wonder if a "goto success" wouldn't be cleaner.

  Will do.

>> +  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.

  Will do.

> 
>> +/* 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.

  OK.  Leaving it here worked out when --enable-plugins was not the default
and there wasn't autoconfigury to detect the dl* functions/headers, but since
I'm adding that (see previous reply) I can easily move this now that there'll
be a HAVE_DLFCN_H for it.


> 
>> +  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

  Thanks, will refactor it into a switch as you suggest.

>> +      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.

  OK, will do.  Thanks for reviewing!

    cheers,
      DaveK



More information about the Binutils mailing list