Hi, These three patches tidy some of the xen build runes around the use of nostdinc. Patch 3/3 hasn''t yet been tested on an actual NetBSD box, so I''d appreciate it if Patrick would give it a go. Apart from that, I''ve tested with every GCC I can lay my hands on (and clang 3.0), building x86 and ARM. I rather suspect that FreeBSD and Solaris ought to be treated the same way -- currently FreeBSD has no workarounds (but on a FreeBSD 5 box I see stdarg.h in /usr/include) and Solaris just drops the nostdinc (I have no Solaris box handy to test on). Cheers, Tim.
Tim Deegan
2013-Aug-15 13:25 UTC
[PATCH 1/3] xen: move some arch CFLAGS into the common Rules.mk.
These are the same on all current architectures, so move them into xen/Rules.mk. Lay them out a little more neatly too. Signed-off-by: Tim Deegan <tim@xen.org> --- xen/Rules.mk | 4 ++++ xen/arch/arm/Rules.mk | 2 -- xen/arch/x86/Rules.mk | 2 -- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 26e5bb6..c432ad6 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -41,6 +41,10 @@ ALL_OBJS-y += $(BASEDIR)/xsm/built_in.o ALL_OBJS-y += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o ALL_OBJS-$(x86) += $(BASEDIR)/crypto/built_in.o +CFLAGS-y += -fno-builtin -fno-common +CFLAGS-y += -Werror -Wredundant-decls -Wno-pointer-arith +CFLAGS-y += -pipe +CFLAGS-y += -iwithprefix include CFLAGS-y += -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h CFLAGS-$(XSM_ENABLE) += -DXSM_ENABLE CFLAGS-$(FLASK_ENABLE) += -DFLASK_ENABLE -DXSM_MAGIC=0xf97cff8c diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk index a18e7fd..d80dfce 100644 --- a/xen/arch/arm/Rules.mk +++ b/xen/arch/arm/Rules.mk @@ -10,8 +10,6 @@ HAS_DEVICE_TREE := y HAS_VIDEO := y HAS_ARM_HDLCD := y -CFLAGS += -fno-builtin -fno-common -Wredundant-decls -CFLAGS += -iwithprefix include -Werror -Wno-pointer-arith -pipe CFLAGS += -I$(BASEDIR)/include $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk index 0a9d68d..f629dff 100644 --- a/xen/arch/x86/Rules.mk +++ b/xen/arch/x86/Rules.mk @@ -25,8 +25,6 @@ ifneq ($(XEN_OS),SunOS) CFLAGS-$(gcc) += -nostdinc endif -CFLAGS += -fno-builtin -fno-common -Wredundant-decls -CFLAGS += -iwithprefix include -Werror -Wno-pointer-arith -pipe CFLAGS += -I$(BASEDIR)/include CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default -- 1.7.10.4
Previously we didn''t use it at all the on ARM ports or for clang builds. For ARM, I think this is just an oversight. For clang, this used not to work, because ''-withprefix include'' didn''t let us see stdarg.h, but that''s fixed in clang v3.0. Also move the ''-withprefix include'' to beside -nostdinc as it''s only needed with -nostdinc anyway. Signed-off-by: Tim Deegan <tim@xen.org> --- xen/Rules.mk | 6 +++++- xen/arch/x86/Rules.mk | 6 ------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index c432ad6..bbfc1ac 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -44,8 +44,12 @@ ALL_OBJS-$(x86) += $(BASEDIR)/crypto/built_in.o CFLAGS-y += -fno-builtin -fno-common CFLAGS-y += -Werror -Wredundant-decls -Wno-pointer-arith CFLAGS-y += -pipe -CFLAGS-y += -iwithprefix include CFLAGS-y += -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h +# Solaris puts stdarg.h &c in the system include directory. +ifneq ($(XEN_OS),SunOS) +CFLAGS-y += -nostdinc -iwithprefix include +endif + CFLAGS-$(XSM_ENABLE) += -DXSM_ENABLE CFLAGS-$(FLASK_ENABLE) += -DFLASK_ENABLE -DXSM_MAGIC=0xf97cff8c CFLAGS-$(FLASK_ENABLE) += -DFLASK_DEVELOP -DFLASK_BOOTPARAM -DFLASK_AVC_STATS diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk index f629dff..04c1a90 100644 --- a/xen/arch/x86/Rules.mk +++ b/xen/arch/x86/Rules.mk @@ -19,12 +19,6 @@ xenoprof := y # supervisor_mode_kernel ?= n -# Solaris grabs stdarg.h and friends from the system include directory. -# Clang likewise. -ifneq ($(XEN_OS),SunOS) -CFLAGS-$(gcc) += -nostdinc -endif - CFLAGS += -I$(BASEDIR)/include CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default -- 1.7.10.4
On *BSD, stdbool.h lives in /usr/include, but we don''t want to have that on the search path in case we pick up any headers from the build host''s C libraries. Copy the equivalent hack already in place for stdarg.h: on all supported compilers the contents of stdbool.h are trivial, so just supply the things we need in a xen/stdbool.h header. Signed-off-by: Tim Deegan <tim@xen.org> --- xen/include/xen/libelf.h | 4 ++-- xen/include/xen/stdbool.h | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 xen/include/xen/stdbool.h diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index e65db6d..6393040 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -29,8 +29,6 @@ #error define architectural endianness #endif -#include <stdbool.h> - typedef int elf_errorstatus; /* 0: ok; -ve (normally -1): error */ typedef int elf_negerrnoval; /* 0: ok; -EFOO: error */ @@ -39,12 +37,14 @@ typedef int elf_negerrnoval; /* 0: ok; -EFOO: error */ #ifdef __XEN__ #include <public/elfnote.h> #include <public/features.h> +#include <xen/stdbool.h> #include <xen/string.h> #else #include <xen/elfnote.h> #include <xen/features.h> #include <stdarg.h> +#include <stdbool.h> #include <string.h> struct elf_binary; diff --git a/xen/include/xen/stdbool.h b/xen/include/xen/stdbool.h new file mode 100644 index 0000000..2eecd52 --- /dev/null +++ b/xen/include/xen/stdbool.h @@ -0,0 +1,13 @@ +#ifndef __XEN_STDBOOL_H__ +#define __XEN_STDBOOL_H__ + +#if defined(__OpenBSD__) || defined(__NetBSD__) +# define bool _Bool +# define true 1 +# define false 0 +# define __bool_true_false_are_defined 1 +#else +# include <stdbool.h> +#endif + +#endif /* __XEN_STDBOOL_H__ */ -- 1.7.10.4
>>> On 15.08.13 at 15:25, Tim Deegan <tim@xen.org> wrote: > These three patches tidy some of the xen build runes around the use of > nostdinc. > > Patch 3/3 hasn''t yet been tested on an actual NetBSD box, > so I''d appreciate it if Patrick would give it a go. Apart from that, > I''ve tested with every GCC I can lay my hands on (and clang 3.0), > building x86 and ARM. > > I rather suspect that FreeBSD and Solaris ought to be treated the same > way -- currently FreeBSD has no workarounds (but on a FreeBSD 5 box I > see stdarg.h in /usr/include) and Solaris just drops the nostdinc > (I have no Solaris box handy to test on).Reviewed-by: Jan Beulich <jbeulich@suse.com>
Ian Campbell
2013-Aug-15 14:11 UTC
Re: [PATCH 1/3] xen: move some arch CFLAGS into the common Rules.mk.
On Thu, 2013-08-15 at 14:25 +0100, Tim Deegan wrote:> These are the same on all current architectures, so move them > into xen/Rules.mk. Lay them out a little more neatly too. > > Signed-off-by: Tim Deegan <tim@xen.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>> --- > xen/Rules.mk | 4 ++++ > xen/arch/arm/Rules.mk | 2 -- > xen/arch/x86/Rules.mk | 2 -- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/xen/Rules.mk b/xen/Rules.mk > index 26e5bb6..c432ad6 100644 > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -41,6 +41,10 @@ ALL_OBJS-y += $(BASEDIR)/xsm/built_in.o > ALL_OBJS-y += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o > ALL_OBJS-$(x86) += $(BASEDIR)/crypto/built_in.o > > +CFLAGS-y += -fno-builtin -fno-common > +CFLAGS-y += -Werror -Wredundant-decls -Wno-pointer-arith > +CFLAGS-y += -pipe > +CFLAGS-y += -iwithprefix include > CFLAGS-y += -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h > CFLAGS-$(XSM_ENABLE) += -DXSM_ENABLE > CFLAGS-$(FLASK_ENABLE) += -DFLASK_ENABLE -DXSM_MAGIC=0xf97cff8c > diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk > index a18e7fd..d80dfce 100644 > --- a/xen/arch/arm/Rules.mk > +++ b/xen/arch/arm/Rules.mk > @@ -10,8 +10,6 @@ HAS_DEVICE_TREE := y > HAS_VIDEO := y > HAS_ARM_HDLCD := y > > -CFLAGS += -fno-builtin -fno-common -Wredundant-decls > -CFLAGS += -iwithprefix include -Werror -Wno-pointer-arith -pipe > CFLAGS += -I$(BASEDIR)/include > > $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) > diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk > index 0a9d68d..f629dff 100644 > --- a/xen/arch/x86/Rules.mk > +++ b/xen/arch/x86/Rules.mk > @@ -25,8 +25,6 @@ ifneq ($(XEN_OS),SunOS) > CFLAGS-$(gcc) += -nostdinc > endif > > -CFLAGS += -fno-builtin -fno-common -Wredundant-decls > -CFLAGS += -iwithprefix include -Werror -Wno-pointer-arith -pipe > CFLAGS += -I$(BASEDIR)/include > CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic > CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
Ian Campbell
2013-Aug-15 14:12 UTC
Re: [PATCH 2/3] xen: move -nostdinc into common Rules.mk.
On Thu, 2013-08-15 at 14:25 +0100, Tim Deegan wrote:> Previously we didn''t use it at all the on ARM ports or for clang builds. > > For ARM, I think this is just an oversight.Yes.> > For clang, this used not to work, because ''-withprefix include'' didn''t > let us see stdarg.h, but that''s fixed in clang v3.0. > > Also move the ''-withprefix include'' to beside -nostdinc as it''s only > needed with -nostdinc anyway. > > Signed-off-by: Tim Deegan <tim@xen.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>> --- > xen/Rules.mk | 6 +++++- > xen/arch/x86/Rules.mk | 6 ------ > 2 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/xen/Rules.mk b/xen/Rules.mk > index c432ad6..bbfc1ac 100644 > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -44,8 +44,12 @@ ALL_OBJS-$(x86) += $(BASEDIR)/crypto/built_in.o > CFLAGS-y += -fno-builtin -fno-common > CFLAGS-y += -Werror -Wredundant-decls -Wno-pointer-arith > CFLAGS-y += -pipe > -CFLAGS-y += -iwithprefix include > CFLAGS-y += -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h > +# Solaris puts stdarg.h &c in the system include directory. > +ifneq ($(XEN_OS),SunOS) > +CFLAGS-y += -nostdinc -iwithprefix include > +endif > + > CFLAGS-$(XSM_ENABLE) += -DXSM_ENABLE > CFLAGS-$(FLASK_ENABLE) += -DFLASK_ENABLE -DXSM_MAGIC=0xf97cff8c > CFLAGS-$(FLASK_ENABLE) += -DFLASK_DEVELOP -DFLASK_BOOTPARAM -DFLASK_AVC_STATS > diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk > index f629dff..04c1a90 100644 > --- a/xen/arch/x86/Rules.mk > +++ b/xen/arch/x86/Rules.mk > @@ -19,12 +19,6 @@ xenoprof := y > # > supervisor_mode_kernel ?= n > > -# Solaris grabs stdarg.h and friends from the system include directory. > -# Clang likewise. > -ifneq ($(XEN_OS),SunOS) > -CFLAGS-$(gcc) += -nostdinc > -endif > - > CFLAGS += -I$(BASEDIR)/include > CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic > CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
On Thu, 2013-08-15 at 14:25 +0100, Tim Deegan wrote:> On *BSD, stdbool.h lives in /usr/include, but we don''t want to have > that on the search path in case we pick up any headers from the build > host''s C libraries. > > Copy the equivalent hack already in place for stdarg.h: on all > supported compilers the contents of stdbool.h are trivial, so just > supply the things we need in a xen/stdbool.h header. > > Signed-off-by: Tim Deegan <tim@xen.org>Acked-by: Ian Campbell <ian.campbell@citrix.com> (or Reviewed-by if you prefer) I suppose we ought to start prefering xen/stdbool.h+bool to xen/types.h +bool_t in new code and think about migrating uses of bool_t over. Ian.
On 15/08/2013 14:51, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 15.08.13 at 15:25, Tim Deegan <tim@xen.org> wrote: >> These three patches tidy some of the xen build runes around the use of >> nostdinc. >> >> Patch 3/3 hasn''t yet been tested on an actual NetBSD box, >> so I''d appreciate it if Patrick would give it a go. Apart from that, >> I''ve tested with every GCC I can lay my hands on (and clang 3.0), >> building x86 and ARM. >> >> I rather suspect that FreeBSD and Solaris ought to be treated the same >> way -- currently FreeBSD has no workarounds (but on a FreeBSD 5 box I >> see stdarg.h in /usr/include) and Solaris just drops the nostdinc >> (I have no Solaris box handy to test on). > > Reviewed-by: Jan Beulich <jbeulich@suse.com>Acked-by: Keir Fraser <keir@xen.org>
Tim Deegan
2013-Aug-15 15:55 UTC
Solaris / FreeBSD builds (was Re: [PATCH 0/3] stdinc-related build fixes.)
At 14:25 +0100 on 15 Aug (1376576704), Tim Deegan wrote:> I rather suspect that FreeBSD and Solaris ought to be treated the same > way -- currently FreeBSD has no workarounds (but on a FreeBSD 5 box I > see stdarg.h in /usr/include)Yep, FreeBSD works with the same handling as the other BSDs. (I just sent a patch to do that.)> and Solaris just drops the nostdinc (I have no Solaris box handy to test on).Playing with a Solaris 10 VM (and gcc &c from opencsw) it looks like ''-nostdinc -iwithprefix include'' should work fine there, i.e.: diff --git a/xen/Rules.mk b/xen/Rules.mk index bbfc1ac..bf870da 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -45,10 +45,7 @@ CFLAGS-y += -fno-builtin -fno-common CFLAGS-y += -Werror -Wredundant-decls -Wno-pointer-arith CFLAGS-y += -pipe CFLAGS-y += -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h -# Solaris puts stdarg.h &c in the system include directory. -ifneq ($(XEN_OS),SunOS) CFLAGS-y += -nostdinc -iwithprefix include -endif CFLAGS-$(XSM_ENABLE) += -DXSM_ENABLE CFLAGS-$(FLASK_ENABLE) += -DFLASK_ENABLE -DXSM_MAGIC=0xf97cff8c Sadly all I could find at short notice was 32-bit install media so I can''t actually test the x86_64 build. Does anyone from Oracle want to try it? :) Cheers, Tim.
On Thu, Aug 15, 2013 at 02:25:04PM +0100, Tim Deegan wrote:> These three patches tidy some of the xen build runes around the use of > nostdinc. > > Patch 3/3 hasn''t yet been tested on an actual NetBSD box, > so I''d appreciate it if Patrick would give it a go. Apart from that, > I''ve tested with every GCC I can lay my hands on (and clang 3.0), > building x86 and ARM.Your changes allowed a successful "gmake xen", thank you! If we want to carry on helping smooth out the build, I was surprised by "gmake install xen" giving [ -d //boot ] || install -d -m0755 -p //boot install: //boot exists but is not a directory given that I had configured with --prefix=/usr/local/xen. /boot isn''t a directory, it''s my second stage bootloader, so I would rather not loose it ;-) Start another thread? Cheers, Patrick
On 15/08/13 20:49, Patrick Welche wrote:> On Thu, Aug 15, 2013 at 02:25:04PM +0100, Tim Deegan wrote: >> These three patches tidy some of the xen build runes around the use of >> nostdinc. >> >> Patch 3/3 hasn''t yet been tested on an actual NetBSD box, >> so I''d appreciate it if Patrick would give it a go. Apart from that, >> I''ve tested with every GCC I can lay my hands on (and clang 3.0), >> building x86 and ARM. > Your changes allowed a successful "gmake xen", thank you! > > If we want to carry on helping smooth out the build, I was surprised > by "gmake install xen" giving > > [ -d //boot ] || install -d -m0755 -p //boot > install: //boot exists but is not a directory > > given that I had configured with --prefix=/usr/local/xen. > > /boot isn''t a directory, it''s my second stage bootloader, so I > would rather not loose it ;-) Start another thread? > > Cheers, > > PatrickAs for your boot problem, that will be something not correctly setting the DESTDIR make variable. Is gmake having a fit with the "local variable" syntax in xen/Makefile .PHONY: _install _install: D=$(DESTDIR) _install: T=$(notdir $(TARGET)) _install: Z=$(CONFIG_XEN_INSTALL_SUFFIX) _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX) [ -d $(D)/boot ] || $(INSTALL_DIR) $(D)/boot $(INSTALL_DATA) $(TARGET)$(Z) $(D)/boot/$(T)-$(XEN_FULLVERSION)$(Z) or is something else not setting DESTDIR higher up? ~Andrew> > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Thu, Aug 15, 2013 at 08:58:44PM +0100, Andrew Cooper wrote:> > given that I had configured with --prefix=/usr/local/xen. > > As for your boot problem, that will be something not correctly setting > the DESTDIR make variable.Right - I''m just being silly - what is in the "xen" directory is untouched by configure, so setting --prefix isn''t going change anything for a "gmake install xen". A "gmake -j8 install xen DESTDIR=/usr/local/xen44" was fine as you would expect... Cheers, Patrick
At 20:49 +0100 on 15 Aug (1376599776), Patrick Welche wrote:> On Thu, Aug 15, 2013 at 02:25:04PM +0100, Tim Deegan wrote: > > These three patches tidy some of the xen build runes around the use of > > nostdinc. > > > > Patch 3/3 hasn''t yet been tested on an actual NetBSD box, > > so I''d appreciate it if Patrick would give it a go. Apart from that, > > I''ve tested with every GCC I can lay my hands on (and clang 3.0), > > building x86 and ARM. > > Your changes allowed a successful "gmake xen", thank you!Great; thanks for testing. I''ve pushed them to the staging tree now. Jan, I think 3/3 (7b9685ca4ed2fd723600ce66eb20a6d0c115b6cb) is a candidate for backporting as far as 4.1. Tim.
On Thu, Aug 15, 2013 at 10:15:31PM +0100, Tim Deegan wrote:> > > I''ve tested with every GCC I can lay my hands on (and clang 3.0), > > > building x86 and ARM.Joerg has a "clang fix" for warnings for xen/arch/x86/time.c, essentially mul to mull or mulq - did you see such warnings when testing with clang? http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/sysutils/xenkernel42/patches/patch-xen_arch_x86_time.c Cheers, Patrick
At 08:17 +0100 on 16 Aug (1376641071), Patrick Welche wrote:> On Thu, Aug 15, 2013 at 10:15:31PM +0100, Tim Deegan wrote: > > > > I''ve tested with every GCC I can lay my hands on (and clang 3.0), > > > > building x86 and ARM. > > Joerg has a "clang fix" for warnings for xen/arch/x86/time.c, essentially > mul to mull or mulq - did you see such warnings when testing with clang? > > http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/sysutils/xenkernel42/patches/patch-xen_arch_x86_time.cWe already have one of those changes upstream: http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=59a28b5f045331641cbf0c1fc8d5d67afe328939 Jan, can that be backported as far as 4.2 as a build fix? I''m not sure why I didn''t trip over the mul in mul_frac() too, but we can probably take that if you''d like to send a patch. The other chunk in that file is against 32-bit code, which is gone in 4.3 and 4.4. I guess we might take that as a fix directly against 4.2. Jan, what do you think? The patch-xen_include_xen_lib.h in the same directory looks OK to me too - is that also a clang fix? The $(EXTRA_CFLAGS) runes I''m less sure of; I know people have strong feelings about the Right Way to do that. You seem to be using it to pass clang-specific options -- it might be better to build with ''clang=y'' and add any more warning suppressions in Config.mk where CFLAGS-$(clang) is set. Cheers, Tim.
>>> On 15.08.13 at 23:15, Tim Deegan <tim@xen.org> wrote: > At 20:49 +0100 on 15 Aug (1376599776), Patrick Welche wrote: >> On Thu, Aug 15, 2013 at 02:25:04PM +0100, Tim Deegan wrote: >> > These three patches tidy some of the xen build runes around the use of >> > nostdinc. >> > >> > Patch 3/3 hasn''t yet been tested on an actual NetBSD box, >> > so I''d appreciate it if Patrick would give it a go. Apart from that, >> > I''ve tested with every GCC I can lay my hands on (and clang 3.0), >> > building x86 and ARM. >> >> Your changes allowed a successful "gmake xen", thank you! > > Great; thanks for testing. I''ve pushed them to the staging tree now. > > Jan, I think 3/3 (7b9685ca4ed2fd723600ce66eb20a6d0c115b6cb) is a > candidate for backporting as far as 4.1.Okay, taking note of it. Will do as soon as the test infrastructure works again and this gets pushed through. Jan
>>> On 16.08.13 at 10:17, Tim Deegan <tim@xen.org> wrote: > At 08:17 +0100 on 16 Aug (1376641071), Patrick Welche wrote: >> On Thu, Aug 15, 2013 at 10:15:31PM +0100, Tim Deegan wrote: >> > > > I''ve tested with every GCC I can lay my hands on (and clang 3.0), >> > > > building x86 and ARM. >> >> Joerg has a "clang fix" for warnings for xen/arch/x86/time.c, essentially >> mul to mull or mulq - did you see such warnings when testing with clang? >> >> > http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/sysutils/xenkernel42/patches/ > patch-xen_arch_x86_time.c > > We already have one of those changes upstream: > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=59a28b5f045331641cbf > 0c1fc8d5d67afe328939 > > Jan, can that be backported as far as 4.2 as a build fix?Sure, even more so that this is not just a build fix - the generated code would end up being wrong if the input "delta" was put in memory instead of a register.> I''m not sure why I didn''t trip over the mul in mul_frac() too, but > we can probably take that if you''d like to send a patch.And that should relax the constraint of multiplier to "rm" at once.> The other chunk in that file is against 32-bit code, which is gone in > 4.3 and 4.4. I guess we might take that as a fix directly against 4.2. > Jan, what do you think?I''d add this as part of the backport of 59a28b5f, as logically the 64-bit change ought to be extended to 32-bit. Yet with all of this I wonder what kind of broken assembler they''re using - deferring the operand size from register operands should work consistently for all instructions or none, i.e. needing explicit suffixes on mul but not any of the other . Jan
At 09:48 +0100 on 16 Aug (1376646500), Jan Beulich wrote:> Yet with all of this I wonder what kind of broken assembler they''re > using - deferring the operand size from register operands should > work consistently for all instructions or none, i.e. needing explicit > suffixes on mul but not any of the other .It''s not just mul (see 794d4b9e85047aacfe23b852d3a03a8eff920aec in the original clang series) but it is certainly odd that it''s inconsistent Presumably in some cases the choice is constrained by some other choice the optimizer has already made. Maybe one day I''ll have the time to audit all the inline asm for implicit operand sizes. :) Tim.
>>> On 16.08.13 at 10:54, Tim Deegan <tim@xen.org> wrote: > At 09:48 +0100 on 16 Aug (1376646500), Jan Beulich wrote: >> Yet with all of this I wonder what kind of broken assembler they''re >> using - deferring the operand size from register operands should >> work consistently for all instructions or none, i.e. needing explicit >> suffixes on mul but not any of the other . > > It''s not just mul (see 794d4b9e85047aacfe23b852d3a03a8eff920aec in the > original clang series) but it is certainly odd that it''s inconsistent > Presumably in some cases the choice is constrained by some other choice > the optimizer has already made.No, from my pov that other change has nothing to do with operand size deduction. Especially the missing ''s'' suffixes on the fi... instructions were an outright bug.> Maybe one day I''ll have the time to audit all the inline asm for > implicit operand sizes. :)Uglifying the code especially for those - like me - who come from the Intel syntax world, which doesn''t know suffixes except for operands which have no way of expressing their size in non- ambiguous ways. Jan
>>> On 16.08.13 at 10:17, Tim Deegan <tim@xen.org> wrote: > At 08:17 +0100 on 16 Aug (1376641071), Patrick Welche wrote: >> On Thu, Aug 15, 2013 at 10:15:31PM +0100, Tim Deegan wrote: >> > > > I''ve tested with every GCC I can lay my hands on (and clang 3.0), >> > > > building x86 and ARM. >> >> Joerg has a "clang fix" for warnings for xen/arch/x86/time.c, essentially >> mul to mull or mulq - did you see such warnings when testing with clang? >> >> > http://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/sysutils/xenkernel42/patches/ > patch-xen_arch_x86_time.c > > We already have one of those changes upstream: > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=59a28b5f045331641cbf > 0c1fc8d5d67afe328939 > > Jan, can that be backported as far as 4.2 as a build fix?Any reason why you ask for 4.2 but not 4.1? Oh, wait - there was no clang support there yet. But as said earlier, this isn''t just a build fix, and hence I guess I''ll apply the 64-bit part of it to 4.1 anyway. Jan
Tim Deegan writes ("[PATCH 3/3] xen: Add stdbool.h workaround for
BSD."):> On *BSD, stdbool.h lives in /usr/include, but we don''t want to
have
> that on the search path in case we pick up any headers from the build
> host''s C libraries.
>
> Copy the equivalent hack already in place for stdarg.h: on all
> supported compilers the contents of stdbool.h are trivial, so just
> supply the things we need in a xen/stdbool.h header.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
The only alternative is to try to reconstruct the compiler-specific
include directory using gcc --print-libgcc-file-name and seddery.
Ian.
At 14:48 +0100 on 19 Aug (1376923719), Ian Jackson wrote:> Tim Deegan writes ("[PATCH 3/3] xen: Add stdbool.h workaround for BSD."): > > On *BSD, stdbool.h lives in /usr/include, but we don''t want to have > > that on the search path in case we pick up any headers from the build > > host''s C libraries. > > > > Copy the equivalent hack already in place for stdarg.h: on all > > supported compilers the contents of stdbool.h are trivial, so just > > supply the things we need in a xen/stdbool.h header. > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > > The only alternative is to try to reconstruct the compiler-specific > include directory using gcc --print-libgcc-file-name and seddery.Sadly, not even that. In these BSD systems, the compiler''s headers and libraries are bundled in with the rest of the system libraries, e.g.: ocelot:tjd$ gcc --print-libgcc-file-name /usr/lib/libgcc.a so it would be a matter of finding the headers we want in /usr/include and pulling on that string to grab all the headers they #include, and copying them somewhere on the Xen build''s include path. Something like: ocelot:tjd$ gcc -E -include stdarg.h -x c /dev/null | grep ''^# 1 "/usr'' # 1 "/usr/include/stdarg.h" 1 3 4 # 1 "/usr/include/sys/cdefs.h" 1 3 4 # 1 "/usr/include/sys/_types.h" 1 3 4 # 1 "/usr/include/machine/_types.h" 1 3 4 Bleah. :( I don''t feel too bad about just defining up the bits of stdarg and stdbool that we need; after all we do that for a bunch of other compiler-specific things. Another approach would be to put those definitions into compiler.h and stop including the compiler''s headers altogether, even on linux -- after all we don''t link against libgcc.a either. Tim.
Tim Deegan writes ("Re: [PATCH 3/3] xen: Add stdbool.h workaround for
BSD."):> Bleah. :(
Quite so.
> I don''t feel too bad about just defining up the bits of stdarg and
> stdbool that we need; after all we do that for a bunch of other
> compiler-specific things.
Yes.
Ian.