Re: [PATCH 2 of 2] Rework binutils in order to provide soon binutils alternative

["Yann E. MORIN" <yann dot morin dot 1998 at free dot fr>]

Yann, All,

On Monday 19 November 2012 Yann Diorcet wrote:
> # HG changeset patch
> # User Yann Diorcet (diorcet.yann@gmail.com)
> # Date 1353320491 -3600
> # Node ID d3d1d51f399e6d2c1163f2f3ace2e3cbc73b2324
> # Parent  65c8bf534d0647ce52cdb319b52dab2f81da5017
> Rework binutils in order to provide soon binutils alternative
> 
> config: now the binutils is choosen by a menu
> script: elf2flt script is merged in binutils.sh
> 
> Signed-off-by: Yann Diorcet <diorcet.yann@gmail.com>

Here are my comments on that patch. I think it will be easier to follow
than what I said on IRC (which was quite hard to follow because of the
messages being inter-spersed).

Aside a few space-damage, here are my comments:

> diff -r 65c8bf534d06 -r d3d1d51f399e config/binutils.in
> --- a/config/binutils.in	Mon Nov 19 11:19:54 2012 +0100
> +++ b/config/binutils.in	Mon Nov 19 11:21:31 2012 +0100
> @@ -21,11 +21,12 @@
>  
>  config ARCH_BINFMT_FLAT
>      bool
> +    depends on ! ARCH_USE_MMU

I like this change, but you're already in a "if ! MMU ... endif" block.
So this "depends on" is redundant. I'll fix here.

>      prompt "Flat"
>      help
>        This will build flat binaries, suitable for
>        MMU-less architectures.
> -
> +      

Space-damage.

>  config ARCH_BINFMT_FDPIC
>      bool
>      prompt "FD_PIC ELF"
> @@ -33,12 +34,14 @@
>        This will build FD_PIC ELF binaries, suitable for
>        MMU-less architectures that still require to use
>        shared libraries (FIXME).
> -

Space-damage.

>  endif # ! ARCH_USE_MMU
>  
>  endchoice
[--SNIP--]
> diff -r 65c8bf534d06 -r d3d1d51f399e config/binutils/binutils.in
> --- a/config/binutils/binutils.in	Mon Nov 19 11:19:54 2012 +0100
> +++ b/config/binutils/binutils.in	Mon Nov 19 11:21:31 2012 +0100
> @@ -117,6 +117,8 @@
>  
>  config BINUTILS_GOLD_SUPPORTS_ARCH
>      bool
> +    default y if ARCH_arm
> +    default y if ARCH_x86
>  
>  config BINUTILS_HAS_PLUGINS
>      bool
> @@ -124,13 +126,6 @@
>  config BINUTILS_HAS_PKGVERSION_BUGURL
>      bool
>  
> -# Only these architectures have support in gold
> -config ARCH_arm
> -    select BINUTILS_GOLD_SUPPORTS_ARCH
> -
> -config ARCH_x86
> -    select BINUTILS_GOLD_SUPPORTS_ARCH
> -

Hmmm... When I wrote that code, I had in mind that it was the duty of
architectures to state if they supported binutils/gold or not.

Your change implies that it is binutils/gold that nows if it supports a
specific architecture. I think it makes much more sense this way. But
this should have been a separate patch.

Remember: one semantically autonomous change per patch. Here you're
mixing (at least) two semantic changes, and a cleanup change:
  - cleanup the ELF/FLAT/FDPIC choice dependencies on MMU/BARE_METAL
  - switch the way gold is available
  - introduce the multi-binutils infrastructure

Those three are unrelated, hence should be in different patches. That's
Ok, I'll do the split here. Next time, please try to better split your
changes.

BTW, when I said on IRC that you should submit those changes, I should have
been more explicit: I meant that these changes are an important foundation
of your Darwin port, but are unrelated to it, so you should get rid of
those changes as soon as possible by getting them upstreamed. What I was
not explicit enough about is that those changes should be reworked first.
Hence, I'll do the fixup here.

> diff -r 65c8bf534d06 -r d3d1d51f399e config/binutils/binutils.in.2
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/config/binutils/binutils.in.2     Mon Nov 19 11:21:31 2012 +0100

Hmmm... Strange that Mercurial did not catch the file rename. The old
elf2flt.in is still around. Did you use 'hg mv' to do the rename, or
did you do it manually?

