On 10.03.2025 14:48, Martin Storsjö wrote:
On Wed, 5 Mar 2025, Jacek Caban wrote:
Based on Wine code by Piotr Caban and Alexandre Julliard, who granted
permission to use it under the mingw-w64 license.
Unlike earlier versions, msvcr120 and UCRT implement fenv.h, but
their representation differs from what mingw-w64
has used so far. fenv_t has a target-independent layout and flags,
requiring translation to and from machine-specific
flags. Generic helpers handle this conversion.
Side note nitpick: The commit message here has longer lines than the
common git standard; it makes it more readable in 80 char wide
terminals if it would be word wrapped to the usual git commit message
width :-)
I didn't know we're sticking with that, I will change it.
It's probably clearer elsewhere, but is the case that x is a fenv word
for x87, and y is one for sse?
It's also kinda hard to look at the constant definitions, as part of
them is identical across FENV_X, but part of them isn't (the upper
bits). For people not intimately familiar with the x86-isms, it would
be nice with some comment somewhere, but I'm not sure where that would
be best placed.
Yes, I will add a comment.
This could really warrant a comment explaining what it does, as 0x20
isn't a named constant.
It originates from your Wine patch:
https://gitlab.winehq.org/wine/wine/-/commit/6391a9f8978b42b71b37c27b673a0a13c21ea577
I guess it's based on testing Windows behavior, I don't know why Windows
does it. I can add a comment like "for Windows compatibility".
BTW, I realized I missed your authorship of the code in the commit
message. Sorry about that, I will fix it.
diff --git a/mingw-w64-crt/misc/fegetenv.c
b/mingw-w64-crt/misc/fegetenv.c
index e17fd491c..cadaf4572 100644
--- a/mingw-w64-crt/misc/fegetenv.c
+++ b/mingw-w64-crt/misc/fegetenv.c
@@ -4,34 +4,25 @@
* No warranty is given; refer to the file DISCLAIMER.PD within this
package.
*/
-#include <fenv.h>
#include <internal.h>
/* 7.6.4.1
The fegetenv function stores the current floating-point environment
in the object pointed to by envp. */
-int fegetenv (fenv_t * envp)
+int fegetenv(fenv_t *env)
{
-#if defined(_ARM_) || defined(__arm__)
- __asm__ volatile ("fmrx %0, FPSCR" : "=r" (*envp));
-#elif defined(_ARM64_) || defined(__aarch64__)
- unsigned __int64 fpcr;
- __asm__ volatile ("mrs %0, fpcr" : "=r" (fpcr));
- envp->__cw = fpcr;
+#if defined(__i386__) || (defined(__x86_64__) && !defined(__arm64ec__))
+ unsigned int x87, sse;
+ __mingw_control87_2(0, 0, &x87, &sse);
+ env->_Fe_ctl = fenv_encode(x87, sse);
+ __mingw_setfp(NULL, 0, &x87, 0);
+ __mingw_setfp_sse(NULL, 0, &sse, 0);
+ env->_Fe_stat = fenv_encode(x87, sse);
#else
- __asm__ __volatile__ ("fnstenv %0;": "=m" (*envp));
- /* fnstenv sets control word to non-stop for all exceptions, so we
- need to reload our env to restore the original mask. */
- __asm__ __volatile__ ("fldenv %0" : : "m" (*envp));
- if (__mingw_has_sse ())
- {
- int _mxcsr;
- __asm__ __volatile__ ("stmxcsr %0" : "=m" (_mxcsr));
- envp->__unused0 = (((unsigned int) _mxcsr) >> 16);
- envp->__unused1 = (((unsigned int) _mxcsr) & 0xffff);
- }
-#endif /* defined(_ARM_) || defined(__arm__) || defined(_ARM64_) ||
defined(__aarch64__) */
+ env->_Fe_ctl = fenv_encode(0, _controlfp(0, 0));
+ env->_Fe_stat = fenv_encode(0, _statusfp());
This bit feels a bit asymetrical; in fesetenv we use our own
__mingw_setfp for setting things, but in fegetenv we just use
_controlfp() and _statusfp() from the CRT. (This is also a bit of a
functional change as we've been entirely detached from the CRT's
handling of this bit so far.)
It's probably fine though, but I'm wondering if this would be clearer
if we'd have our own __mingw_controlfp/__mingw_setupfp at first for
ARM/AArch64 too, so we'd have the full loop of both setting and
getting in our own code, just like before. Then we could convert
__mingw_controlfp into a call to the CRT's _controlfp in a separate step.
I will change it.
It doesn't make this patch smaller or easier, but it makes it
marginally easier to grasp the full picture. The separate step of
taking CRT's _controlfp and _statusfp into use would then be easier to
consider on its own. I presume the UCRT's _controlfp/_statusfp works
as it should, but e.g. for msvcrt.dll I'm not sure if anybody has
tested them? (I did testrun these patches with both UCRT and msvcrt on
all 4 architectures, and it does seem to run fine.)
I'd assume so, although Wine testing on ARM is admittedly much less
widespread than on x86. The main source of weird things the fact that
x86 has two separate ways of dealing with floats, ARM doesn't have this
problem.
Aside from that, I think this mostly looks good. Most of the fe*
function are kinda straightforward and everything boils down to calls
to the internal helper functions. As this is based on Wine with quite
well tested implementations, I'm fairly certain that those aspects are ok.
Thanks for the review!
Jacek
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
