Re: [Xen-devel] [PATCH] fuzz, test x86_emulator: disable sse before including always_inline fns

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.18 at 21:25, <christopher.w.clark@xxxxxxxxx> wrote:
> Compiling with _FORTIFY_SOURCE or higher levels of optimization enabled
> will always_inline several library fns (memset, memcpy, ...)
> (with gcc 8.2.0 and glibc 2.28).
> 
> In fuzz and x86_emulator test, the compiler is instructed not
> to generate SSE instructions via: #pragma GCC target("no-sse")
> because those registers are needed for use by the workload.
> 
> The combination above causes compilation failure as the inline functions
> use those instructions. This is resolved by reordering the inclusion of
> <stdio.h> and <string.h> to after the pragma disabling SSE generation.
> 
> Adds compile-time checks for unwanted inclusion of stdio.h, string.h
> 
> Adds necessary (previously missing) #include <stdio.h> to x86-emulate.h

Why "necessary (previously missing)"? I can't seem to be able to
spot anything in that header that would depend on stdio.h.

> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -6,9 +6,7 @@
>  #include <stdbool.h>
>  #include <stddef.h>
>  #include <stdint.h>
> -#include <stdio.h>
>  #include <stdlib.h>
> -#include <string.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/mman.h>
> @@ -16,6 +14,8 @@
>  #include <xen/xen.h>
>  
>  #include "x86-emulate.h"
> +#include <stdio.h>
> +#include <string.h>
>  #include "fuzz-emul.h"

Such, at the first glance mis-placed, #include-s need a comment.

> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -3,12 +3,23 @@
>  #include <stddef.h>
>  #include <stdint.h>
>  #include <stdlib.h>
> -#include <string.h>
>  
> +/*
> + * Use of sse registers must be disabled prior to the definition of
> + * always_inline functions that would use them (memcpy, memset, etc).
> + */
> +#ifdef _STRING_H
> +# error "Must not include <string.h> before x86-emulate.h"
> +#endif
> +#ifdef _STDIO_H
> +# error "Must not include <stdio.h> before x86-emulate.h"
> +#endif

And what excludes other headers gaining always-inline functions
(or even having them already, when the glibc implementation is
not exactly the upstream one)? Along those lines I'm also
unconvinced _STRING_H and _STDIO_H are universally suitable
here.

>  #if __GNUC__ >= 6
>  #pragma GCC target("no-sse")
>  #endif

I think the right approach would be to move this to the top of the
file, ahead of all #include-s. However, I vaguely recall this not
having worked on at least some systems back when I had
introduced it. And really the commit introducing the above explains
why. Therefore the latest with the appearance of some problematic
inline function in stdlib.h we'd end up in a dead end.

Bottom line - I can accept something along the lines of your patch
as a workaround for what appears to be a compiler bug. But this
needs to be explained in comments, including limitations we're
aware of so that people finding further need to change this can
follow what is done why and where.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel