[CRIU] [PATCH v2] s390: Move -msoft-float/-fno-optimize-sibling-calls into compel Makefiles

[Michael Holzheu]

Am Fri, 21 Jul 2017 16:23:31 +0300
schrieb Dmitry Safonov <dsafonov at virtuozzo.com>:

> On 07/21/2017 02:49 PM, Michael Holzheu wrote:
> > We currently define "CFLAGS += -msoft-float -fno-optimize-sibling-calls"
> > in "Makefile.compel".
> > 
> > Makefile.compel is included into the toplevel Makefile and so changes
> > CFLAGS which are exported to all criu and zdtm Makefiles.
> > 
> > We must not use -msoft-float outside the compel files. E.g. otherwise for
> > zdtm we get the following build error:
> > 
> > uptime_grow.o: In function `main':
> > /tmp/2/criu/test/zdtm/static/uptime_grow.c:36: undefined reference to `__floatdidf'
> > /tmp/2/criu/test/zdtm/static/uptime_grow.c:36: undefined reference to `__muldf3'
> > /tmp/2/criu/test/zdtm/static/uptime_grow.c:36: undefined reference to `__floatdidf'
> > /tmp/2/criu/test/zdtm/static/uptime_grow.c:36: undefined reference to `__adddf3
> > 
> > Fix this and move the CFLAGS definition to the compel Makefile.
> > 
> > As compel/plugins/Makefile and compel/Makefile are called recursively,
> > changed CFLAGS will not be exported back to the top Makefile.
> > 
> > Reported-by: Adrian Reber <areber at redhat.com>
> > Signed-off-by: Michael Holzheu <holzheu at linux.vnet.ibm.com>
> 
> Hm, I'm not against the patch, but it sprays this CFLAGS addition across
> sources..
> What do you think, can we have this in per-arch zone in the top
> Makefile?
> 
> Something like:
>  > --- a/Makefile
>  > +++ b/Makefile
>  > @@ -70,6 +70,7 @@ ifeq ($(ARCH),s390)
>  >          SRCARCH                := s390
>  >          VDSO           := y
>  >          DEFINES                := -DCONFIG_S390
>  > +        CFLAGS_PIE     := -msoft-float -fno-optimize-sibling-calls
>  >  endif
>  >
>  >  LDARCH ?= $(SRCARCH)
> 
> (with your that big comment). And add it to relevant
> ccflags-y += CFLAGS_PIE
> 
> So we could fit it in one place and maybe other arch's have common
> PIE CFLAGS, they need to use.

This sounds like a very good idea - I will send a v3 patch.

> 
> There is also a question inline below.
> 
> > ---
> >   Makefile.compel           | 10 ----------
> >   compel/Makefile           | 14 ++++++++++++++
> >   compel/plugins/Makefile   | 14 ++++++++++++++
> >   criu/pie/Makefile         |  8 +++++++-
> >   criu/pie/Makefile.library |  9 ++++++++-
> >   5 files changed, 43 insertions(+), 12 deletions(-)
> > 
> > diff --git a/Makefile.compel b/Makefile.compel
> > index 5c854e3..1ef7f8c 100644
> > --- a/Makefile.compel
> > +++ b/Makefile.compel
> > @@ -70,13 +70,3 @@ compel/$(LIBCOMPEL_SO): compel/$(LIBCOMPEL_A)
> >   compel-install-targets	+= compel/$(LIBCOMPEL_SO)
> >   compel-install-targets	+= compel/compel
> >   compel-install-targets	+= $(compel-plugins)
> > -
> > -#
> > -# Also ensure with -fno-optimize-sibling-calls that we don't create GOT
> > -# (Global Offset Table) relocations with gcc compilers that don't have
> > -# commit "S/390: Fix 64 bit sibcall".
> > -#
> > -ifeq ($(ARCH),s390)
> > -CFLAGS += -msoft-float -fno-optimize-sibling-calls
> > -HOSTCFLAGS += -msoft-float -fno-optimize-sibling-calls
> > -endif
> > diff --git a/compel/Makefile b/compel/Makefile
> > index 43d27f5..81b9614 100644
> > --- a/compel/Makefile
> > +++ b/compel/Makefile
> > @@ -34,6 +34,20 @@ CFLAGS			+= -DNO_RELOCS
> >   HOSTCFLAGS		+= -DNO_RELOCS
> >   endif
> >   
> > +#
> > +# We assume that compel code does not change floating point registers.
> > +# On s390 gcc uses fprs to cache gprs. Therefore disable floating point
> > +# with -msoft-float.
> > +#
> > +# Also ensure with -fno-optimize-sibling-calls that we don't create GOT
> > +# (Global Offset Table) relocations with gcc compilers that don't have
> > +# commit "S/390: Fix 64 bit sibcall".
> > +#
> > +ifeq ($(ARCH),s390)
> > +CFLAGS += -msoft-float -fno-optimize-sibling-calls
> > +HOSTCFLAGS += -msoft-float -fno-optimize-sibling-calls
> 
> Hm, why is it needed in HOSTCFLAGS?

Probably I did not understand the meaning of HOSTCFLAGS and just
did it the same way as in:

ifneq ($(filter arm aarch64,$(ARCH)),)
CFLAGS                  += -DNO_RELOCS
HOSTCFLAGS              += -DNO_RELOCS
endif

But looks like it is not required.

So what is the meaning for HOSTCFLAGS?

Michael