Re: Adding options for redefining symbols

[Nick Clifton <nickc at redhat dot com>]

Hi Ivan,

  First of all thank you for your interest in the binutils, and 
  posting this patch.


> I found myself in need of redefining only the symbols that correspond
> to defined functions, not the ones that are undefined declarations.

  I have to say my immediate thought was "why bother ?".  There are
  already --redefine-sym and --redefine-syms options, so why not use
  those.  I can see you might want to have a check that the symbol
  really is defined (or undefined) before changing it.  But you could
  achieve that with a small script inserted before calling objcopy,
  and then there would be no need to patch anything.

>     - Is this kind of feature a feasable candidate?

  Well yes and no.  Yes, it is feasible because you have already
  written a patch that does it.  But no, because as mentioned above
  it might be possible to achieve the same goal without needing to
  patch anything.

>     - Setting redefine_mode uses switch fallthroughs, is this a
> readability no-no?

  Nope that is fine.

>     - Do I need to find similar alternatives for "--redefine-syms=<filename>"?

  For consistency's sake I would say yes.  There is a limit on
  the length of a command line, and if the feature needs to be
  used for lots of symbols, then having this alternative would
  be really useful.

> Example usage, first redefining for using implementation_a, then
> changing the same object file to instead use implementation_b:
> 
>     $ objcopy --redefine-sym implementation_a=do_work
>     $ objcopy --redefine-defined-sym do_work=implementation_a
> --redefine-sym implementation_b=do_work

  Now this confuses me.  The first command line makes sense, but
  the second one appears to say "rename function 'do_work' to be
  called 'implementation_a' and rename function 'implementation_b'
  to be called 'do_work'".  But this does not match your description
  of using implementation_b.
 

  I also have a comment on the patch itself:

    +  char mode;

  I would suggest that you use an enum here, rather than a char 
  and some magic numeric values.  It does make the patch slightly
  larger, but also easier to read.

  One final point - are you aware that in order to contribute a
  patch like this you will need an FSF copyright assignment in
  place ?

Cheers
  Nick