Date: Thu, 15 Oct 2015 18:22:58 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <ngie@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r289332 - head/tools/regression/lib/msun Message-ID: <20151015145016.V1299@besplex.bde.org> In-Reply-To: <201510142022.t9EKMC1C088993@repo.freebsd.org> References: <201510142022.t9EKMC1C088993@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 14 Oct 2015, Garrett Cooper wrote: > Log: > Fix test-fenv:test_dfl_env when run on some amd64 CPUs > > Compare the fields that the AMD [1] and Intel [2] specs say will be > set once fnstenv returns. > > Not all amd64 capable processors zero out the env.__x87.__other field > (example: AMD Opteron 6308). The AMD64/x64 specs aren't explicit on what the > env.__x87.__other field will contain after fnstenv is executed, so the values > in env.__x87.__other could be filled with arbitrary data depending on how the > CPU-specific implementation of fnstenv. This is not very amd64-specific. All of the state is basically uninitialized. The ifdef should be the same for i386, so as to only check the part that we use. This part is not properly initialized either, but is less volatile and we need to check that its uninitialized value is what we want. > Modified: head/tools/regression/lib/msun/test-fenv.c > ============================================================================== > --- head/tools/regression/lib/msun/test-fenv.c Wed Oct 14 19:30:04 2015 (r289331) > +++ head/tools/regression/lib/msun/test-fenv.c Wed Oct 14 20:22:12 2015 (r289332) > @@ -133,8 +133,35 @@ test_dfl_env(void) > fenv_t env; > > fegetenv(&env); The fields were initialized in the kernel, not necessarily to our hard- coded value, but we want to check that they were. Then they may have been changed by early code in the program. There is a printf() there, and printf() could easily use the FPU although it probably doesn't. If anything earlier uses the FPU, the the state may be changed in many MD ways. E.g.: - if the part of the FPU used is the NPX, then the state has last-insruction data that should be changed by any non-control operation - the last-instruction data actually works on Intel CPU, but is broken by AMD optimizations - the amd64 optimizations give a security hole. Closing this hole modifies the last-instruction data, so the data may change in unexpected ways - amd64 binaries are unlikely to use the NPX even if they use the FPU, since they use SSE except for long doubles - i386 binaries should use the NPX if they use the FPU, but this is broken by clang optimizations in some cases - the hardware format stored by fnstenv doesn't have enough space for all the last-instruction data in long mode. Apparently, fstenv in long mode stores the same number of bytes as in 32-bit mode, with the leading bytes the same but the tailing bytes containing either truncated last-instruction data or garbage. This is probably the bug fixed by this commit (it doesn't take earlier use to give MD garbage). The kernel doesn't define any format for this case. fenv(3)'s fenv_t is a private format that has 12 leading bytes in common with the hardware format, then 16 opaque bytes in common with the unusable part of the hardware format, then an extension by 4 bytes. Testing this confused me. I thought that FreeBSD userland was in long mode, but tests gave the 32-bit format even for fxsave. This format has fields for segment registers, and shows the nonzero %cs and the zero %ds. > + > +#ifdef __amd64__ > + /* > + * Compare the fields that the AMD [1] and Intel [2] specs say will be > + * set once fnstenv returns. > + * > + * Not all amd64 capable processors implement the fnstenv instruction > + * by zero'ing out the env.__x87.__other field (example: AMD Opteron > + * 6308). The AMD64/x64 specs aren't explicit on what the > + * env.__x87.__other field will contain after fnstenv is executed, so > + * the values in env.__x87.__other could be filled with arbitrary > + * data depending on how the CPU implements fnstenv. Probably none do. Most of the fields that are still tested are documented in at least old amd64 manuals as being half "reserved, don't use". Usually the reserved bits are not 0, but are 1. Our tests usually pass because we hard-code 1 instead of 0. > + * > + * 1. http://support.amd.com/TechDocs/26569_APM_v5.pdf > + * 2. http://www.intel.com/Assets/en_US/PDF/manual/253666.pdf > + */ It should go without saying that fields that are unsupported by us and named __other to inhibit their use should not be tested. Verbose pointers to the man pages might be useful for the parts that we use, but these parts are fundamental and well known and used without comment throughout the implementaton. > + assert(memcmp(&env.__mxcsr, &FE_DFL_ENV->__mxcsr, > + sizeof(env.__mxcsr)) == 0); memcmp is verbose and bogus for comparing scalar values. All 32 bits in the scalar are supported, so this is the only correct part of the test. Oops, this too is incorrect on i386. On i386, __mxcsr doesn't exist. The mxcsr scalar is split into 2 discontiguous parts on i386. These parts need to be tested separately, and the amd64 spelling fails at compile time since neither part is named __mxcsr. > + assert(memcmp(&env.__x87.__control, &FE_DFL_ENV->__x87.__control, > + sizeof(env.__x87.__control)) == 0); This is broken on amd64 since the upper 16 bits of the scalar are "reserved, don't use". This is accidentally correct on i386 due to a hack. This field is basically 16 bits and is just that in 16-bit mode. It is padded with "reserved, don't use" bits in 32-bit mode (the format is expanded to make space), but in other (64-bit) formats it is compressed back to 16 bits. The kernel declares the padding as part of the field in 32-bit formats for convenience. So does fenv(3) for amd64. This is fragile but OK since the field is private. On i386, this test works accidentally since the field is declared as 16 bits so as to repurpose the padding bits for holding half of the mxcsr bits. > + assert(memcmp(&env.__x87.__status, &FE_DFL_ENV->__x87.__status, > + sizeof(env.__x87.__status)) == 0); This has the same bugs and accidental fix on i386 as the control word. The status word is the most volatile one so it is most likely to be changed from its kernel initialization by earlier code in the program. The only reasonable fix is to do the fegetenv() as the first thing in main(). Moving it earlier would be unreasonable, and initializing everything to a known state would break the test that the default state is as expected. Alternatively, only test the supported and/or non-volatile parts of the state -- just the exception flags. Bits giving the result of previous FP comparisons are uninteresting. Bits giving the stack pointer are interesting but not really supported -- we just need them to not give a corrupt stack. We don't support them enough to know when that is, and just test if they are 0 (or 1's?) > + assert(memcmp(&env.__x87.__tag, &FE_DFL_ENV->__x87.__tag, > + sizeof(env.__x87.__tag)) == 0); Similarly, except it is not accidentally fixed on i386. The most correct fix is to declare the padding explicitly and never look at it. Next best is to mask the bits so as to not look at them. Don't obfuscate this using memcmp() with a reduced size. Use: assert((env.__x87.__tag & 0xffff) == (FE_DFL_ENV->__x87.__tag & 0xffff)); Tag bits may be changed during previous FP operations, but I think the ABI requires them to be restored to EMPTY for subsequent operations to work. > +#else > assert(memcmp(&env, FE_DFL_ENV, sizeof(env)) == 0); > #endif > + > +#endif > assert(fetestexcept(FE_ALL_EXCEPT) == 0); Checking exception status bits in __status is not really needed since this checks these bits again. > } Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151015145016.V1299>