This is the mail archive of the crossgcc@sourceware.org mailing list for the crossgcc project.

See the CrossGCC FAQ for lots more information.


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: simple multilib option


Konrad, All,

On Friday 11 November 2011 16:38:17 Konrad Eisele wrote:
> Appended is a patch that adds simple glibc multilib capability.

Hehe! Very nice!

>  scripts/build/libc/glibc-eglibc.sh-common
>   main part: do_libc_backend_multilib():
>    calls "do_libc_backend" multilib-times with 2 options
>    extra_dir="/${dir}" extra_flags="${flags}"
>    at the end do_libc_backend_rearrange is called to rearange
>    the sysroot directories.
>  config/cc/gcc.in.2
>    Add CC_MULTILIB option
>  patches/gcc/4.6.0/200-sparc-leon.patch
>    Multilibbed sparc-leon compile. The multilibbed gcc is
>    compiled with Vendor set to "leon3"
>  patches/glibc/2.14/400-sparc-leon.patch
>    Some fixes to get glibc-2.14 compiled with soft-float
>  patches/glibc/2.14/401-s_fma_f.patch
>    Some fixes to get glibc-2.14 compiled with soft-float
>  scripts/build/binutils/binutils.sh
>    add multilib
>  scripts/build/cc/gcc.sh
>    add multilib

Next time, could you send in the body rather in attachement, it's easier to
review and comment...

First, the patch addresses different unrelated changes. For example, the
Leon changes should go in a separate patch.

Then, the multi-lib change is huge. I believe it could be split in a few,
smaller incremental patches.

Globally, avoid lines longer than 80 chars. I know there already are some
(very) long lines in the code, but try not to add more...


