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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.10.17 at 15:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/10/17 13:03, Jan Beulich wrote:
>>>>> 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?
> 
> This came straight from Linux.  I can s/GCC/compiler/ if you like?

Yes please (provided it's usable with 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.
> 
> I'm not fussed.  Which would you prefer?

I'd prefer the combination HAVE_UBSAN plus UBSAN, as outlined.

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

What about this part?

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

Could you mention this in the commit message then, please?

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

Except for the #include-s.

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

As there's no reasonable chance for us to ever be able to take
a Linux file completely unmodified, if putting such #define-s into
individual files is too restrictive for your taste, how about making
those files identify themselves (by, say, #define LINUX_SOURCE)
and enabling such compatibility things only for those?

Jan


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