On 04/10/17 13:03, Jan Beulich wrote: >>>> On 03.10.17 at 20:07, <andrew.coop...@citrix.com> wrote: >> --- a/xen/Kconfig >> +++ b/xen/Kconfig >> @@ -38,4 +38,10 @@ config LTO >> >> If unsure, say N. >> >> +# >> +# For architectures that know their GCC __int128 support is sound >> +# >> +config ARCH_SUPPORTS_INT128 >> + bool > Why GCC? What about Clang?
This came straight from Linux. I can s/GCC/compiler/ if you like? > >> --- a/xen/Kconfig.debug >> +++ b/xen/Kconfig.debug >> @@ -121,6 +121,14 @@ config SCRUB_DEBUG >> Verify that pages that need to be scrubbed before being allocated to >> a guest are indeed scrubbed. >> >> +config UBSAN >> + bool "Undefined behaviour sanitizer" >> + depends on X86 > I think we should switch away from this model of explicitly stating > architectures, and instead have HAVE_* symbols selected by each > architecture supporting it, and the main symbol then depending on > the HAVE_* one. Us having only two architectures right now > doesn't make this a big difference, but Linux has (partially?) > switched to that model for a reason, I think. I'm not fussed. Which would you prefer? > >> --- a/xen/Rules.mk >> +++ b/xen/Rules.mk >> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y) >> $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS >> += >> -fprofile-arcs -ftest-coverage >> endif >> >> +ifeq ($(CONFIG_UBSAN),y) >> +$(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): >> CFLAGS += -fsanitize=undefined >> +endif > You have no users of noubsan-y, other than what Wei's RFC patch > had. Also neither you nor he explain why *.init.o are unilaterally > excluded. The answer is complicated. If you want it to work with .init. files, then the following change is required: diff --git a/xen/Rules.mk b/xen/Rules.mk index cafc67b..9ce5b56 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -189,7 +189,7 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile .text|.text.*|.data|.data.*|.bss) \ test $$sz != 0 || continue; \ echo "Error: size of $<:$$name is 0x$$sz" >&2; \ - exit $$(expr $$idx + 1);; \ + # exit $$(expr $$idx + 1);; \ esac; \ done $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ I was debating whether to keep or remove the noubsan, but I figured that keeping it would be more flexible for developing with. > >> --- a/xen/common/ubsan/ubsan.c >> +++ b/xen/common/ubsan/ubsan.c >> @@ -10,13 +10,21 @@ >> * >> */ >> >> -#include <linux/bitops.h> >> -#include <linux/bug.h> >> -#include <linux/ctype.h> >> -#include <linux/init.h> >> -#include <linux/kernel.h> >> -#include <linux/types.h> >> -#include <linux/sched.h> >> +#include <xen/bitops.h> >> +#include <xen/kernel.h> >> +#include <xen/lib.h> >> +#include <xen/types.h> >> +#include <xen/spinlock.h> >> +#include <xen/percpu.h> > I don't think all of these are needed - xen/types.h is certainly > being included implicitly by several others, for example, and > always will be. Hmm. This list of headers is ~18 months old. I will see if I can prune it some more. > >> --- a/xen/include/xen/compiler.h >> +++ b/xen/include/xen/compiler.h >> @@ -15,6 +15,7 @@ >> #define noinline __attribute__((__noinline__)) >> >> #define noreturn __attribute__((__noreturn__)) >> +#define __noreturn noreturn > Please let's avoid new name space violations if at all possibly, or > at least restrict them to individual source files where eliminating > them would be undesirable. This is entirely down to how much we want to diverge the Linux code. For ubsan, I've gone out of my way not to modify the Linux code at all. I can see an argument for making this local to the file in question. However, that needs to be weighed up against other Linux source we choose to take. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel