RE: Re C4x patch

RE: Re C4x patch

[Greg Smart]

hi Nick,
	I would prefer that the patch I made against binutils-2.9.1 is
not used to patch against the CVS tree.  When I originally created a
patch against the CVS tree in June, I uncovered lots of little issues
like the ones you have mentioned, and so I have fixed a lot of these
problems already.  As Ian mentioned, the main concern is that I
haven't adequately ensured that I don't break other ports.  Your
help here is greatly appreciated. :) 
I have generated a patch against CVS@24-dec-1999, but I'm having
a problem in that gas core dumps.  I am planning to send this to you
as soon as I get this sorted out, as well as addressing your other
comments.
I have also broken the patch up into 2 patches.  The first patch contains
only new files, so is guaranteed not to break any existing code, and the
second patch is all the modifications to existing code.  If you want, I can
send you the first patch to start the review process.

cheers
Greg

> -----Original Message-----
> From:	Nick Clifton [SMTP:nickc@cygnus.com]
> Sent:	Tuesday, December 28, 1999 6:19 AM
> To:	ian@zembu.com
> Cc:	joel.sherrill@OARcorp.com; GSmart@tennyson.com.au;
> m.hayes@elec.canterbury.ac.nz; binutils@sourceware.cygnus.com
> Subject:	Re: Re C4x patch
> 
> Hi Ian,
> 
> : Nick, as you know, please be extremely careful about any patches to
> : any files which are not specific to the C4x.
> 
> Just to set people's minds at rest I have not actually checked *any*
> of Joel's patch into the source tree yet.  When I said "applied" in my
> comments what I meant was that I had applied his patch to my checked
> out tree in order to build a C4X target.  I then generated a new patch
> based on this work and mailed it to Joel.
> 
> My hope is that he will be able to check to see if this revised patch
> actually works for the C4X (since I assume that he can test it on real
> hardware, whereas I cannot) and that he will then resubmit this patch
> with whatever tweaks are needed, and with ChangeLog entries, so that
> it can be properly reviewed and accepted.
> 
> Cheers
> 	Nick

Re: Re C4x patch

[Nick Clifton]

Hi Ian,

: Nick, as you know, please be extremely careful about any patches to
: any files which are not specific to the C4x.

Just to set people's minds at rest I have not actually checked *any*
of Joel's patch into the source tree yet.  When I said "applied" in my
comments what I meant was that I had applied his patch to my checked
out tree in order to build a C4X target.  I then generated a new patch
based on this work and mailed it to Joel.

My hope is that he will be able to check to see if this revised patch
actually works for the C4X (since I assume that he can test it on real
hardware, whereas I cannot) and that he will then resubmit this patch
with whatever tweaks are needed, and with ChangeLog entries, so that
it can be properly reviewed and accepted.

Cheers
	Nick

Re: Re C4x patch

[Ian Lance Taylor]

   Date: Thu, 23 Dec 1999 12:33:42 -0800
   From: Nick Clifton <nickc@cygnus.com>

     * This patch (to bfd/coffcode.h) would not apply.  I could not
       locate this code anywhere, so I have left it out.

   *** 1068,1078 ****
   --- 1068,1080 ----
	  that is also how gdb locates the section.
	  We need to handle the .ctors and .dtors sections similarly, to
	  avoid introducing null words in the tables.  */
   + #ifndef C4XMAGIC
       if (COFF_DEFAULT_SECTION_ALIGNMENT_POWER > 2
	   && (strncmp (section->name, ".stab", 5) == 0
	     || strcmp (section->name, ".ctors") == 0
	     || strcmp (section->name, ".dtors") == 0))
	 section->alignment_power = 2;
   + #endif

This has been replaced by the coff_set_custom_section_alignment
support, in which the target file defines
COFF_SECTION_ALIGNMENT_ENTRIES if necessary.

The patch above is almost certainly incorrect.  It's hard for me to
imagine any target for which it would not be appropriate to set the
alignment of those sections.

Nick, as you know, please be extremely careful about any patches to
any files which are not specific to the C4x.

     * Why did you make the code to handle s->fix_line at line 745 of
       bfd/coffgen.c conditional only on RS6000COFF_C ?

Any such change would be incorrect, as coffgen.c is compiled for all
COFF targets.  It is incorrect to test a target specific macro like
RS6000COFF_C in that file.

It so happens that s->fix_line will only be true when working with an
RS/6000 COFF file.  However, it is incorrect to add a #ifdef test to
coffgen.c.  Not only is it the wrong approach, it won't even work
correctly--it will break RS/6000 COFF support.

     * This patch (to gas/config/obj-coff.c) would not apply. I could not
       locate the correct place to apply this patch, so I left it out.

This code is in c_symbol_merge.  I believe it is trying to fix a bug
which was fixed by this change:

