phobos
phobos copied to clipboard
math: add LDC restritions on usage of x87 extensions
This patch adds some restrictions if the library is compiled with LDC compiler targeting MSVC C runtime.
Partially cherry picked from: ldc-developers/phobos
We need this if we want to go forward with compiling and running the test suite with sanitization and heuristic-based fuzzing.
CC @kinke
Thanks for your pull request, @ljmf00!
Bugzilla references
Your PR doesn't reference any Bugzilla issue.
If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub run digger -- build "master + phobos#8323"
We need this if we want to go forward with compiling and running the test suite with sanitization and heuristic-based fuzzing.
More details please, and why upstreaming some ugliness from LDC is needed here.
This could simpler when moving away from version
to enum
+ static if
- as x87 can be detected via real.mant_dig == 64
. Not sure what that would mean in terms of compilation speed etc.
We need this if we want to go forward with compiling and running the test suite with sanitization and heuristic-based fuzzing.
More details please, and why upstreaming some ugliness from LDC is needed here.
I have talked about this in a forum thread, maybe we can discuss this more in depth, in a separate one.
The idea would be to add the possibility of fuzzing the official standard library or at least making it simpler. We don't have anything set up on Google oss-fuzz and we should start worrying about it. If we want to have a secure implementation, we should not only fuzz the official standard library but also run the test suite against ASAN/UBSAN, and this is a step forward towards that.
This could simpler when moving away from
version
toenum
+static if
- as x87 can be detected viareal.mant_dig == 64
. Not sure what that would mean in terms of compilation speed etc.
Is the behaviour of real.mant_dig == 64
specified as such, or implementation-defined? I just ported the code from LDC. Can you clarify what to improve here? CRuntime_Microsoft
and Android
conditions are only specific to LDC, right?
I have talked about this in a forum thread, maybe we can discuss this more in depth, in a separate one.
I'm not sure what the goal is - getting upstream Phobos here to be usable and testable with the current LDC and LDC's druntime version? That's a much bigger task. I've been hesitant to upstream pure LDC specifics; stuff that is also interesting for GDC is a different topic. [And much of that has only recently been enabled, by LDC supporting the GDC/GCC inline asm syntax.] druntime is particularly tricky due to the tight compiler coupling, and LDC not being in sync with DMD master (and that's utopic - people won't work on a new feature if that requires glue layer changes in multiple compilers to be mergable).
Is the behaviour of
real.mant_dig == 64
specified as such, or implementation-defined? I just ported the code from LDC. Can you clarify what to improve here?CRuntime_Microsoft
andAndroid
conditions are only specific to LDC, right?
We currently have 3/4 real
formats - double, x87 extended, quad and, AFAIK to some very limited extent, 128-bit IBM 'doubledouble'. DMD only supports x87 for all of its targets; LDC and GDC the other ones. In particular, LDC uses 64-bit double precision for MSVC targets, just like the MSVC itself (C long double
is a differently mangled double
), while DMD sticks with x87. So that explains the CRuntime_Microsoft
special case for LDC here. Android is a mess, it uses double for 32-bit x86, and a software quad precision real
for 64-bit x86 (i.e. the same formats as the ARM architecture with corresponding bitness).
As for real.mant_dig == 64
, x87 is the only format with a 64-bit mantissa at the moment, and I bet it stays that way for a while.
Can you clarify what to improve here?
In case I misunderstood - the ugliness comes from defining extra helper version
s requiring knowledge about the real
formats for Android, MSVC etc., while the x87 check would be a simple static if (real_mant.dig == 64)
. The problem is that it's all version-based at the moment, and you cannot define a version inside a static-if:
static if (real.mant_dig == 64)
version = abc; // Error: version `abc` defined after use
version (abc) void foo() {}
So that would require moving from versions to private enums and static-ifs.
Can you clarify what to improve here?
In case I misunderstood - the ugliness comes from defining extra helper
version
s requiring knowledge about thereal
formats for Android, MSVC etc., while the x87 check would be a simplestatic if (real_mant.dig == 64)
. The problem is that it's all version-based at the moment, and you cannot define a version inside a static-if:static if (real.mant_dig == 64) version = abc; // Error: version `abc` defined after use version (abc) void foo() {}
So that would require moving from versions to private enums and static-ifs.
Oh, I understand. Is there any compiler issue about it I can reference here, or is this supposed to be the intended behaviour and perhaps a language limitation? If it is a compiler error, I can stale this and try to fix it.
I marked this as draft due to https://issues.dlang.org/show_bug.cgi?id=7386 . After that we should be able to check the mantissa length and set the version without workarounds. Feel free to set this with blocked label, I can't on Phobos.