core.stdc.math: proper support for Microsoft C Runtime (Visual Studio >= 2013) #968

kinke · 2014-09-27T16:43:20Z

These commits add proper support for Microsoft C runtimes to core.stdc.math. It is especially useful in combination with MSVCRT v12 (VS 2013) with its greatly improved C99 support (30+ new math functions, in single and double precision).
Microsoft's lacking 80-bit x87 support is approximated by 64-bit double-precision wrappers.

yebblies · 2014-09-27T16:45:51Z

I doubt requiring MSVC >= 2014 is reasonable.

kinke · 2014-09-27T16:57:08Z

The MSVC >= 2014 requirement is only relevant for the fpclassify() family (it may even work with VS 2013, I must check this yet). The other 2 commits just provide wrapper implementations for functions which aren't available in any Microsoft runtime version anyway (so all they do is get rid of annoying linking errors).
Given the current state of VCRT support in both druntime and Phobos, I hope requiring MSVC >= 2014 is not too much to ask for in the short term. The guys in Redmond have worked further on their C99 support, and imo this now provides a very good opportunity for us to substantially improve the VCRT support in druntime.

/edit: Okay, the requirement can be dropped to MSVC 2013 as the fpclassify() family is implemented identically (not sure since when exactly). If MS has added some implementations in 2014, there may still be a few unresolved externals when linking to the 2013 runtime; but MSVC 2013 has brought most of the "new" ;) C99 functions, see this MSDN blog.

WalterBright · 2014-09-28T06:48:47Z

src/core/stdc/math.d

+    //int isfinite(real-floating x);
+    int isfinite(float x)       { return fpclassify(x) <= 0; }
+    int isfinite(double x)      { return fpclassify(x) <= 0; }
+    int isfinite(real x)        { return fpclassify(x) <= 0; }


Microsoft's "real" is not the same as D's "real". Use c_long_double instead.

Hmm that'd be a bigger change and, if done consistently, means replacing all real occurrences in all C header wrappers:

complex.d

float_.d

math.d

stdlib.d

wchar_.d

If you think that should be part of this pull request, I'll do it.

I agree with @kinke here, there should be a proper strategy using c_long_double, but not to be decided in this PR. Right now, it would not be used, because you have to add it explicitely at the call site. If there is implicite conversion, it would probably introduce ambiguities.
The problem here is not mangling, but overload resolution for real.

Okay so how about this as work-around:

-real acoshl(real x); +extern(D) real acoshl(real x) { return acosh(cast(double) x); }

etc., i.e., adding a dedicated *l wrapper working with proper D reals and using D name mangling so as not to collide with the (hidden) C function in MSVCRT.
All MSVCRT *l functions working with their 64-bit C long double are completely useless for us as long as D real remains defined as 80-bit on Windows x64 and including them in druntime would only make sense if we relaxed it to 64-bit.
The proposed D wrappers would at least feature the expected signature for the time being.

Adding the wrappers is what I would do, too. It is what's happening in the C-library anyway.
I just tried what happens if you pass a double to acoshl(real) with "-O -inline": there is no trace of a conversion double->real->double left.

Okay, I've just added a commit for these wrappers. This also means that one could safely relax real to 64-bit on Win64 without having to touch core.stdc.math.

rainers · 2014-10-07T06:31:32Z

I also think that requiring VS2013 or even VS2014 is possible. I suspect Walter is using VS2010, and the build server uses VS2012.

rainers · 2014-10-07T06:44:51Z

src/core/stdc/math.d

+        FP_NORMAL    = -1,
+        FP_ZERO      =  0,
+        FP_INFINITE  =  1,
+        FP_NAN       =  2,


Please don't remove existing functionality. The enumerators and the following functions still exist in VS2013, I guess they will be kept by MS.

Hi Rainer, I'd prefer getting rid of most existing MS workarounds (I'm not talking about Windows, but the - at least until now - crappy C99 support in their runtime) bloating druntime and forcing D developers to code in a portable way - in this case, by using the standard C99 functions if not something from Phobos.

People might be using this stuff, so we would have to at least deprecate it for some time.
I'd prefer the Microsoft way here, though: leave it in, it doesn't really hurt.

