This is the mail archive of the binutils@sourceware.cygnus.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: Binutils Bug on i960 file format


Hi Su,

Thanks very much for submitting this patch.

: Changelog entry
: 	*cpu-i960.c(scan_960_mach) :add some code to recongnize the i960 machine architecture	describing as "80960KB". 

I have a couple of comments on patch submission in general that helps
me, and other readers of this list, understand patches that are submitted:

  * Please format your ChangeLog entry(s) in the same way that other
    Changelog entries are formatted.  In particular the entry that you
    supplied did not include your name and email address, was not
    wrapped to 80 columns, and was not written as a normal,
    descriptive English sentance.  I would suggest reformating your
    entry like this: 

2000-05-22  Su Wenming  <careri@public.sta.net.cn>

	* cpu-i960.c (scan_960_mach): Add code to recongnize the i960
	machine architecture described as "80960KB".

  * If possible, please include a patch as plain ASCII, rather than as
    encoed attachment.  This helps old fogies like me who do not have
    an attachment enabled email reader.  (I have to save your email to
    a temporary, switch to a shell, run metamail on the file, save the
    decoded patch, then switch back to my editor and load up the
    decoded file, all just to be able to examine the patch.  If it was
    in plain text, along with the rest of your email, it would be much
    easier for me to read).  Of course if the patch is very long, then
    it makes sense to compress and encode it.  But for short patches,
    plain text is preferable.

  * When generating a patch please use 'diff -p' to generate a context
    patch, and please try to have the unpatched file as the "old" or
    first file and the patched file as the "new" or second file.  The
    patch you generated had the two files swapped, so it looked like
    you were actually removing your new code from the cpu-i960.c file:

37,41d36
<   int i;
< 
< 
<   for( i = 0; i < strlen(string); i ++ )
< 	string[i] = tolower( string[i] );
44c39
<   /*in some bfd the cpu-id is written as "80960KB"*/
---
> 

  * In order for me to be able to accept a patch it is necessary for
    you to have a copyright assignment on file with the FSF.  It helps
    me if you can mention whether you do have such an assignment on
    file, when you submit a patch.


Finally, on a technical note, your patch was not quite complete, in
that it would recognise some illegal names, such as "80960core".  I
propose the following alternative patch which I belive should do the
right thing, and which also takes the opportunity to tidy up the code
formating in the scan_960_match() function.  Please let me know if you
find this version acceptable.

Cheers
	Nick


Index: bfd/cpu-i960.c
===================================================================
RCS file: /cvs/src//src/bfd/cpu-i960.c,v
retrieving revision 1.1.1.1
diff -p -r1.1.1.1 cpu-i960.c
*** cpu-i960.c	1999/05/03 07:28:55	1.1.1.1
--- cpu-i960.c	2000/05/22 17:51:21
*************** scan_960_mach (ap, string)
*** 34,61 ****
       const char *string;
  {
    unsigned long machine;
  
!   /* Look for the string i960, or somesuch at the front of the string  */
  
!   if (strncmp("i960",string,4) == 0) {
!     string+=4;
!   }
!   else {
!     /* no match, can be us */
!     return false;
!   }
!   if (string[0] == 0) {
!     /* i960 on it's own means core to us*/
!     if (ap->mach == bfd_mach_i960_core) return true;
!     return false;
!   }
  
!   if (string[0] != ':') {
      return false;
!   }
!   string++;
    if (string[0] == '\0')
      return false;
    if (string[0] == 'c' && string[1] == 'o' && string[2] == 'r' &&
        string[3] == 'e' && string[4] == '\0')
      machine = bfd_mach_i960_core;
--- 34,69 ----
       const char *string;
  {
    unsigned long machine;
+   int i;
  
!   for (i = 0; i < strlen (string); i ++)
!     string[i] = tolower (string[i]);
  
!   /* Look for the string i960 at the front of the string.  */
!   if (strncmp ("i960", string, 4) == 0)
!     {
!       string += 4;
  
!       /* i960 on it's own means core to us.  */
!       if (string[0] == 0)
! 	return ap->mach == bfd_mach_i960_core;
!       
!       /* "i960:*" is valid, anything else is not.  */
!       if (string[0] != ':')
! 	return false;
! 
!       string ++;
!     }
!   /* In some bfds the cpu-id is written as "80960KB".  */
!   else if (strcmp ("80960kb", string) == 0)
!     return ap->mach == bfd_mach_i960_kb_sb;
!   /* No match, can't be us.  */
!   else
      return false;
!   
    if (string[0] == '\0')
      return false;
+   
    if (string[0] == 'c' && string[1] == 'o' && string[2] == 'r' &&
        string[3] == 'e' && string[4] == '\0')
      machine = bfd_mach_i960_core;
*************** scan_960_mach (ap, string)
*** 63,69 ****
      machine = bfd_mach_i960_ka_sa;
    else if (strcmp (string, "kb_sb") == 0)
      machine = bfd_mach_i960_kb_sb;
!   else if (string[1] == '\0' || string[2] != '\0') /* rest are 2-char */
      return false;
    else if (string[0] == 'k' && string[1] == 'b')
      machine = bfd_mach_i960_kb_sb;
--- 71,77 ----
      machine = bfd_mach_i960_ka_sa;
    else if (strcmp (string, "kb_sb") == 0)
      machine = bfd_mach_i960_kb_sb;
!   else if (string[1] == '\0' || string[2] != '\0') /* rest are 2-char.  */
      return false;
    else if (string[0] == 'k' && string[1] == 'b')
      machine = bfd_mach_i960_kb_sb;
*************** scan_960_mach (ap, string)
*** 85,91 ****
      machine = bfd_mach_i960_hx;
    else
      return false;
!   if (machine == ap->mach)   return true;
    return false;
  }
  
--- 93,102 ----
      machine = bfd_mach_i960_hx;
    else
      return false;
!   
!   if (machine == ap->mach)
!     return true;
!   
    return false;
  }
  

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]