Mon Jul 13 13:55:42 1998  Ian Lance Taylor  <ian@cygnus.com>

	* config/obj-coff.c (c_symbol_merge): Correct number of bytes when
	copying aux information.



     * Why does your patch remove a test on flag_m68k_mri at lines 295
       and 509 of gas/expr.c ?

I believe any such change would be incorrect.

Ian

Re C4x patch

[Nick Clifton]

Hi Joel,

  I have now had a chance to look at the patch that you sent to me,
  and in general it is OK.  I cannot just accept that patch however as
  there were no Changelog entries to accompany it.  If you can supply
  these thenm I believe that most of the patch can be applied.

  There is one problem with the patch as a whole - it was produced
  using 'diff -c' rather than 'diff -p'.  The lack of a context for
  several of the patch hunks made applying it to the latest sources
  rather difficult.

  I do also have some specific comments on parts of the patch:

  * This patch (to bfd/coffcode.h) would not apply.  I could not
    locate this code anywhere, so I have left it out.

*** 1068,1078 ****
--- 1068,1080 ----
       that is also how gdb locates the section.
       We need to handle the .ctors and .dtors sections similarly, to
       avoid introducing null words in the tables.  */
+ #ifndef C4XMAGIC
    if (COFF_DEFAULT_SECTION_ALIGNMENT_POWER > 2
        && (strncmp (section->name, ".stab", 5) == 0
  	  || strcmp (section->name, ".ctors") == 0
  	  || strcmp (section->name, ".dtors") == 0))
      section->alignment_power = 2;
+ #endif
  
    /* Similarly, the .stabstr section must be aligned to 2**0 at most.  */
    if (COFF_DEFAULT_SECTION_ALIGNMENT_POWER > 0


  * The new function that you added to coffcode.h called
    adjust_bf_ef() lacks a comment describing what it does.  (ie what
    kind of adjustment is it making to bf end ef ?)

  * Why have you changed the values of MAXCHUNK and CHUNK in
    bfd/srec.c ?

  * Why did you make the code to handle s->fix_line at line 745 of
    bfd/coffgen.c conditional only on RS6000COFF_C ?

  * The patch to fix the dectection of signed overflow in bfd/reloc.c:
    _bfd_relocate_contents() could not be applied because this code
    has been reworked.  I have assumed that no patch is now necessary.

  * The patch to add detection of c4x to config.sub was redundant
    because this name is already recognised by that script.

  * This patch (to gas/config/obj-coff.c) would not apply. I could not
    locate the correct place to apply this patch, so I left it out.

*** 279,290 ****
        /* @@ How many fields do we want to preserve?  Would it make more
  	 sense to pick and choose those we want to copy?  Should look
  	 into this further....  [raeburn:19920512.2209EST]  */
        alent *linenos;
!       linenos = coffsymbol (normal->bsym)->lineno;
!       memcpy ((char *) &coffsymbol (normal->bsym)->native,
! 	      (char *) &coffsymbol (debug->bsym)->native,
! 	      S_GET_NUMBER_AUXILIARY(debug) * AUXESZ);
        coffsymbol (normal->bsym)->lineno = linenos;
      }
  
    /* Move the debug flags. */
--- 279,296 ----
        /* @@ How many fields do we want to preserve?  Would it make more
  	 sense to pick and choose those we want to copy?  Should look
  	 into this further....  [raeburn:19920512.2209EST]  */
+       combined_entry_type *native;
        alent *linenos;
!       boolean done_lineno;
!       
!       native = coffsymbol (debug->bsym)->native;
!       coffsymbol (normal->bsym)->native = native;
!       
!       linenos = coffsymbol (debug->bsym)->lineno;
        coffsymbol (normal->bsym)->lineno = linenos;
+       
+       done_lineno = coffsymbol (debug->bsym)->done_lineno;
+       coffsymbol (normal->bsym)->done_lineno = done_lineno;
      }
  
    /* Move the debug flags. */

  * Why does your patch remove a test on flag_m68k_mri at lines 295
    and 509 of gas/expr.c ?

  * Why did you add a call to ldfile_set_output_arch() in ld/ldfile.c:
    ldfile_add_arch() ?

  * I have not applied the patches to ld/ldlang.c and ld/ldwrite.c
    since they unconditionally change the value of size.  These
    changes ought to be conditional upon targeting a c4x system.  (And
    they should be conditional at run time, not compile timne, since
    ld may be targeted at multiple hosts).

Anyway Joel - in a seperate email I will send you a gzipped cvs diff
of your patches, applied to the current sourceware sources and with a
few minor bug fixes, compiler warning removals and structure updates
included as well.  If anyone else wants a copy of this file, please
let me know.

Cheers
	Nick