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

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Mon, Sep 24, 2018 at 5:06 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 21.09.18 at 21:25, <christopher.w.clark@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel