Re: New linker option --dsbt_index for C6x; c6x-uclinux fixes

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 29 Sep 2010, Bernd Schmidt wrote:

> Index: include/bfdlink.h
> ===================================================================
> RCS file: /cvs/src/src/include/bfdlink.h,v
> retrieving revision 1.81
> diff -c -p -r1.81 bfdlink.h
> *** include/bfdlink.h	15 Dec 2009 02:02:39 -0000	1.81
> --- include/bfdlink.h	29 Sep 2010 12:40:25 -0000
> *************** struct bfd_link_info
> *** 467,472 ****
> --- 467,475 ----
>   
>     /* List of symbols should be dynamic.  */
>     struct bfd_elf_dynamic_list *dynamic_list;
> + 
> +   /* For DSBT binaries, the DSBT index.  */
> +   int dsbt_index;

My inclination is that someone else should approve such a change to 
generic code.  I'll review the rest of the patch.

> + @kindex --dsbt_index
> + @item --dsbt_index @var{index}
> + This option sets the DSBT index of the current executable or shared library
> + to @var{index}.

I see no existing documentated "--" options containing "_".  The normal 
style would definitely be --dsbt-index.

> + GENERATE_SHLIB_SCRIPT=yes

I'd expect this addition to cause various tests to fail (that previously 
didn't run for these targets) because of other parts of shared library 
support not being in yet - at least, that's what I previously observed.  
Thus, I think it would be better to postpone adding this to the patch 
adding proper shared library support, making sure that with that patch in 
all the tests that are then run do pass for both c6x-elf and c6x-uclinux 
unless there are generic reasons for failures that are under discussion.  
Results for c6x-elf are currently clean; we should keep them that way.

> + PARSE_AND_LIST_ARGS_CASES='
> +     case OPTION_DSBT_INDEX:
> +       link_info.dsbt_index = atoi (optarg);
> +       break;

Using atoi is a bad idea as it has undefined behavior for invalid input or 
overflow.

There are several cases in emultempl/ of using strtoul, and checking that 
it did consume the whole argument, for such parsing.  Actually using 
strtol instead of strtoul is better here because it allows you to detect 
negative inputs reliably.  There should probably be a range check (the 
value needs to fit in an unsigned 15-bit range) here and testcases in the 
testsuite for out-of-range values (negative or too large) and completely 
invalid values.  (The range requirement in the ABI is "R < DSBT Size - 1" 
but you obviously can't make that check or add testcases for it until you 
also have the linker support for specifying the DSBT size.)

> Index: ld/testsuite/lib/ld-lib.exp
> ===================================================================
> RCS file: /cvs/src/src/ld/testsuite/lib/ld-lib.exp,v
> retrieving revision 1.74
> diff -c -p -r1.74 ld-lib.exp
> *** ld/testsuite/lib/ld-lib.exp	29 Sep 2010 06:06:00 -0000	1.74
> --- ld/testsuite/lib/ld-lib.exp	29 Sep 2010 12:40:27 -0000
> *************** proc is_elf_format {} {
> *** 409,414 ****
> --- 409,415 ----
>   	 && ![istarget *-*-linux*]
>   	 && ![istarget frv-*-uclinux*]
>   	 && ![istarget bfin-*-uclinux]
> + 	 && ![istarget c6x-*-uclinux]

The canonical form of the target, which is the one that DejaGnu will see 
here, is tic6x-*-uclinux.

Although I only thought of ld-lib.exp, Alan Modra's recent patch 
<http://sourceware.org/ml/binutils/2010-09/msg00514.html> points out that 
similar changes are needed in two other files as well.

-- 
Joseph S. Myers
joseph@codesourcery.com