tools/fuzz fails due build, osstest did not notice

tools/fuzz fails due build, osstest did not notice

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 808 bytes --]

Since about two months staging fails to build because tools/fuzz can not cope with CFLAGS="-O2 -Wall  -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection". While I can easily hide the bug by undefining _FORTIFY_SOURCE, I always wonder why osstest does not catch such bugs?

Looking at some random build-amd64/6.ts-xen-build.log output, it seems no CFLAGS at all is set. I'm sure SUSE is not the only one that sets CFLAGS during their package build, and further I think SUSE is not the only one who enforces -D_FORTIFY_SOURCE= globally. So on that ground, shouldn't whatever osstest does match what the consumers of xen use? Running osstest with throw-away-binaries compiled with -D_FORTIFY_SOURCE will likely not hurt or invalidate the overall coverage.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: tools/fuzz fails due build, osstest did not notice

[Jan Beulich]

>>> On 03.09.18 at 11:53, <olaf@aepfle.de> wrote:
> Since about two months staging fails to build because tools/fuzz can not cope 
> with CFLAGS="-O2 -Wall  -fstack-protector-strong -funwind-tables 
> -fasynchronous-unwind-tables -fstack-clash-protection". While I can easily 
> hide the bug by undefining _FORTIFY_SOURCE, I always wonder why osstest does 
> not catch such bugs?
> 
> Looking at some random build-amd64/6.ts-xen-build.log output, it seems no 
> CFLAGS at all is set. I'm sure SUSE is not the only one that sets CFLAGS 
> during their package build, and further I think SUSE is not the only one who 
> enforces -D_FORTIFY_SOURCE= globally. So on that ground, shouldn't whatever 
> osstest does match what the consumers of xen use? Running osstest with 
> throw-away-binaries compiled with -D_FORTIFY_SOURCE will likely not hurt or 
> invalidate the overall coverage.

Leaving aside the suggestion you make (I'm unconvinced either variant is
strictly better than the other) - what is the actual problem? The mere
listing of compiler flags passed does not make clear to me where the clash
is, or how it would surface.

Jan



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

Re: tools/fuzz fails due build, osstest did not notice

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 3147 bytes --]

Am Mon, 03 Sep 2018 06:35:42 -0600
schrieb "Jan Beulich" <JBeulich@suse.com>:

> what is the actual problem? The mere
> listing of compiler flags passed does not make clear to me where the clash
> is, or how it would surface.

As I noticed just now, it fails to build only in Tumbleweed. So in this specific case osstest would have caught it only in a few years from now.

