Skip to content

use C mangling for unsigned long long for now#7523

Merged
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:C-mangling
Dec 27, 2017
Merged

use C mangling for unsigned long long for now#7523
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:C-mangling

Conversation

@WalterBright
Copy link
Member

Because of mangling problems on OSX for unsigned long long when using a bootstrap dmd, we just use C mangling for now.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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.

@JinShil
Copy link
Contributor

JinShil commented Dec 26, 2017

I don't like where this is going. First with #7522 and now this, we have workarounds and temporary fixes to track and maintain. I'd like to see a more disciplined approach to dealing with the core problem.

If we need a multi-step implementation, let's document it somewhere with specific tasks to be performed and specific dates on which to perform them.

I suggest closing this, #7522, & #7519, and re-opening #7505 to revert the cause of this debacle until a more disciplined development plan is devised and documented.

@WalterBright
Copy link
Member Author

I'm not concerned about workarounds, because the back end will be converted to D soon and the whole problem will go away. The correct fix is already in - using uint64_t/int64_t, which is a principled approach.

@jacob-carlborg
Copy link
Contributor

I'm not concerned about workarounds, because the back end will be converted to D soon and the whole problem will go away.

That might solve it for DMD but I think the it needs to be possible to use long, int and c_long in a C++ function if we want to support C++ linkage.

@WalterBright
Copy link
Member Author

it needs to be possible to use long, int and c_long in a C++ function if we want to support C++ linkage.

That's outside the scope of this PR.

@WalterBright WalterBright force-pushed the C-mangling branch 7 times, most recently from 4580ef9 to b6d857d Compare December 26, 2017 10:07
q[1] = cast(ubyte)v;
p += 2;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Undefined symbols for architecture x86_64:
  "Outbuffer::writeShort(int)", referenced from:
      Outbuffer::writeChar(int) in dmd.o

@WalterBright WalterBright force-pushed the C-mangling branch 15 times, most recently from 99b92c0 to ca95468 Compare December 27, 2017 03:04
@WalterBright WalterBright force-pushed the C-mangling branch 3 times, most recently from 16fb9ca to 0831a3f Compare December 27, 2017 07:56
@wilzbach
Copy link
Contributor

Seems like ispow also needs a changed mangling:

Undefined symbols for architecture x86_64:
  "ispow2(unsigned long long)", referenced from:
      asmSemantic(AsmStatement*, Scope*) in dmd.o
  "DtBuilder::size(unsigned long long)", referenced from:
      Initializer_toDt::InitToDt::visit(ArrayInitializer*) in dmd.o
      Expression_toDt::ExpToDt::visit(StringExp*) in dmd.o
      Expression_toDt::ExpToDt::visit(ArrayLiteralExp*) in dmd.o
      cpp_type_info_ptr_toDt(ClassDeclaration*, DtBuilder*) in dmd.o
      _D3dmd4todt11membersToDtFCQy9aggregate20AggregateDeclarationCQCh7backend2dt9DtBuilderPSQDh4root5array__T5ArrayTCQEg10expression10ExpressionZQBkmCQFn6dclass16ClassDeclarationPPPSQGtQBg9BaseClassZv in dmd.o
      TypeInfoDtVisitor::visit(TypeInfoDeclaration*) in dmd.o
      TypeInfoDtVisitor::visit(TypeInfoConstDeclaration*) in dmd.o
      ...
ld: symbol(s) not found for architecture x86_64
clang: error: linker 

@WalterBright WalterBright force-pushed the C-mangling branch 2 times, most recently from 46cf455 to 400a7d9 Compare December 27, 2017 10:27
@WalterBright
Copy link
Member Author

ispow2 is covered by #7522

@WalterBright
Copy link
Member Author

@wilzbach this is ready to pull now.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I am not happy about this either, but I really don't like that our CIs (Travis in this case) are broken. If the conversion is really that imminent, we will get rid of most hacks anyways.

void abytes(tym_t ty, uint offset, uint size, const(char)* ptr, uint nzeros);
void abytes(uint offset, uint size, const(char)* ptr, uint nzeros);
void dword(int value);
version (OSX)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this is a clang issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, g++ on OSX behaves the same.

@dlang-bot dlang-bot merged commit 17bfbec into dlang:master Dec 27, 2017
@WalterBright WalterBright deleted the C-mangling branch December 28, 2017 00:21
@kinke
Copy link
Contributor

kinke commented Apr 8, 2018

I'm a bit late here, but problems just popped up now when finalizing LDC's upgrade to 2.079.

I agree with the previous concerns about this whole mangling change. As it stands now, a C++ size_t and ptrdiff_t ([unsigned] long) cannot be represented in D on 64-bit OSX at all anymore (no, core.stdc.config.c[pp]_[u]long don't work as they should), clearly a huge regression, as a D size_t/ptrdiff_t used to map to the C++ one.

@jacob-carlborg
Copy link
Contributor

For Posix cpp_long/cpp_ulong only exist on 32bit.

@kinke
Copy link
Contributor

kinke commented Apr 8, 2018

[That's what I meant, without going into too many details.]

What this size_t incompatibility means in practice is that basically there should be no occurrence of size_t in the front-end's C++ headers anymore (at least in function signatures); they would all need to be migrated to a d_size_t. And that's the simple case where the implementations are in D and the C++ headers can be adjusted. The opposite case, interfacing with external C++ functions taking or returning size_t's, is currently only feasible by overriding the D mangle manually, which is... pretty bad.

@WalterBright
Copy link
Member Author

I wonder if this can work:

enum cpp_long = long;

and then handle it specially in the C++ name mangler.

@jacob-carlborg
Copy link
Contributor

I wonder if this can work:
enum cpp_long = long;
and then handle it specially in the C++ name mangler.

No, I don’t think that’s legal D code. I think enums require a value on the right side. cpp_long needs to be added to druntime for Posix 64bit just as it already exists for 32bit.

@kinke
Copy link
Contributor

kinke commented Apr 9, 2018

cpp_long needs to be added to druntime for Posix 64bit just as it already exists for 32bit.

That's still not a nice solution, as e.g. ints aren't implicitly converted to a cpp_[u]long struct => foo(ptr, cpp_ulong(123)). That wouldn't be a big deal if it was seldomly used, but a size_t is ubiquitous.
I think the mangling change should be reverted and druntime get a new magic struct for C++ long long mangling (cpp_[u]longlong). A new alias, something like core.stdc.config.[u]int64_t, would alias that magic struct type on 64-bit OSX (and usually alias D [u]long directly on other target platforms).

@jacob-carlborg
Copy link
Contributor

That's still not a nice solution, as e.g. ints aren't implicitly converted to a cpp_[u]long struct => foo(ptr, cpp_ulong(123)). That wouldn't be a big deal if it was seldomly used, but a size_t is ubiquitous.

It's not just ints that are not implicitly converted, it won't work for any numeric type since D doesn't have implicit constructions of structs.

@jacob-carlborg
Copy link
Contributor

I tried adding support for specifying a mangling on an alias #7458 but I ran into some issues with type comparisons.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Apr 9, 2018

I had another look at the mangling of numeric types in C++ and D. This table is the result:

Type C 64 bit DMD 64 bit C 32 bit DMD 32 bit
int (D/C++) i i i i
long (C++) l N/A l N/A
cpp_long (D) N/A N/A (l) N/A l
long long (C++) x N/A x N/A
long (D) N/A x N/A x
int64_t (D/C++) x x x x
uint (D/C++) j j j j
u long (C++) m N/A m N/A
cpp_ulong (D) N/A N/A (m) N/A m
u long long (C++) y N/A y N/A
ulong (D) N/A y N/A y
uint64_t (D/C++) y y y y
size_t (D/C++) m y m j

Giving that result, this is how we should map C++ types in D:

C++ Type D Type
int int
long cpp_long
long long long
int64_t int64_t
u int uint
u long cpp_ulong
u long long ulong
uint64_t uint64_t
size_t cpp_ulong

With the current implementation, what is missing is cpp_long/cpp_ulong on 64bit. size_t should also be aliased to cpp_ulong. In my opinion it would be better if we could add a mangle on an alias.

@kinke
Copy link
Contributor

kinke commented Apr 9, 2018

It's probably not that simple as you seem to have tested on macOS only. 32-bit macOS seems to map size_t to unsigned long, not unsigned int as on Linux (so size_t D/C++ interop has already been broken on 32-bit macOS before the recent mangling change). So mapping size_t to cpp_ulong would only work on macOS for both 32/64-bit variants; on Linux, it's fine the way it is now (unsigned int for 32-bit, unsigned long for 64-bit).

In my opinion it would be better if we could add a mangle on an alias.

It'd be nicer than the magic structs with the implicit conversion problem, but how do you specify a C++-only mangle? A new pragma? I'm against touching D mangling here, I don't want D to repeat the C++ mistakes with ambiguous integer types...

@jacob-carlborg
Copy link
Contributor

It's probably not that simple as you seem to have tested on macOS only. 32-bit macOS seems to map size_t to unsigned long, not unsigned int as on Linux (so size_t D/C++ interop has already been broken on 32-bit macOS before the recent mangling change). So mapping size_t to cpp_ulong would only work on macOS for both 32/64-bit variants; on Linux, it's fine the way it is now (unsigned int for 32-bit, unsigned long for 64-bit).

The definition of size_t would look different on macOS and the rest of the Posix platforms. But since one would be a struct and the other a built-in type, it would be a bit problematic.

It'd be nicer than the magic structs with the implicit conversion problem, but how do you specify a C++-only mangle? A new pragma? I'm against touching D mangling here, I don't want D to repeat the C++ mistakes with ambiguous integer types...

When I originally had that idea it was as a more generic solution to the existing __c_long struct. cpp_long would be an alias to long/int with a C++ specific mangling. It would only be used when interfacing with C++. The same could be done with size_t, i.e. cpp_size_t. This would solve the issue with D's lack of implicit construction of structs. The underlying type of size_t and cpp_size_t would be the same.

@kinke
Copy link
Contributor

kinke commented Apr 9, 2018

I find having to use import core.stdc.config : cpp_size_t and changing all size_t's to cpp_size_t inacceptable. The D size_t alias has always been working fine (except for 32-bit macOS, but luckily hardly anyone seems to care about that platform anymore) until the recent mangling change, on 64-bit macOS, 32/64-bit Linux/Solaris/..., 32/64-bit Windows, so I consider any more work from the user to be a huge regression.

Use LDC as an example for a non-trivial mixed D/C++ code base - who wants to replace all size_t's in the DMD frontend C++ headers to a d_size_t, or replace most size_t's in the D files by cpp_size_t?

@jacob-carlborg
Copy link
Contributor

It’s not ideal, I’m trying to come up with a middle ground.

@WalterBright
Copy link
Member Author

See #8152

@kinke
Copy link
Contributor

kinke commented Apr 9, 2018

I've experimentally added cpp_(u)long(long) and cpp_(u)int64_t for LDC, switched back to the previous mangling scheme and am now looking forward to the CI results.

The expected result is that C++

uint64_t testlongmangle(int a, unsigned int b, int64_t c, uint64_t d);
unsigned long testCppLongMangle(long a, unsigned long b);
unsigned long long testCppLongLongMangle(long long a, unsigned long long b);
size_t testCppSizeTMangle(ptrdiff_t a, size_t b);

and D

extern(C++):
cpp_uint64_t testlongmangle(int a, uint b, cpp_int64_t c, cpp_uint64_t d);
cpp_ulong testCppLongMangle(cpp_long a, cpp_ulong b);
cpp_ulonglong testCppLongLongMangle(cpp_longlong a, cpp_ulonglong b);
size_t testCppSizeTMangle(ptrdiff_t a, size_t b);

are mangled identically, i.e., that firstly all C++ integer types can be represented in D (on all platforms) and that secondly ptrdiff_t and size_t match up (excl. 32-bit macOS).

Thx Walter for trying to fix the implicit conversion issue.

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