I'll fix here.

> diff -r 65c8bf534d06 -r d3d1d51f399e scripts/build/binutils/binutils.sh
> --- a/scripts/build/binutils/binutils.sh	Mon Nov 19 11:19:54 2012 +0100
> +++ b/scripts/build/binutils/binutils.sh	Mon Nov 19 11:21:31 2012 +0100
[--SNIP--]
> @@ -41,10 +54,21 @@
>      binutils_opts+=( "prefix=${CT_BUILDTOOLS_PREFIX_DIR}" )
>      binutils_opts+=( "cflags=${CT_CFLAGS_FOR_BUILD}" )
>      binutils_opts+=( "ldflags=${CT_LDFLAGS_FOR_BUILD}" )
> +    binutils_opts+=( "binutils_bld=${CT_BUILD_DIR}/build-binutils-build-${CT_HOST}" )
> +    binutils_opts+=( "binutils_src=${CT_SRC_DIR}/binutils-${CT_BINUTILS_VERSION}" )

The last two should be in the "if FLAT ... fi" block: ...

>      do_binutils_backend "${binutils_opts[@]}"
>  
>      CT_Popd
> +    
> +    if [ -n "${CT_ARCH_BINFMT_FLAT}" ]; then
> +        CT_mkdir_pushd "${CT_BUILD_DIR}/build-elf2flt-build-${CT_BUILD}"

... here.

> +        do_elf2flt_backend "${binutils_opts[@]}"
> +         
> +        CT_Popd
> +    fi
> +    
>      CT_EndStep
>  }
>  
> @@ -52,7 +76,7 @@
>  do_binutils_for_host() {
>      local -a binutils_tools
>      local -a binutils_opts
> -
> +    

Space-damage.

>      CT_DoStep INFO "Installing binutils for host"
>      CT_mkdir_pushd "${CT_BUILD_DIR}/build-binutils-host-${CT_HOST}"
>  
> @@ -62,9 +86,20 @@
>      binutils_opts+=( "cflags=${CT_CFLAGS_FOR_HOST}" )
>      binutils_opts+=( "ldflags=${CT_LDFLAGS_FOR_HOST}" )
>      binutils_opts+=( "build_manuals=${CT_BUILD_MANUALS}" )
> +    binutils_opts+=( "binutils_bld=${CT_BUILD_DIR}/build-binutils-host-${CT_HOST}" )

You forgot binutils_src
Ditto, should be in the "if FLAT ... fi" block, below...

>      do_binutils_backend "${binutils_opts[@]}"
>  
> +    CT_Popd
> +
> +    if [ -n "${CT_ARCH_BINFMT_FLAT}" ]; then
> +        CT_mkdir_pushd "${CT_BUILD_DIR}/build-elf2flt-host-${CT_HOST}"

... here.

> +        do_elf2flt_backend "${binutils_opts[@]}"
> +        
> +        CT_Popd
> +    fi
> +    
>      # Make those new tools available to the core C compilers to come.
>      # Note: some components want the ${TARGET}-{ar,as,ld,strip} commands as
>      # well. Create that.
> @@ -73,6 +108,9 @@
>      case "${CT_TOOLCHAIN_TYPE}" in
>          cross|native)
>              binutils_tools=( ar as ld strip )
> +            if [ -n "${CT_ARCH_BINFMT_FLAT}" ]; then
> +                binutils_tools+=( elf2flt flthdr )
> +            fi

Exactly! :-)

>              case "${CT_BINUTILS_LINKERS_LIST}" in
>                  ld)         binutils_tools+=( ld.bfd ) ;;
>                  gold)       binutils_tools+=( ld.gold ) ;;
> @@ -218,6 +255,50 @@
>      fi
>  }
>  
> +# Build elf2flt for X -> target
> +#     Parameter     : description               : type      : default
> +#     host          : machine to run on         : tuple     : (none)
> +#     prefix        : prefix to install into    : dir       : (none)
> +#     static_build  : build statcially          : bool      : no
> +#     cflags        : cflags to use             : string    : (empty)
> +#     ldflags       : ldflags to use            : string    : (empty)

Also, binutils_src and binutils_bld.

[--SNIP--]

All in all, that's OK.
I'll do the split + misc fixes here. No need to resend.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

--
For unsubscribe information see http://sourceware.org/lists.html#faq