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

[Michael Holzheu]

Am Fri, 21 Jul 2017 18:37:25 +0300
schrieb Dmitry Safonov <dsafonov at virtuozzo.com>:

> On 07/21/2017 06:24 PM, Michael Holzheu wrote:
> > 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.
> 
> Thanks :)
> 
> > 
> >>
> >> 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?
> 
> Well, HOSTCFLAGS are meant to be used by cross-compile make.
> Some people build criu on x86 for arm embeded devices.
> So, this Makefile defines CFLAGS for compel binary tool,
> which prepares pie blobs. For cross-compilation HOSTCFLAGS are
> used for building compel tool which is used on the host during build.

Makes sense.

> 
> I'm not sure why there should be -msoft-float or
> -fno-optimize-sibling-calls here, as they applyies to compel binary,
> meant to be run during building. Maybe there is a reason, but adding
> it into HOSTCFLAGS looks pure wrong to me.

Yes I agree.
 
Michael