Re: [Xen-devel] [PATCH 4/4] xen/ubsan: Introduce and use CONFIG_UBSAN

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.10.17 at 20:07, <andrew.cooper3@xxxxxxxxxx> 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?

> --- 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.

> --- 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.

> --- 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.

> --- 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel