RE: [PATCH] Addition of new option '--rlib' in linker for Renesas SH targets

["Gina Verlekar" <GinaV at KPITCummins dot com>]

Hi Nick,

I have modified the patch as per your comments and suggestions. Please
find	
attached herewith the modified patch.

Presently I am facing some problem in adding a test case in binutils for

testing the utility. However, I have attached a separate test case for

convrenesaslib utility along with this mail, for your reference.

I will soon be adding the testcase in binutils also.


>> * The --remove-underscore switch is a bad idea.  It would be much

>> cleaner and safer if you just invoke "objcopy

>> --remove-leading-char" for yourself as a separate step after

>> completing the conversion to the GNU ar format.

Whenever a user wants to use custom Renesas library file with a project
compiled
using sh*-linux toolchain, the leading underscores of all functions and
global	
symbols in the Renesas library file have to be removed.

If this is not done, the user will get 'undefined references' errors for
all the
functions and global symbols in the library file.

Hence a '--remove-underscore' switch is provided in the convrenesaslib
utility	
for ease of use.	

Changelog

2006-07-22  Gina G. Verlekar  <ginav@kpitcummins.com>
	* convrenesaslib.c: Add new file
	* Makefile.am: Modify to build convrenesaslib.c.
	* configure.in: Check for Renesas SH targets to build
convrenesaslib.
	* doc/binutils.texi: Add documentation for convrenesaslib
utility.

Any comments will be appreciated.

Regards,
Gina Verlekar
KPIT Cummins InfoSystems Ltd.
Pune, India
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~	
Free download of GNU based tool-chains for Renesas SH, H8, R8C, M16C and

M32C Series. The following site also offers free technical support to
its	
users.	
Visit http://www.kpitgnutools.com for details.
Latest versions of KPIT GNU tools were released on June 1, 2006.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~




-----Original Message-----
From: Nick Clifton [mailto:nickc@redhat.com] 
Sent: Wednesday, July 12, 2006 11:39 PM
To: Gina Verlekar
Cc: binutils@sourceware.org
Subject: Re: [PATCH] Addition of new option '--rlib' in linker for
Renesas SH	
targets

Hi Gina,

> Please refer to the link below for the patch I had submitted:

> http://sources.redhat.com/ml/binutils/2006-07/msg00068.html


Sorry for the delay in reviewing this patch.


Unfortunately I do not think that it is ready for inclusion into the

sources yet.  The problems that I have with it are:


  * The changes to Makefile.am mean that recreating Makefile.in

    results a warning message about the convrenesaslib_SOURCES being

    defined but not used.  You need to add it to the EXTRA_PROGRAMS

    definition.

    
  * The copyright comment at the start of convrenesaslib.c has the

    wrong address for the FSF and it says that the tool is part of GLD

    whereas in fact it is part of the GNU Binutils.


  * REPORT_BUGS_TO is defined in the bin-bugs.h header.


  * Having the "magic_num" field of the rlib_header structure be of

    the "unsigned char" type results in compile time warning messages

    when it is used as an argument to strcpy().

    
  * None of the textual output of the program has been

    internationalized.

    
  * The --remove-underscore switch is a bad idea.  It would be much

    cleaner and safer if you just invoke "objcopy

    --remove-leading-char" for yourself as a separate step after

    completing the conversion to the GNU ar format.

    
  * The indentation and formatting do not quite match up to the GNU

    Coding Standard.

    
  * The use of the program_name array looks very strange to me.  I

    think that it would be much cleaner if you left program_name as

    being the value of argv[0] and you used a different variable when

    you are constructing the ar command line.


  * There are various "magic" constants in some of the malloc() calls

    (eg + 50, or + 100) without any explanation of where they come

    from.  There are also several fixed length arrays that should not

    be fixed (eg filepath, module_name).


  * The fatal() function should be used in place of the

    printf();exit(1); combination currently used.


  * [optional] Adding a --verbose switch would be useful.  It could

    tell the user the command line it is using to invoke "ar", and

    also mention when files are being created and deleted.


  * [optional] A test case to check that the tool works would be very

    helpful.


Cheers
  Nick

Attachment: testcase.tar.gz
Description: testcase.tar.gz

Attachment: patch.tar.gz
Description: patch.tar.gz