phobos icon indicating copy to clipboard operation
phobos copied to clipboard

math: add LDC restritions on usage of x87 extensions

Open ljmf00 opened this issue 2 years ago • 7 comments

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

ljmf00 avatar Nov 28 '21 19:11 ljmf00

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"

dlang-bot avatar Nov 28 '21 19:11 dlang-bot

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.

kinke avatar Nov 29 '21 18:11 kinke

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 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.

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?

ljmf00 avatar Nov 29 '21 19:11 ljmf00

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 and Android 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.

kinke avatar Nov 29 '21 19:11 kinke

Can you clarify what to improve here?

In case I misunderstood - the ugliness comes from defining extra helper versions 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.

kinke avatar Nov 29 '21 20:11 kinke

Can you clarify what to improve here?

In case I misunderstood - the ugliness comes from defining extra helper versions 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.

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.

ljmf00 avatar Dec 10 '21 17:12 ljmf00

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.

ljmf00 avatar Jan 07 '22 13:01 ljmf00