Skip to content

Comments

fix Issue 19499 - __c_long_double has exact match with double#9117

Merged
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:fix19499
Dec 28, 2018
Merged

fix Issue 19499 - __c_long_double has exact match with double#9117
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:fix19499

Conversation

@WalterBright
Copy link
Member

This reverts #8632 which was an attempt to fix https://issues.dlang.org/show_bug.cgi?id=19201 but dmd source code should be fixed instead of changing the language semantics.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
19499 regression __c_long_double has exact match with double

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9117"

@WalterBright WalterBright added the Severity:Regression PRs that fix regressions label Dec 22, 2018
@WalterBright WalterBright requested a review from ibuclaw December 22, 2018 08:26
@thewilsonator
Copy link
Contributor

Target stable.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 22, 2018

This will cause a bootstrap build regression.

Without checking, first point of call for changing dmd would be the sinteger_t aliases.

alias sinteger_t = int64_t;
alias uinteger_t = uint64_t;

So there wouldn't be any implicit D long <-> C long conversions in the first place.

Can work backwards from there to find less breaking ways to fix dmd source.

@WalterBright
Copy link
Member Author

You listed about 4 places in dmd source, why not just insert a cast in those places?

@ibuclaw
Copy link
Member

ibuclaw commented Dec 25, 2018

Looks like druntime has mitigated this, so no longer reproducible on Linux (dlang/druntime#2302). But will still fail to build on OSX.

You listed about 4 places in dmd source, why not just insert a cast in those places?

There are explicit casts in all linked places, the kinds of errors I was seeing:

gcc/d/dmd/dcast.d:381:52: error: cannot cast expression ‘cast(long)value’ of type ‘long’ to ‘longdouble’
                 const f = cast(T) cast(sinteger_t) value;
                                                    ^
gcc/d/dmd/dcast.d:459:22: error: template instance ‘dmd.dcast.implicitConvTo.ImplicitConvTo.visit.isLosslesslyConvertibleToFP!(longdouble)’ error instantiating
                 if (!isLosslesslyConvertibleToFP!real_t)
                      ^

Casts would mean adding overrides where implicit long -> int64_t conversion is rejected.

static if (!is(int32_t == int))
    void set()(int d) { return this.set(cast(int32_t)d); }
static if (!is(uint32_t == uint))
    void set()(uint d) { return this.set(cast(uint32_t)d); }
static if (!is(int64_t == long))
    void set()(long d) { return this.set(cast(int64_t)d); }
static if (!is(uint64_t == ulong))
    void set()(ulong d) { return this.set(cast(uint64_t)d); }

It seems odd however that implicit conversion may work on some, but fail on other host systems depending on what druntime bindings are aliasing today.

@WalterBright
Copy link
Member Author

@ibuclaw I don't really understand your comment. What I need is can you fix the dmd code to fix the bootstrap errors you're seeing? Can you submit a PR for that?

@ibuclaw
Copy link
Member

ibuclaw commented Dec 25, 2018

The bootstrap errors are no longer reproducible since druntime changed aliases of int64_t and uint64_t in dlang/druntime#2302.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 25, 2018

The bootstrap errors are in places where there's already appropriate casting.

f = cast(longdouble) cast(long) value;

The errors are local to the longdouble implementation which only has overrides for int64_t/uint64_t, not long/ulong.

What I posted is what adding long and ulong overrides would look like in gdc's longdouble. Which just looks silly.

@WalterBright
Copy link
Member Author

It doesn't matter if it looks silly in the dmd source code. What does matter is adding an "exact" match of __c_long_double with double, which will cause no end of grief.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 28, 2018

Well I am not happy that conversely, having long and int64_t not match will cause the same level of grief.

This isn't just compiling dmd itself, but anyone interfacing with a C++ library that uses int64_t/uint64_t in its public API.

@dlang-bot dlang-bot merged commit 7581ed9 into dlang:master Dec 28, 2018
@WalterBright
Copy link
Member Author

Well I am not happy that conversely, having long and int64_t not match will cause the same level of grief. This isn't just compiling dmd itself, but anyone interfacing with a C++ library that uses int64_t/uint64_t in its public API.

This issue affects C++ as well, because it affects C++ name mangling. Different integer types with the same size can have different mangling. D code should use core.stdc.stdint's definitions of int64_t and uint64_t to match the C++ mangling.

@WalterBright WalterBright deleted the fix19499 branch December 28, 2018 21:12
@ibuclaw
Copy link
Member

ibuclaw commented Dec 29, 2018

Well it's no good if getting the correct C++ mangling means making code that uses it uncompilable.

import core.stdc.stdint;

void func(uint64_t r);
void func(int64_t r);

void main()
{
    func(0L);
}

It is not ambiguous here as to which overload should be called. if the special enum types don't solve this, then there is no point to having them in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants