Re: Proposed changes for pdp11 --*magic options

[Nick Clifton <nickc@redhat.com>]

Hi Stephen,

  First of all, please accept my apologies for taking such a long
  time to reply to your email.

  Secondly to answer your questions:

>   - I have used the existing boolean ld_config_type::separate_code to
>     indicate when --imagic is seen in the option parsing.  To me its
>     meaning seemed consistent with the new way I'm using it, but I
>     could add another boolean if that would be preferred.

No, reusing the current logic makes sense to me too.

>   - Rather than define another flag bit in bfd_target::object_flags,
>     I'm copying the state of the separate_code boolean into a local
>     variable in bfd/pdp11.c at _final_link() time for reference later
>     in adjust_sizes_and_vmas() as the sections are written.

I don't particularly like this approach, but I think that it is the
best solution without making major changes to the BFD library.  Ideally
the link_info structure should be passed down to adjust_sizes_and_vmas
but this would mean changing target vectors.
 
>   - Since the --imagic format intentionally causes the .text and .data
>     sections to occupy the same memory addresses, I had to set
>     command_line.check_section_addresses = 0.  Would that cause any
>     other problems?

None that I can think of.  I would however recommend adding a comment
where you set check_section_addresses to zero, so that readers will
understand why it is done.

>   - In my generated epdp11.c, the function name prefix is gldpdp11
>     rather than gld_pdp11 since that is how the code was written in
>     the emulation from which I borrowed code.  But I also saw the
>     underscore included in a different emulation.  Is one of these
>     considered correct?

No, although personally I like the presence of the underscore as it
helps to separate the name of the target from the name of the tool.

> The two one-line changes that I needed to make in common code are as
> follows:
> 
>   - I needed to add i_magic to enum aout_magic in bfd/libaout.h.
> 
>   - In ld/lexsup.c, I needed to set config.text_read_only to TRUE in
>     case 'n'.  This is the same as the default value of that boolean
>     as initialized in ldmain.c so I assume my change won't affect any
>     non-pdp11 emulation.  I need this change so that my change of the
>     default value for pdp11 will work.  That boolean is initialized to
>     FALSE or TRUE in case 'N' and case OPTION_NO_OMAGIC.

I have no problem with these changes.

> The diff is attached.

The diff itself is fine.  I do have a few requests however:

  * Please could you add documentation to ld/ld.texi describing
    the new option, what it does, and also mentioning that it is
    only for the PDP11.

  * Please could you add a line to ld/NEWS mentioning the new feature. 

  * It would be nice if you could create a new linker test to check
    the behaviour of the new feature.  I appreciate that there are no
    PDP11 specific tests in the linker testsuite (or aout specific tests
    for that matter).  But there is no reason that you cannot add a 
    new directory and put the test(s) inside it.

  * If/when you do decide to submit the patch for inclusion, please
   could you also provide ChangeLog entries describing what is happening.

One final thing - which really should have been the first item - I do not
think that we have an FSF Copyright Assignment on file for you for 
contributions to the binutils project.  A change of this size/complexity
cannot be considered to be "obvious", so an assignment will be needed.
If you would like to fill out the form here and email it off, that will
start the process rolling:

  http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob_plain;f=doc/Copyright/request-assign.changes;hb=HEAD

Cheers
  Nick