This is the mail archive of the binutils@sourceware.org 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]
Other format: [Raw text]

Re: PATCH: support for NEC SX architecture


hi!

On Wed, 2009-06-03 at 12:56 +0200, Tristan Gingold wrote:
> Hi,
> 
> this is a weird but interesting port :-)
weird by all means.

> You'd better to submit your patches inline (not as a tar.gz) as it is  
> easier to review.
well, the patches are quite big, so I though it would be nicer to pack
them - didnt really expect someone would review them in their email
client ... I can resubmit them inline, if necessary.

> Don't forget to capitalize the first letter and put a double space  
> after the dot.  There are still a few
> places where this GNU style is not followed, eg:
> 
> +#ifdef NEC_SX_COFF
> +  /* the optional header magic number and the stack size are provided  
> as
> +     commandline arguments of the linker. */
ok. wasn't aware of there was comment related stuff in the coding style
- just tried to mimic the existing one.

> It is strange to use bfd_vma type for f_nsyms in include/coff/internal.h
true. however, I needed a type with the same domain as the virtual
addresses of the target CPU: any suggestions?

> Why are you using SX_PACKED in declaration of external_filhdr,  
> external_aouthdr ... ?
just to make sure that the compiler does not introduce any padding as
these structures should correspond to the memory/file contents of an SX
COFF file, bit by bit. it's an sx target specific define that translates
to gcc packed attribute. I tend to abstract gccisms away with defines
whenever possible.

> Although I am ready to read your patch (when inlined), I can't approve  
> it.
I thought so, it was more a question for Nick or anybody else who can
say yes to some of the changes in the core coff code - it's those that
probably need review most, not the target specific code.

> BTW: in there a documentation for the cpu ?
sure there is. ;) it is unfortunately not (or, better, is not supposed
to be) publicly available. however, asking google about it, you just
might find something. ;)

thanks and regards,
  jaKa 

-- 
email: jaka@xlab.si
w3:    http://www.gmajna.net/svojat/jaka/


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