Re: simple multilib option

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

Konrad, All,

On Monday 14 November 2011 11:30:54 Konrad Eisele wrote:
> I'll send revised patches as reply to this one, in the
> meantime here are the comment-comments:

OK, I saw your updated patch. I'll have a look tomorow, it's a bit late
for me here for now, and I could not get to review it this evening...
Sorry for the delay this implies... :-(

> I'll send the Multilib-patches only. I also skip the CC_MULTILIB
> option definition, as for you have to decide where to put it.

Sorry I was not explicit. My opinion is it should go into the toolchain
sub-menu. If you think of a better place, feel free to suggest it. :-)

> The gcc-4.6 and glibc sparcleon toolchain patch I'll send some other
> time in the future.

OK, good!

> To get the multilib patches in would be nice as for without them none
> of our toolchains can be built with crosstools.

Yes, I am very much interested in this patch! I just want to make it as
good as possible before it gets in. Once the code is in, it tends to be
more difficult to change it than before inclusion.

> >> diff --git a/build/lib/ct-ng-/scripts/build/libc/glibc-eglibc.sh-com
> >> +           bdir=`echo ${dir} | sed -e 's/\//\\\\\//g'`
> > 
> > Why do you need to double-escape the '/' ?
> > And for readability:
> >   sed -e 's:/:\\\\/:g'
> 
> I use $bdir in a sed expression below: "s/\/lib\/$l/\/lib\/$bdir\/$l/g",
> so I have to doublequote to get quotes.

Yes, I saw it afterwards, and forgot to remove my comment.
OK.

> >> +               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.
> 
> I emit a Warning. If _iri_i exist it will already have been patched.

But why would a file, that we _just_installed_, be already patched?

> >> +
> >>      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}"
> > 
> 
> I'm not shure how to do it. Please decide yourself and change...

OK, I'll do it here.

> >>      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.
> 
> Sorry, cant handle this. The scenario is modeled so that if multilib
> issues "-msoft-float" then glibc is given "--with-fp=no". In fact I'm not shure
> weather --with-fp=no is the same as --without-fp. In the
> glibc Makefile snippet I use "ifeq ($(with-fp),no)", maybe the
> configure is smart enough to define with-fp=no for "--without-fp"...

What I meant was:
 - instead of writing:
    case "${CT_ARCH_FLOAT_HW},${CT_ARCH_FLOAT_SW}" in
      y,) blabla;;
      ,y) blibli;;
    esac

 - we now write:
    case "${CT_ARCH_FLOAT}" in
      hard) blabla;;
      soft) blibli;;
    esac

That's why the hunk does not apply. It does not change the fact that the
case..esac is needed, it's just the way it is written now. It's easier to
read, and we now have a third case: softfp (which is hard float using
integer registers, but only for archs that support it, such as ARM).

I will adapt that part to the new construct, don't worry.

> >> +        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.
> 
> I remember that it didnt proceed compiling without it when using the
> "RESTART, STOP" option, maybe it can be removed, on the other hand it
> doesnt hurt.

OK. But I would prefer to really understand why it breaks, so we get to
fix it rather than have a workaround...

> > We need to find a place where to put that option, but I think the gcc
> > sub-menu is not the proper place.
> 
> I didnt add this to the patchset this time. I think you have
> to decide where to place CC_MULTILIB ...
> 
> > 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.

And here I said where to put it. ;-)

Thanks for the efforts you are putting in that feature. It's much
appreciated!

As I said above (it was about 1h ago for me!), I'll review the new patch
tomorrow.

Going to sleep for now... (eyes, stay open for a few more minutes, please)...

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