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

[Dmitry Safonov]

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.

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.

NO_RELOCS compile definition is for building compel binary, which
shouldn't handle relocations. It's used ATM for arm/aarch64 pie's,
for which there isn't any code in compel to handle relocs (yet).
Maybe name is a bit misleading.


-- 
              Dmitry