> diff --git a/build/lib/ct-ng-/scripts/build/libc/glibc-eglibc.sh-common b/build/lib/ct-ng-/scripts/build/libc/glibc-eglibc.sh-common
> index 2af3a9a..3218656 100644
> --- a/scripts/build/libc/glibc-eglibc.sh-common
> +++ b/scripts/build/libc/glibc-eglibc.sh-common
> @@ -57,18 +57,99 @@ do_libc_extract() {
> 
>  do_libc_start_files() {
>      # Start files and Headers should be configured the same way as the
>      # final libc, but built and installed differently.
> -    do_libc_backend libc_mode=startfiles
> +    do_libc_backend_multilib libc_mode=startfiles
> 
>  }
>  
>  # This function builds and install the full C library
>  do_libc() {
> -    do_libc_backend libc_mode=final
> +    do_libc_backend_multilib libc_mode=final
>  }
> 
> +# installation is done into ${CT_SYSROOT_DIR}/${dir}. The multilib prefix has to be included
> +# to reflect the runtime setting.
> +do_libc_backend_rearrange() {
> +

No empty line at start of function.

> +    local libc_mode=final

No default.
The 'libc_mode' is not optional, so every caller has to pass it.

But are you sure that it makes sense to pass the libc_mode when
calling rearrange?

> +    while [ $# -ne 0 ]; do
> +        eval "${1// /\\ }"
> +        shift
> +    done
> +
> +    if [ "${libc_mode}" = "final" ]; then
> +        cross_cc=$(CT_Which "${CT_TARGET}-gcc")
> +        for i in `${cross_cc} --print-multi-lib 2>/dev/null`; do
> +            dir=`echo $i | sed -e 's/;.*$//'`;

dir="${i%%;*}"
Also, no trailing semi-column, it's useless in shell scripts
( this is not C! ;-) )

> +           bdir=`echo ${dir} | sed -e 's/\//\\\\\//g'`

Why do you need to double-escape the '/' ?
And for readability:
  sed -e 's:/:\\\\/:g'

Also, use ${sed} for portability reasons (eg. on MacOS-X, this is gsed,
not sed, and this is detected by ./configure).

And then, use the '-r' option to sed:
  ${sed} -r -e 's:/:\\\\/:g'

> +           if [ "${dir}" = "." ]; then
> +               true;
> +           else

if [ "${dir}" != "." ]; then

> +               flags=`echo $i | sed -e 's/^[^;]*;//' -e 's/@/ -/g'`;
> +               CT_DoStep INFO "Fixing up multilib location ${CT_SYSROOT_DIR}/${dir}/lib to ${CT_SYSROOT_DIR}/lib/${dir}"

It does not belong to the 'INFO' log-level, but rather to 'EXTRA' or even
better yet, to 'DEBUG'.

> +               mkdir -p ${CT_SYSROOT_DIR}/lib/${dir}
> +               mkdir -p ${CT_SYSROOT_DIR}/usr/lib/${dir}
> +
> +               # recreate the symbolic links for multilib
> +               find ${CT_SYSROOT_DIR}/usr/lib/${dir}/ -type l -maxdepth 1 > ${CT_SYSROOT_DIR}/usr/lib/${dir}/links
> +               for f in `cat ${CT_SYSROOT_DIR}/usr/lib/${dir}/links`; do

You could use an array variable, if you really want two lines:
    links=( $(find blabla       \
                   -type l      \
                   -maxdepth 1  ) )
    for f in "${links[@]}"; do

But you could also use a single call:
    for f in $(find blabla      \
                   -type l      \
                   -maxdepth 1  )
    do
        [...]
    done

> +                   fn=`basename $f`
> +                   ln=`readlink $f`
> +                   ln=`basename $ln`
> +                   pre=`echo ${dir} | awk '{r="";c=split($0,b,"/");for(i=0;i<c;i++){if(i!=0){r=r "/";}r=r "..";}; printf("%s",r); }'`

Explain what you're doing here. Basically, you want to turn
    foo/bar
into
    ../..

Also, try to get a shorter line, and use $(...) as everywhere else:
    pre=$( echo ${dir}  \
           |awk '{ r="";
                   c=split($0, b, "/");
                   for( i=0; i<c; i++ ) {
                       if( i!=0 ) {
                           r=r "/";
                       }
                       r = r "..";
                   }
                   printf("%s",r);
                 }' )

> +                   (cd ${CT_SYSROOT_DIR}/usr/lib/${dir}/; ln -sf ../../${pre}/lib/${dir}/$ln $fn)

I'm not fan of having a sub-shell here.
    CT_Pushd "blabla"
    ln -sf blabla blabla
    CT_Popd

> +               done
> +
> +                # rewrite the library multiplexers
> +               for l in libc libpthread libgcc_s; do
> +                   for d in lib/${dir} usr/lib/${dir}; do
> +                       if [ -f "${CT_SYSROOT_DIR}/${d}/${l}.so" -a ! -L ${CT_SYSROOT_DIR}/${d}/${l}.so ]; then
> +                           if [ ! -f "${CT_SYSROOT_DIR}/${d}/${l}.so_ori_i" ]; then

What if the the _ori_i file already exist?
Surely, it should not happen, and if it does, we should consider this
as an error.

> +                               cp ${CT_SYSROOT_DIR}/${d}/${l}.so ${CT_SYSROOT_DIR}/${d}/${l}.so_ori_i
> +                               cat ${CT_SYSROOT_DIR}/${d}/${l}.so_ori_i | sed -e "s/\/lib\/$l/\/lib\/$bdir\/$l/g" > ${CT_SYSROOT_DIR}/${d}/${l}.so
> +                           fi

Transform that whole if..fi into:
    ${sed} -r -i        \
           -e "blabla"  \
           foo.so

> +                       fi
> +                   done
> +               done
> +               CT_EndStep
> +           fi
> +        done;
> +    fi
> +}
> +
> +# call do_libc_backend <multilib> times with varying <extra_dir> and <extra_flags> options
> +do_libc_backend_multilib() {
> +    local flags=""; local i; local dir;
> +    cross_cc=$(CT_Which "${CT_TARGET}-gcc")
> +    if [ "${CT_CC_MULTILIB}" = "y" ]; then
> +       CT_DoStep INFO "C Library with multilib"
> +       CT_DoStep INFO "C Library with multilib. Flags: \"\" Dir: \"\""

Redundant log line.
Missing flags and dir.

> +       # first create <non-multilib> to create ${CT_HEADERS_DIR} dir
> +       do_libc_backend "$@"
> +       CT_EndStep
> +       for i in `${cross_cc} --print-multi-lib 2>/dev/null`; do
> +           dir=`echo $i | sed -e 's/;.*$//'`;

dir="${i%%;*}"

> +           if [ "${dir}" = "." ]; then
> +               true;
> +           else

if [ "${dir}" != "." ]; then

> +               flags=`echo $i | sed -e 's/^[^;]*;//' -e 's/@/ -/g'`;
> +               CT_DoStep INFO "C Library with multilib. Flags: \"${flags}\" Dir: \"${dir}\""
> +               do_libc_backend "$@" extra_dir="/${dir}" extra_flags="${flags}"
> +               CT_EndStep
> +           fi
> +       done;
> +       do_libc_backend_rearrange

Missing the libc_mode.

> +       CT_EndStep
> +    else
> +       do_libc_backend "$@"
> +    fi
> +}

This will install headers multiple times, right? Is this expected/wanted?

Also, I'd do something like:

    do_libc_backend_multilib() {
        # We need to call the backend at least once,
        # with all settings set to the defaults
        do_libc_backend "$@"
        if [ "${CT_CC_MULTILIB}" = "y" ]; then
            for i in $( "${cross_cc}" --print-multi-lib 2>/dev/null ); do
                dir="${i%%;*}"
                if [ "${dir}" != "." ]; then
                    flags="${i#*;}"; flags="${flags//@/ -}"
                    CT_DoStep INFO "C Library with multilib. Flags: '${flags}' Dir: '${dir}'"
                    do_libc_backend "$@" extra_dir="/${dir}" extra_flags="${flags}"
                    CT_EndStep
                fi
            done
        fi
    }

>  do_libc_backend() {
>      local src_dir="${CT_SRC_DIR}/${CT_LIBC}-${CT_LIBC_VERSION}"
>      local libc_mode=final
>      local extra_cc_args
> +    local extra_dir=""; local extra_dir_p;

On two lines, no trailing semi-column.

> +    local extra_flags=""
>      local -a extra_config
>      local -a extra_make_args
>      local glibc_cflags
> 
> @@ -77,15 +158,16 @@ do_libc_backend() {
>          eval "${1// /\\ }"
>          shift
>      done
> -
> +    extra_dir_p=`echo ${extra_dir} | sed -e 's/\///g'`

Use $(...) instead of `...`, as everywhere else in the code.
Also, do not forget to quote strings:

    extra_dir_p="$( echo "${extra_dir}" | sed -e 's/\///g' )"

> +
>      if [ "${libc_mode}" = "startfiles" ]; then
>          CT_DoStep INFO "Installing C library headers & start files"
> -        mkdir -p "${CT_BUILD_DIR}/build-libc-start-files"
> -        cd "${CT_BUILD_DIR}/build-libc-start-files"
> +        mkdir -p "${CT_BUILD_DIR}/build-libc-start-files${extra_dir_p}"
> +        cd "${CT_BUILD_DIR}/build-libc-start-files${extra_dir_p}"
>      else # libc_mode = final
>          CT_DoStep INFO "Installing C library"
> -        mkdir -p "${CT_BUILD_DIR}/build-libc"
> -        cd "${CT_BUILD_DIR}/build-libc"
> +        mkdir -p "${CT_BUILD_DIR}/build-libc${extra_dir_p}"
> +        cd "${CT_BUILD_DIR}/build-libc${extra_dir_p}"
>      fi

I would prefer that there is only one 'top-level' build dir. Also, we
could replace this whole fi..else.fi block with a single construct.
Something like:

    mkdir -p "${CT_BUILD_DIR}/build-libc-${libc_mode}/${extra_dir_p:-base}"
    cd "${CT_BUILD_DIR}/build-libc-${libc_mode}/${extra_dir_p:-base}"

>      CT_DoLog EXTRA "Configuring C library"
> @@ -133,7 +215,13 @@ do_libc_backend() {
>      esac
>      
>      case "${CT_ARCH_FLOAT_HW},${CT_ARCH_FLOAT_SW}" in
> -        y,) extra_config+=("--with-fp");;
> +        y,) # if it is a <multilib> build then check if -msoft-float is given
> +           if [ "x`expr "${extra_flags}" : '.*-msoft-float.*'`" != "x0" ]; then
> +               extra_config+=("--with-fp=no");
> +           else
> +               extra_config+=("--with-fp");
> +           fi
> +           ;;
>          ,y) extra_config+=("--without-fp");;
>      esac

The hunk above no longer applies to the tree. The soft/hard float selection
now uses a string, because there might be a bunch of possibilities, not only
'soft' and 'hard', but also 'softfp' now.

[--SNIP--]
> @@ -288,9 +376,13 @@ do_libc_backend() {
>                                             -nostartfiles    \
>                                             -shared          \
>                                             -x c /dev/null   \
> -                                           -o "${CT_SYSROOT_DIR}/usr/lib/libc.so"
> +                                           -o "${CT_SYSROOT_DIR}${extra_dir}/usr/lib/libc.so"
>          fi # threads == nptl
>      else # libc_mode = final
> +        CT_DoLog EXTRA "Prepare C library"
> +        CT_DoExecLog ALL make ${JOBSFLAGS}                      \
> +                              "${extra_make_args[@]}"           \
> +                              clean

Why do you need to clean here?
The build dir should be brand new, as we just mkdir it a few lines above.
In any way, do not reuse any existing dir.

>          CT_DoLog EXTRA "Building C library"
>          CT_DoExecLog ALL make ${JOBSFLAGS}                      \
>                                "${extra_make_args[@]}"           \

[--SNIP--]
> diff --git a/config/cc/gcc.in.2 b/config/cc/gcc.in.2
> index 35a1070..8fe3a2e 100644
> --- a/config/cc/gcc.in.2
> +++ b/config/cc/gcc.in.2
> @@ -1,4 +1,10 @@
>  # gcc configuration options
> +config CC_MULTILIB
> +    bool
> +    prompt "enable multilib"
> +    default n
> +    help
> +      Enable multilib build of libc

We need to find a place where to put that option, but I think the gcc
sub-menu is not the proper place.

Multi-lib is a property of the toolchain, that is implemented by many
components, and gcc is only one of those. binutils also plays a role in
multilib.

So, it makes more sense to put the option in a more global sub-menu, such
as in the toolchain options sub-menu.

>  config CC_ENABLE_CXX_FLAGS
>      string
> @@ -11,6 +17,7 @@ config CC_ENABLE_CXX_FLAGS
>        Note: just pass in the option _value_, that is only the part that
>        goes after the '=' sign.
> 
> +

Spurious empty line.

>  config CC_CORE_EXTRA_CONFIG_ARRAY
>      string
>      prompt "Core gcc extra config"

[--SNIP leon patches--]

> diff --git a/scripts/build/binutils/binutils.sh b/scripts/build/binutils/binutils.sh
> index e082590..7969b13 100644
> --- a/scripts/build/binutils/binutils.sh
> +++ b/scripts/build/binutils/binutils.sh
[--SNIP--]
> @@ -151,7 +156,13 @@ do_binutils_target() {
>              extra_config+=("--with-pkgversion=${CT_PKGVERSION}")
>              [ -n "${CT_TOOLCHAIN_BUGURL}" ] && extra_config+=("--with-bugurl=${CT_TOOLCHAIN_BUGURL}")
>          fi
> -
> +

Spurious empty-line change: space damage.

> +       if [ "${CT_CC_MULTILIB}" = "n" ]; then
> +            extra_config+=("--disable-multilib")
> +       else
> +            extra_config+=("--enable-multilib")
> +       fi
> +
>          CT_DoExecLog CFG                                            \
>          "${CT_SRC_DIR}/binutils-${CT_BINUTILS_VERSION}/configure"   \
>              --build=${CT_BUILD}                                     \
> @@ -162,7 +173,6 @@ do_binutils_target() {
>              --enable-shared                                         \
>              --enable-static                                         \
>              --disable-nls                                           \
> -            --disable-multilib                                      \
>              "${extra_config[@]}"                                    \
>              ${CT_ARCH_WITH_FLOAT}                                   \
>              "${CT_BINUTILS_EXTRA_CONFIG[@]}"

Be careful with binutils for target. It only installs a pair of libraries,
not the whole binutils. So, is it necessary to enable multilib for the
target binutils?

[--SNIP gcc changes (OK)--]

Globally, I am very happy to see this patch coming in!
There are a bunch of issues, but that's expected with such core changes.

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


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