Okay I've edited the original commit to preserve that legacy stuff (in a version(all) block with an appropriate comment).

kinke · 2014-10-12T12:45:49Z

The wrappers introduced in the last commit unveil the current state wrt. core.stdc.math and MSVCRT < 2013:

37 unresolved externals
of which only 4, _(f)dclass and _(f)dsign, are due to my changes
33 unresolved symbols are regular C99 math functions, i.e., functions which have been added in MSVCRT 2013/2014

These 33 missing functions are currently (druntime head) imported/declared and obviously keen on leading to linking errors as soon as you dare attempt to use them. And these 33 don't even include the *l functions, which are each either not part of the C runtime (more linking errors) or imported with mismatching signature (80-bit D real vs. 64-bit MS C long double), and the *f functions neither, most of which are also missing. So we're talking about 100+ potential linking errors with MS runtimes < 2013.
That's why I think this PR is important and why we should support MSVCRT 2013+ only.

kinke · 2015-02-01T01:39:48Z

Common people, it's been 4 months now. Continuing to neglect the MSVCRT support, essential on Win64 (still hugely popular in the real world!) for both DMD and LDC, is really not gonna help D breaking through.

VS 2015 is soon to be released and VS 2013 is offered as free Community edition (not cut down like the previous Express editions). I see no point in continuing the current way of half-heartedly supporting older MS C runtimes. Instead, I strongly recommend requiring VS 2013+, updating the build bots accordingly and considering this and my other PRs (needless to say, all pending too):

I'll rebase once I see some actual commitment; you can't seriously expect contributors to keep rebasing while PRs simply sit there uncommented for months.

rainers · 2015-02-01T09:58:26Z

Sorry for the long silence, but I guess we are conservative around here regarding backward compatibility. I think the goal should be to not break linking with VS 2010 or VS2012, but add support for C99, too.

AFAICT the float versions are in the MS runtime since VS 2005 or longer, so there is no problem adding these. Why do you version some of these on Win32/Win64?
You've added wrappers for the real-versions, I think that's good.

