Skip to content

Conversation

@WalterBright
Copy link
Member

…gling D long/ulong

I'm tired of expending many hours dealing with this issue again and again. Fortunately, it only changes the mangling on OSX 64.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Description
18053 Use stdint.h mangling for int64_t/uint64_t when mangling D long/ulong

@WalterBright
Copy link
Member Author

Note how much nicer the test cases get :-)

@WalterBright
Copy link
Member Author

@ibuclaw @klickverbot please check effect on gdc/ldc

case Tint64:
c = (Target.c_longsize == 8 ? 'l' : 'x');
// Mangle like target's stdint.h int64_t
c = (Target.c_longsize == 4 || global.params.isOSX ? 'x' : 'l');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be turned into a target hook. The frontend should not be asking questions such as "Am I targeting osx?". I think we already have a mangle type hook already present, the logic can be put there.

Copy link
Member

@ibuclaw ibuclaw Dec 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dmd/src/ddmd/target.d

Lines 459 to 465 in 5494dba

* For a vendor-specific type, return a string containing the C++ mangling.
* In all other cases, return null.
*/
extern (C++) static const(char)* cppTypeMangle(Type t)
{
return null;
}

Comment would need to be updated to reflect that if the target requires mangling in a specific way, then handle it here.

@jacob-carlborg
Copy link
Contributor

I'm not sure which C++ compiler you're looking at but these are the results I got with Xcode 9.0 containing Clang 900.0.37:

64bit:

Type C++ D D After PR
long l l x
long long x l x
int64_t x l x
unsigned long m m y
unsigned long long y m y
uint64_t y m y

32bit:

Type C++ D D After PR
long l i i
long long x x x
int64_t x x x
unsigned long m j j
unsigned long long y y y
uint64_t y y y

As can be seen in these tables, D uses the wrong mangling for the long type.

I've used the following C++ code:

#include <stdint.h>

void a(long){}
void b(long long){}
void c(int64_t){}

void d(unsigned long){}
void e(unsigned long long){}
void f(uint64_t){}

And the following D code:

extern (C++):

import core.stdc.config;
import core.stdc.stdint;

void a(c_long){}
void b(long){}
void c(int64_t){}

void d(c_ulong){}
void e(ulong){}
void f(uint64_t){}

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Dec 10, 2017

Hmm, seems like __c_long/__c_ulong needs to be used on macOS 64bit as well and not only 32bit. Shouldn't cpp_long/cpp_ulong exist on both 32bit and 64bit architectures regardless which platform?

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Dec 10, 2017

BTW, why this complicated logic with a struct with a specific name that matches the mangling of C++? Couldn't we make the following to work?

pragma(mangle, "m") alias c_ulong = ulong;

That would also avoid the need to explicitly instantiate the struct, since D doesn't have support for implicit conversions in this case. The above solution is more generic and nothing in the compiler needs to be hard coded like it is now.

@WalterBright
Copy link
Member Author

D uses the wrong mangling for the long type.

There is no right mangling for the long type, because the C long may be 32 bits or 64 bits.

The above solution is more generic and nothing in the compiler needs to be hard coded like it is now.

I'd welcome a better solution for that. But not in this PR.

@WalterBright WalterBright dismissed ibuclaw’s stale review December 11, 2017 00:59

Implemented Target.int64Mangle and Target.uint64Mangle

@jacob-carlborg
Copy link
Contributor

I'd welcome a better solution for that. But not in this PR.

I have something that is a work in progress.

@JinShil
Copy link
Contributor

JinShil commented Dec 23, 2017

@WalterBright Can you please rebase?

@dlang-bot dlang-bot merged commit 961c7a1 into dlang:master Dec 24, 2017
@wilzbach
Copy link
Contributor

This broke the OSX build on Travis, which shows the problems @jacob-carlborg pointed out. Should we revert, s.t. the error can be properly fixed/worked out?

@jacob-carlborg
Copy link
Contributor

@wilzbach I have an alternative solution to the existing __c_long available here #7458. It adds support for specifying pragma(mangle) on aliases. Hopefully with this PR together with mine we could get the correct mangling without breaking any existing code.

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.

6 participants