[   38s] make -C x86_instruction_emulator install
[   38s] make[6]: Entering directory '/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools/fuzz/x86_instruction_emulator'
[   38s] [ -L x86-emulate.h ] || ln -sf /home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools/fuzz/x86_instruction_emulator/../../../tools/tests/x86_emulator/x86-emulate.h
[   38s] [ -L x86_emulate ] || ln -sf /home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools/fuzz/x86_instruction_emulator/../../../xen/arch/x86/x86_emulate
[   38s] /usr/bin/gcc  -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O0 -fno-omit-frame-pointer -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF .fuzz-emul.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -I/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools/fuzz/x86_instruction_emulator/../../../tools/include -D__XEN_TOOLS__ -I.  -c -o fuzz-emul.o fuzz-emul.c
[   38s] In file included from /usr/include/features.h:428,   
[   38s]                  from /usr/include/assert.h:35,
[   38s]                  from fuzz-emul.c:1:
[   38s] fuzz-emul.c: In function 'input_read':
[   38s] /usr/include/bits/string_fortified.h:31:1: error: inlining failed in call to always_inline 'memcpy': target specific option mismatch
[   38s]  __NTH (memcpy (void *__restrict __dest, const void *__restrict __src,
[   38s]  ^~~~~
[   38s] fuzz-emul.c:67:5: note: called from here
[   38s]      memcpy(dst, &s->corpus->data[s->data_index], size);
[   38s]      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[   38s] In file included from /usr/include/features.h:428,   
[   38s]                  from /usr/include/assert.h:35,
[   38s]                  from fuzz-emul.c:1:
[   38s] /usr/include/bits/string_fortified.h:31:1: error: inlining failed in call to always_inline 'memcpy': target specific option mismatch
[   38s]  __NTH (memcpy (void *__restrict __dest, const void *__restrict __src,
[   38s]  ^~~~~
[   38s] fuzz-emul.c:67:5: note: called from here
[   38s]      memcpy(dst, &s->corpus->data[s->data_index], size);
[   38s]      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[   38s] make[6]: *** [/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools/fuzz/x86_instruction_emulator/../../../tools/Rules.mk:225: fuzz-emul.o] Error 1

Appending -U_FORTIFY_SOURCE in tools/fuzz fixes it. Not sure why fuzz is different from the rest of tools.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: tools/fuzz fails due build, osstest did not notice

[Jan Beulich]

>>> On 04.09.18 at 09:32, <olaf@aepfle.de> wrote:
> Am Mon, 03 Sep 2018 06:35:42 -0600
> schrieb "Jan Beulich" <JBeulich@suse.com>:
> 
>> what is the actual problem? The mere
>> listing of compiler flags passed does not make clear to me where the clash
>> is, or how it would surface.
> 
> As I noticed just now, it fails to build only in Tumbleweed. So in this 
> specific case osstest would have caught it only in a few years from now.
> 
> [   38s] make -C x86_instruction_emulator install
> [   38s] make[6]: Entering directory 
> '/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tool
> s/fuzz/x86_instruction_emulator'
> [   38s] [ -L x86-emulate.h ] || ln -sf 
> /home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools
> /fuzz/x86_instruction_emulator/../../../tools/tests/x86_emulator/x86-emulate.h
> [   38s] [ -L x86_emulate ] || ln -sf 
> /home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools
> /fuzz/x86_instruction_emulator/../../../xen/arch/x86/x86_emulate
> [   38s] /usr/bin/gcc  -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall 
> -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable 
> -Wno-unused-local-typedefs   -O0 -fno-omit-frame-pointer 
> -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF 
> .fuzz-emul.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -O2 -Wall 
> -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables 
> -fasynchronous-unwind-tables -fstack-clash-protection 
> -I/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tool
> s/fuzz/x86_instruction_emulator/../../../tools/include -D__XEN_TOOLS__ -I.  -c -o 
> fuzz-emul.o fuzz-emul.c
> [   38s] In file included from /usr/include/features.h:428,   
> [   38s]                  from /usr/include/assert.h:35,
> [   38s]                  from fuzz-emul.c:1:
> [   38s] fuzz-emul.c: In function 'input_read':
> [   38s] /usr/include/bits/string_fortified.h:31:1: error: inlining failed 
> in call to always_inline 'memcpy': target specific option mismatch
> [   38s]  __NTH (memcpy (void *__restrict __dest, const void *__restrict 
> __src,
> [   38s]  ^~~~~
> [   38s] fuzz-emul.c:67:5: note: called from here
> [   38s]      memcpy(dst, &s->corpus->data[s->data_index], size);
> [   38s]      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [   38s] In file included from /usr/include/features.h:428,   
> [   38s]                  from /usr/include/assert.h:35,
> [   38s]                  from fuzz-emul.c:1:
> [   38s] /usr/include/bits/string_fortified.h:31:1: error: inlining failed 
> in call to always_inline 'memcpy': target specific option mismatch
> [   38s]  __NTH (memcpy (void *__restrict __dest, const void *__restrict 
> __src,
> [   38s]  ^~~~~
> [   38s] fuzz-emul.c:67:5: note: called from here
> [   38s]      memcpy(dst, &s->corpus->data[s->data_index], size);
> [   38s]      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> [   38s] make[6]: *** 
> [/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tool
> s/fuzz/x86_instruction_emulator/../../../tools/Rules.mk:225: fuzz-emul.o] 
> Error 1
> 
> Appending -U_FORTIFY_SOURCE in tools/fuzz fixes it. Not sure why fuzz is 
> different from the rest of tools.

No idea here either, for the moment, even after looking at gcc's
ix86_can_inline_p() (which apparently is the function triggering the
diagnostic).

Jan



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

Re: tools/fuzz fails due build, osstest did not notice

[Christopher Clark]

On Tue, Sep 4, 2018 at 2:07 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 04.09.18 at 09:32, <olaf@aepfle.de> wrote:
> > Am Mon, 03 Sep 2018 06:35:42 -0600
> > schrieb "Jan Beulich" <JBeulich@suse.com>:
> >
> >> what is the actual problem? The mere
> >> listing of compiler flags passed does not make clear to me where the clash
> >> is, or how it would surface.
> >
> > As I noticed just now, it fails to build only in Tumbleweed. So in this
> > specific case osstest would have caught it only in a few years from now.
> >
> > [   38s] make -C x86_instruction_emulator install
> > [   38s] make[6]: Entering directory
> > '/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tool
> > s/fuzz/x86_instruction_emulator'
> > [   38s] [ -L x86-emulate.h ] || ln -sf
> > /home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools
> > /fuzz/x86_instruction_emulator/../../../tools/tests/x86_emulator/x86-emulate.h
> > [   38s] [ -L x86_emulate ] || ln -sf
> > /home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tools
> > /fuzz/x86_instruction_emulator/../../../xen/arch/x86/x86_emulate
> > [   38s] /usr/bin/gcc  -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall
> > -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable
> > -Wno-unused-local-typedefs   -O0 -fno-omit-frame-pointer
> > -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF
> > .fuzz-emul.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE  -O2 -Wall
> > -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables
> > -fasynchronous-unwind-tables -fstack-clash-protection
> > -I/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tool
> > s/fuzz/x86_instruction_emulator/../../../tools/include -D__XEN_TOOLS__ -I.  -c -o
> > fuzz-emul.o fuzz-emul.c
> > [   38s] In file included from /usr/include/features.h:428,
> > [   38s]                  from /usr/include/assert.h:35,
> > [   38s]                  from fuzz-emul.c:1:
> > [   38s] fuzz-emul.c: In function 'input_read':
> > [   38s] /usr/include/bits/string_fortified.h:31:1: error: inlining failed
> > in call to always_inline 'memcpy': target specific option mismatch
> > [   38s]  __NTH (memcpy (void *__restrict __dest, const void *__restrict
> > __src,
> > [   38s]  ^~~~~
> > [   38s] fuzz-emul.c:67:5: note: called from here
> > [   38s]      memcpy(dst, &s->corpus->data[s->data_index], size);
> > [   38s]      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > [   38s] In file included from /usr/include/features.h:428,
> > [   38s]                  from /usr/include/assert.h:35,
> > [   38s]                  from fuzz-emul.c:1:
> > [   38s] /usr/include/bits/string_fortified.h:31:1: error: inlining failed
> > in call to always_inline 'memcpy': target specific option mismatch
> > [   38s]  __NTH (memcpy (void *__restrict __dest, const void *__restrict
> > __src,
> > [   38s]  ^~~~~
> > [   38s] fuzz-emul.c:67:5: note: called from here
> > [   38s]      memcpy(dst, &s->corpus->data[s->data_index], size);
> > [   38s]      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > [   38s] make[6]: ***
> > [/home/abuild/rpmbuild/BUILD/xen-4.12.20180831T120653.6164970942/non-dbg/tool
> > s/fuzz/x86_instruction_emulator/../../../tools/Rules.mk:225: fuzz-emul.o]
> > Error 1
> >
> > Appending -U_FORTIFY_SOURCE in tools/fuzz fixes it. Not sure why fuzz is
> > different from the rest of tools.
>
> No idea here either, for the moment, even after looking at gcc's
> ix86_can_inline_p() (which apparently is the function triggering the
> diagnostic).

I've just encountered this problem, building with the master branch of
OpenEmbedded and Yocto's poky, with compiler flags that include
    _FORTIFY_SOURCE=2 -msse3
with gcc 8.2.0.

Xen's x86_emulator header file does:
    #pragma GCC target("no-sse")

The pragma was introduced in Xen commit 79136f26, to prevent the compiler from
using registers otherwise used by inline memset and memcpy.

Setting _FORTIFY_SOURCE causes the use of always_inline variants of memset and
memcpy which use those instructions, which is now causing the inline to fail.

The behaviour is described in the GCC bugzilla here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71991

{quote}
    cat fuzz-emul.i
    __attribute__((__always_inline__)) a() {}
    #pragma GCC target "no-sse"
    b() { a(); }

    Where 'a' is 'memcpy' and 'b' is a function in xen.

    Can you Honza also take a look r251333 where we started to reject such code?
{quote}

links to this GCC change:
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=251333

Would the below Xen patch, which adds undefs for _FORTIFY_SOURCE just for the
two binaries which are affected be acceptable? It allows for use of
_FORTIFY_SOURCE with the rest of the build.

Christopher

diff --git a/tools/tests/x86_emulator/Makefile
b/tools/tests/x86_emulator/Makefile
index dec81c3..f7b9e58 100644
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -133,7 +133,7 @@ x86_emulate/%: x86_emulate ;

 HOSTCFLAGS-x86_64 := -fno-PIE
 $(call cc-option-add,HOSTCFLAGS-x86_64,HOSTCC,-no-pie)
-HOSTCFLAGS += $(CFLAGS_xeninclude) -I. $(HOSTCFLAGS-$(XEN_COMPILE_ARCH))
+HOSTCFLAGS += $(CFLAGS_xeninclude) -I.
$(HOSTCFLAGS-$(XEN_COMPILE_ARCH)) -U_FORTIFY_SOURCE

 x86.h := $(addprefix $(XEN_ROOT)/tools/include/xen/asm/,\
                      x86-vendors.h x86-defns.h msr-index.h)
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile
b/tools/fuzz/x86_instruction_emulator/Makefile
index eb88f94..83e5633 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -16,7 +16,7 @@ x86_emulate/%: x86_emulate ;
 x86-emulate.c x86-emulate.h wrappers.c: %:
    [ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$*

-CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I.
+CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I. -U_FORTIFY_SOURCE

 GCOV_FLAGS := --coverage
 %-cov.o: %.c

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

Re: tools/fuzz fails due build, osstest did not notice

[Jan Beulich]

>>> On 20.09.18 at 20:19, <christopher.w.clark@gmail.com> wrote:
> I've just encountered this problem, building with the master branch of
> OpenEmbedded and Yocto's poky, with compiler flags that include
>     _FORTIFY_SOURCE=2 -msse3
> with gcc 8.2.0.
> 
> Xen's x86_emulator header file does:
>     #pragma GCC target("no-sse")
> 
> The pragma was introduced in Xen commit 79136f26, to prevent the compiler from
> using registers otherwise used by inline memset and memcpy.
> 
> Setting _FORTIFY_SOURCE causes the use of always_inline variants of memset and
> memcpy which use those instructions, which is now causing the inline to fail.
> 
> The behaviour is described in the GCC bugzilla here:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71991 
> 
> {quote}
>     cat fuzz-emul.i
>     __attribute__((__always_inline__)) a() {}
>     #pragma GCC target "no-sse"
>     b() { a(); }
> 
>     Where 'a' is 'memcpy' and 'b' is a function in xen.
> 
>     Can you Honza also take a look r251333 where we started to reject such 
> code?
> {quote}
> 
> links to this GCC change:
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=251333 
> 
> Would the below Xen patch, which adds undefs for _FORTIFY_SOURCE just for the
> two binaries which are affected be acceptable? It allows for use of
> _FORTIFY_SOURCE with the rest of the build.

In a suitably massaged form I think this could be acceptable. It needs
to fully describe the reasons for the addition, and if at all possible it
should add the -U only for affected compilers (the bugzilla entry you
found suggests to me that this has been fixed earlier this year).

That said, I'm certainly feeling a little uneasy about adding such
overrides to the build flags "just for the two binaries which are
affected". How can we be certain the same issue won't creep in
elsewhere?

Furthermore it looks as if _FORTIFY_SOURCE implied
__always_inline__ solely because its use takes a minimum level of
optimization as a prereq. If that's indeed the case, un-defining that
symbol won't help builds which enable sufficient optimization,
irrespective of the use of _FORTIFY_SOURCE. Yet I'd prefer to see
the root of the issue addressed, not just one particular incarnation.

Jan



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

[PATCH] fuzz, test x86_emulator: disable sse before including always_inline fns

[Christopher Clark]

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

Signed-off-by: Christopher Clark <christopher.clark6@baesystems.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c |  4 ++--
 tools/tests/x86_emulator/wrappers.c             |  3 ++-
 tools/tests/x86_emulator/x86-emulate.h          | 13 ++++++++++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 03a2473..d0a02d5 100644
--- 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"
 
 #define MSR_INDEX_MAX 16
diff --git a/tools/tests/x86_emulator/wrappers.c b/tools/tests/x86_emulator/wrappers.c
index d02013c..349b9de 100644
--- a/tools/tests/x86_emulator/wrappers.c
+++ b/tools/tests/x86_emulator/wrappers.c
@@ -1,9 +1,10 @@
 #include <stdarg.h>
-#include <stdio.h>
 
 #define WRAP(x) typeof(x) emul_##x
 #include "x86-emulate.h"
 
+#include <stdio.h>
+
 size_t emul_fwrite(const void *src, size_t sz, size_t n, FILE *f)
 {
     emul_save_fpu_state();
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index b249e46..8760bb8 100644
--- 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
 #if __GNUC__ >= 6
 #pragma GCC target("no-sse")
 #endif
 
+#include <string.h>
+#include <stdio.h>
 #include <xen/xen.h>
 
 #include <xen/asm/msr-index.h>
-- 
2.1.4


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

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

[Jan Beulich]

>>> On 21.09.18 at 21:25, <christopher.w.clark@gmail.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Christopher Clark]

On Mon, Sep 24, 2018 at 5:06 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 21.09.18 at 21:25, <christopher.w.clark@gmail.com> wrote:
> >
> > 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.

When WRAP is defined prior to including x86-emulate.h, it introduces
the need for stdio.h to be included to avoid:

| x86-emulate.h:83:6: error: 'fwrite' undeclared here (not in a
function); did you mean 'free'?
|  WRAP(fwrite);

and similar for the other wrapped functions.

It was fine previously as wrapper.c included <stdio.h> prior to "x86-emulate.h"
but that gets changed with this commit. I'll rewrite that part of the commit
message though, and I'll also made the new <stdio.h> inclusion
dependent on WRAP,
since WRAP being defined is what prompts the need for it.

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

ack, will do.

>
> > --- 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)?

It is possible that could happen; but these two are the headers that are known
to be a problem today, and are also the same headers that define the functions
already listed within x86-emulate.h as deserving of their own wrappers, so
it seems tolerable to introduce checking for just these two.

> Along those lines I'm also
> unconvinced _STRING_H and _STDIO_H are universally suitable
> here.

OK, I'll switch the <stdio.h> check over to look for EOF instead, as that's
defined in the C standard, which will make the check portable. I don't have a
similar option for <string.h> though - however, I don't think it absolutely has
to be universally correct: for false-negatives, GLIBC is common enough that it
should be caught relatively quickly if a violation is somehow introduced by
a developer not using GLIBC, and false-positives seem unlikely in practice.

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

Yes, this is still the case, failing in my development environment at least.
Declaration of atof fails with: "SSE register return with SSE disabled".

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

Yes, that would be an unfortunate (compiler, optimize level, library)
tuple to have to handle.

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

Thanks for the review. I will send an updated version of the patch
aiming to address these comments shortly.

Christopher

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

[PATCH v2] fuzz, test x86_emulator: disable sse before including always_inline fns

[Christopher Clark]

Workaround for compiler rejection of SSE-using always_inlines defined before
SSE is disabled.

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.

It would be preferable to locate the no-sse pragma within x86-emulate.h at the
top of the file, prior to including any other headers; unfortunately doing so
before <stdlib.h> causes compilation failure due to declaration of 'atof' with:
  "SSE register return with SSE disabled".
Fortunately there is no (known) current dependency on any always_inline
SSE-inclined function declared in <stdlib.h> or any of its dependencies, so the
pragma is therefore issued immediately after inclusion of <stdlib.h> with a
comment introduced to explain its location there.

Add compile-time checks for unwanted prior inclusion of <string.h> and
<stdio.h>, which are the two headers that provide the library functions that
are handled with wrappers and listed within "x86-emulate.h" as ones "we think
might access any of the FPU state".
* Use standard-defined "EOF" macro to detect prior <stdio.h> inclusion.
* Use "_STRING_H" (non-standardized guard macro) as best-effort
  for detection of prior <string.h> inclusion. This is non-universally
  viable but will provide error output on common GLIBC systems, so
  provides some defensive coverage.

Adds conditional #include <stdio.h> to x86-emulate.h because fwrite, printf,
etc. are referenced when WRAP has been defined.

Signed-off-by: Christopher Clark <christopher.clark6@baesystems.com>
---
Changes in v2:

* Added comment to explain the deliberate ordering of includes that might
  otherwise be believed to be incorrect.

* Added comment to explain the positioning of the no-sse pragma and the
  ordering of includes around it.

* Put the defensive header-inclusion compile-time checks right next to the
  inclusion of the headers that they are checking for.

* Narrowed the inclusion of stdio.h in x86-emulate.h to be conditional on
  WRAP being defined, since that condition determines the need to include it.
  Removed the include of stdio.h from wrappers.c as no longer required and to
  obviate the need for an explanatory comment about include ordering there.

* Use definition of 'EOF' to detect prior inclusion of <stdio.h>, since this
  is defined in the standard and increases portability to other C libraries.

* Added context information to the commit message.

 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 10 +++++++--
 tools/tests/x86_emulator/wrappers.c             |  1 -
 tools/tests/x86_emulator/x86-emulate.h          | 28 +++++++++++++++++++++++--
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 03a2473..0ffd0fb 100644
--- 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,14 @@
 #include <xen/xen.h>
 
 #include "x86-emulate.h"
+/*
+ * include "x86-emulate.h" prior to <stdio.h> and <string.h>:
+ * x86-emulate.h disables use of SSE registers, while <stdio.h> and <string.h>
+ * declare functions that may be always_inline and use those registers
+ * unless they have been disabled earlier, which can fail to compile.
+ */
+#include <stdio.h>
+#include <string.h>
 #include "fuzz-emul.h"
 
 #define MSR_INDEX_MAX 16
diff --git a/tools/tests/x86_emulator/wrappers.c b/tools/tests/x86_emulator/wrappers.c
index d02013c..eba7cc9 100644
--- a/tools/tests/x86_emulator/wrappers.c
+++ b/tools/tests/x86_emulator/wrappers.c
@@ -1,5 +1,4 @@
 #include <stdarg.h>
-#include <stdio.h>
 
 #define WRAP(x) typeof(x) emul_##x
 #include "x86-emulate.h"
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index b249e46..07ea1e8 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -3,11 +3,35 @@
 #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),
+ * so do this as early as possible, aiming to be before any always_inline
+ * functions that are used are declared.
+ * Unfortunately, this cannot be done prior to inclusion of <stdlib.h>
+ * due to functions such as 'atof' that have SSE register return declared,
+ * so do so here, immediately after that.
+ */
 #if __GNUC__ >= 6
 #pragma GCC target("no-sse")
 #endif
+ /*
+ * Attempt detection of unwanted prior inclusion of some headers known to use
+ * always_inline with SSE registers in some library / compiler / optimization
+ * combinations.
+ */
+#ifdef _STRING_H
+# error "Must not include <string.h> before x86-emulate.h"
+#endif
+#include <string.h>
+
+/* EOF is a standard macro defined in <stdio.h> so use it for detection */
+#ifdef EOF
+# error "Must not include <stdio.h> before x86-emulate.h"
+#endif
+#ifdef WRAP
+#include <stdio.h>
+#endif
 
 #include <xen/xen.h>
 
-- 
2.1.4


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

Re: [PATCH v2] fuzz, test x86_emulator: disable sse before including always_inline fns

[Wei Liu]

This patch is causing build failure.

See:

http://logs.test-lab.xenproject.org/osstest/logs/128087/build-amd64/6.ts-xen-build.log

Wei.

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

Re: [PATCH v2] fuzz, test x86_emulator: disable sse before including always_inline fns

[Wei Liu]

On Wed, Sep 26, 2018 at 09:36:39AM +0100, Wei Liu wrote:
> This patch is causing build failure.
> 
> See:
> 
> http://logs.test-lab.xenproject.org/osstest/logs/128087/build-amd64/6.ts-xen-build.log

Oh, it appears that Jan already posted a fix.

Wei.

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

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

[Jan Beulich]

>>> On 25.09.18 at 01:17, <christopher.w.clark@gmail.com> wrote:
> On Mon, Sep 24, 2018 at 5:06 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 21.09.18 at 21:25, <christopher.w.clark@gmail.com> wrote:
>> >
>> > 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.
> 
> When WRAP is defined prior to including x86-emulate.h, it introduces
> the need for stdio.h to be included to avoid:
> 
> | x86-emulate.h:83:6: error: 'fwrite' undeclared here (not in a
> function); did you mean 'free'?
> |  WRAP(fwrite);
> 
> and similar for the other wrapped functions.
> 
> It was fine previously as wrapper.c included <stdio.h> prior to 
> "x86-emulate.h"
> but that gets changed with this commit. I'll rewrite that part of the commit
> message though, and I'll also made the new <stdio.h> inclusion
> dependent on WRAP,
> since WRAP being defined is what prompts the need for it.

But that's (in my opinion) a requirement towards the file defining
WRAP then, not towards the header.

>> >  #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.
> 
> Yes, this is still the case, failing in my development environment at least.
> Declaration of atof fails with: "SSE register return with SSE disabled".

Hmm - I (now) wonder whether -mfpmath= could help here.

Jan



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

Re: [PATCH v2] fuzz, test x86_emulator: disable sse before including always_inline fns

[Jan Beulich]

>>> On 25.09.18 at 01:22, <christopher.w.clark@gmail.com> wrote:
> --- a/tools/tests/x86_emulator/wrappers.c
> +++ b/tools/tests/x86_emulator/wrappers.c
> @@ -1,5 +1,4 @@
>  #include <stdarg.h>
> -#include <stdio.h>
>  
>  #define WRAP(x) typeof(x) emul_##x
>  #include "x86-emulate.h"

Ah, I see now why you can't take care of the inclusion here.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

with one cosmetic remark (which I may take care of while committing):

> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -3,11 +3,35 @@
>  #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),
> + * so do this as early as possible, aiming to be before any always_inline
> + * functions that are used are declared.
> + * Unfortunately, this cannot be done prior to inclusion of <stdlib.h>
> + * due to functions such as 'atof' that have SSE register return declared,
> + * so do so here, immediately after that.
> + */
>  #if __GNUC__ >= 6
>  #pragma GCC target("no-sse")
>  #endif
> + /*
> + * Attempt detection of unwanted prior inclusion of some headers known to use
> + * always_inline with SSE registers in some library / compiler / optimization
> + * combinations.
> + */
> +#ifdef _STRING_H
> +# error "Must not include <string.h> before x86-emulate.h"
> +#endif
> +#include <string.h>
> +
> +/* EOF is a standard macro defined in <stdio.h> so use it for detection */
> +#ifdef EOF
> +# error "Must not include <stdio.h> before x86-emulate.h"
> +#endif
> +#ifdef WRAP
> +#include <stdio.h>
> +#endif

There's some inconsistency with directive indentation: The two "error"
ones you indent, but the "include" you leave unindented (matching the
pre-existing "pragma"). At least all your additions should be consistent
with one another.

Jan



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