So the problematic cases are the functions needing fpclassify: how about just reimplementing this function here. That could also support reals without conversion. (isnan on real produces a runtime error with this PR, I don't think that is acceptable).

rainers · 2015-02-01T10:23:00Z

src/core/stdc/math.d

+    private int _fdsign(float x);
+    private int _dsign(double x);
+
+  deprecated("Please use the standard C99 functions instead.")


I recommend emitting more specific deprecation messages here, but just for functions that have a backward compatible alternative. IMO just a comment would be ok, too.

kinke · 2015-02-01T12:09:31Z

Sorry for the long silence, but I guess we are conservative around here regarding backward compatibility. I think the goal should be to not break linking with VS 2010 or VS2012, but add support for C99, too.

Well in my opinion there's no backward compatibility. While one is currently able to build druntime with these older runtimes, there's a potential of 100+ undefined symbols, and worse, functions with wrongly imported signature (all *l versions) when trying to actually use C math functions.
If you're really keen on keeping this form of support, we'd need to discriminate between different MSVCRT versions. This would obviously bloat and clutter druntime, and as far as I've seen so far, we don't do this for any other C runtime. Instead, we require a minimum version. The long overdue but finally arriving and nearly complete C99 support by MS provides a great opportunity for us to elevate Windows to a first-class citizen/platform for D. The first step would be requiring MSVCRT 2013+. I don't imagine this to be a big hassle for D devs targeting Win64 -- that platform has been a bumpy ride for D from the start.

AFAICT the float versions are in the MS runtime since VS 2005 or longer, so there is no problem adding these. Why do you version some of these on Win32/Win64?

MS provides the *f versions of the math functions the runtime supports, i.e., there are 33+ *f functions missing in MSCVRT < 2013. Some are defined inline in the C header and aren't exported by the DLL, so that's why some need to be implemented here in druntime.
For some bizarre reason, some *f functions are only exported by the 64-bit runtime and declared inline for 32-bit. My PR here is solely based on Microsoft's C header. And I wanted to support the 32-bit runtime too.

So the problematic cases are the functions needing fpclassify: how about just reimplementing this function here. That could also support reals without conversion. (isnan on real produces a runtime error with this PR, I don't think that is acceptable).

That would probably mean duplicating code from Phobos' std.math, I'm not sure that's a good idea. LDC on Win64 defines real as 64-bit double, so this would only be necessary for DMD and of course only if you guys really don't wanna drop the current form of support for older MS runtimes.

rainers · 2015-02-01T21:25:24Z

I've tried declaring all 'f' functions without body and produced a main calling all functions.
This links with VS 2013, both Win32 and Win64.
It fails for VS2012 and earlier, with 65 unresolved symbols for Win64 and 68 for Win32. I think that is acceptable.

rainers · 2015-02-01T21:27:11Z

src/core/stdc/math.d

+    extern(D)      real   fabsl(real   x) { return fabs(cast(double) x); }
+
+    private        float  _hypotf(float x, float  y);
+                   double hypot (double x, double y);


You should map the double version of hypot to _hypot, too. Otherwise linking against the DLL import library msvcrt.lib fails.

Yeah, iirc hypot() has been introduced with MSVCRT 14. I'll change it.

kinke · 2015-02-02T09:05:28Z

If all *f functions are really exported by both x86 and x64 runtimes, that is really good news and will simplify the PR. Thanks for checking!

MartinNowak · 2015-02-02T11:39:55Z

You could probably emit weakly linked shims to support older runtimes.

kinke · 2015-02-03T23:52:25Z

Alright, thanks for your suggestions, I've tried to incorporate all of them in the reworked commits. I think we have a reasonable solution now.

Orvid · 2015-02-04T00:01:26Z

I am one of those who would really prefer if we don't require the 2013 runtime, as my build scripts all use 2010, but, as long as this is just adding the APIs that are supported in >2010 in a way that doesn't break 2010, then I have no problem with this PR.

MartinNowak · 2015-02-04T00:28:09Z

You could probably emit weakly linked shims to support older runtimes.

What about the weak link idea?
Could we add the functions like so.

// creates a weakly linked function that forwards to impl
template forward(string name, alias impl)
{
    static if (is(typeof(impl) RT == return) && is(typeof(impl) Args == function))
    {
        pragma(mangle, name) RT forward(Args args) { return impl(args); }
    }
    else
        static assert(0);
}

alias atanf = forward!("atanf", atan);
alias logf = forward!("logf", log);

kinke · 2015-02-04T00:47:22Z

@MartinNowak I've dummy-templatized the D wrappers (mostly *l versions for real) with similar weak-link effect. Your template lacks the downcasts of real arguments to double and may therefore produce many warnings. [And I doubt that it's perfectly suited for automated documentation purposes.]

@Orvid We should now have perfect backward compatibility with older runtimes. This now also applies to #969.

kinke · 2015-03-21T16:08:25Z

PING! It's been almost half a year now and I don't see any reason not to merge this.

redstar · 2015-03-22T11:57:51Z

I like to see this merged, too. It supports the Win64 port of LDC.

rainers · 2015-03-27T17:32:25Z

src/core/stdc/math.d

    enum
    {
-        ///


Rebasing seems to have removed these comments. I guess these have been added so the symbols are shown in the online documenttion.

This was actually intentional for these MS specific symbols. They haven't had these /// comments before 2.067, so I thought it'd be better to leave these non-standard declarations uncommented and only decorate the standard ones while rebasing.
I can add them if you insist. :)

Ok. I'm also not a huge fan of those empty comments. Let's consider this enumerator "internal".

Especially for MSVCRT 12+ (VS 2013).

By implementing functions defined inline in MS math.h header (from VS 2014 CTP3) and using D wrappers for all *l functions. hypot() forwards to _hypot() to support VS <= 2013.

rainers · 2015-04-04T16:28:49Z

I'm ready to merge this if there are no objections.

I tried building this against VS2015 (build 22310), but that failed due to some unrelated missing symbols (_set_output_format, _snprintf and a few more). Did you tried/managed to build against this version?

kinke · 2015-04-04T16:47:48Z

Yes I tried and managed (for LDC), see ldc-developers#17 and ldc-developers/phobos#6.
stdio.h has changed a lot, so backward compatibility isn't achievable without adding an additional version specification for the new runtime...

rainers · 2015-04-05T10:08:42Z

stdio.h has changed a lot, so backward compatibility isn't achievable without adding an additional version specification for the new runtime...

Ouch, looks like more trouble in the future. Maybe adding some stubs in a library to be added to the command line when linking against a VS runtime might work.

rainers · 2015-04-05T10:08:54Z

Auto-merge toggled on

core.stdc.math: proper support for Microsoft C Runtime (Visual Studio >= 2013)

rainers · 2015-04-05T10:10:46Z

Thanks for submission and bearing the slow review.

WalterBright reviewed Sep 28, 2014
View reviewed changes

kinke force-pushed the math branch from 6129ad6 to 2b6c867 Compare September 30, 2014 17:33

rainers reviewed Oct 7, 2014
View reviewed changes

kinke force-pushed the math branch from 2b6c867 to fd6222e Compare October 12, 2014 11:12

kinke changed the title ~~core.stdc.math: proper support for Microsoft C Runtime (Visual Studio >= 2014)~~ core.stdc.math: proper support for Microsoft C Runtime (Visual Studio >= 2013) Oct 12, 2014

kinke force-pushed the math branch 4 times, most recently from 575cffd to 59190e0 Compare October 12, 2014 11:52

This was referenced Oct 12, 2014

[WIP] Set real == double on Win64 MSVC. ldc-developers/ldc#747

Merged

core.stdc.math: full support for MSVCRT 2013+ ldc-developers/druntime#13

Merged

rainers reviewed Feb 1, 2015
View reviewed changes

kinke force-pushed the math branch from a488b54 to dbf10ea Compare February 1, 2015 19:15

rainers reviewed Feb 1, 2015
View reviewed changes

kinke force-pushed the math branch 2 times, most recently from a81ea80 to 91b51cc Compare February 3, 2015 23:13

kinke force-pushed the math branch from 91b51cc to b94e79b Compare February 3, 2015 23:50

kinke force-pushed the math branch from b94e79b to db0e80f Compare February 4, 2015 10:55

rainers reviewed Mar 27, 2015
View reviewed changes

kinke added 3 commits April 4, 2015 15:05

core.stdc.math: Update fpclassify() macros family for MSVCRT.

47e5962

Especially for MSVCRT 12+ (VS 2013).

core.stdc.math: Improve MSVCRT support, especially for VS 2013+.

2135e94

By implementing functions defined inline in MS math.h header (from VS 2014 CTP3) and using D wrappers for all *l functions. hypot() forwards to _hypot() to support VS <= 2013.

core.stdc.math: Work around linking errors with MSVCRT < 12 (VS 2013).

2a9cf76

kinke force-pushed the math branch from db0e80f to 2a9cf76 Compare April 4, 2015 13:06

rainers merged commit dd39631 into dlang:master Apr 5, 2015

kinke mentioned this pull request Apr 16, 2015

Cherry-pick some MSVCRT fixes which finally made it into upstream ldc-developers/druntime#26

Merged

core.stdc.math: proper support for Microsoft C Runtime (Visual Studio >= 2013) #968

core.stdc.math: proper support for Microsoft C Runtime (Visual Studio >= 2013) #968

Conversation

kinke commented Sep 27, 2014

yebblies commented Sep 27, 2014

kinke commented Sep 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Oct 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Oct 12, 2014

kinke commented Feb 1, 2015

rainers commented Feb 1, 2015

Choose a reason for hiding this comment

kinke commented Feb 1, 2015

rainers commented Feb 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Feb 2, 2015

MartinNowak commented Feb 2, 2015

kinke commented Feb 3, 2015

Orvid commented Feb 4, 2015

MartinNowak commented Feb 4, 2015

kinke commented Feb 4, 2015

kinke commented Mar 21, 2015

redstar commented Mar 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Apr 4, 2015

kinke commented Apr 4, 2015

rainers commented Apr 5, 2015

rainers commented Apr 5, 2015

rainers commented Apr 5, 2015