Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

math: add LDC restritions on usage of x87 extensions #8323

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljmf00
Copy link
Member

@ljmf00 ljmf00 commented Nov 28, 2021

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 ljmf00 requested a review from ibuclaw as a code owner November 28, 2021 19:12
@dlang-bot
Copy link
Contributor

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"

This patch adds some restrictions if the library is compiled with LDC compiler
targeting MSVC C runtime.

Partially cherry picked from: https://github.com/ldc-developers/phobos

Co-authored-by: Martin Kinkelin <[email protected]>
Signed-off-by: Luís Ferreira <[email protected]>
@kinke
Copy link
Contributor

kinke commented Nov 29, 2021

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.

@ljmf00
Copy link
Member Author

ljmf00 commented Nov 29, 2021

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?

@kinke
Copy link
Contributor

kinke commented Nov 29, 2021

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
Copy link
Contributor

kinke commented Nov 29, 2021

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.

@ljmf00
Copy link
Member Author

ljmf00 commented Dec 10, 2021

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 ljmf00 marked this pull request as draft January 7, 2022 13:00
@ljmf00
Copy link
Member Author

ljmf00 commented Jan 7, 2022

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.

@LightBender LightBender added the Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
@LightBender
Copy link
Contributor

This related to: #7668
Adding @thewilsonator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:Blocked Merge:stalled Review:Salvage This PR needs work, but we want to keep it and it can be salvaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants