Skip to content

Comments

Revert mangling of D long back to C++ long on 64-bit OS X#8285

Merged
dlang-bot merged 2 commits intodlang:masterfrom
kinke:osx64_long
Jul 4, 2018
Merged

Revert mangling of D long back to C++ long on 64-bit OS X#8285
dlang-bot merged 2 commits intodlang:masterfrom
kinke:osx64_long

Conversation

@kinke
Copy link
Contributor

@kinke kinke commented May 23, 2018

As before 2.079, which allowed for direct interop of D size_t with C++ size_t on all platforms except for 32-bit OS X (which is about to reach end of life AFAIK).
The 2.079 mangling change to C++ long long allows for direct interop of D long with C++ int64_t on all platforms (which was already the case, with the notable 64-bit OS X exception). My argument is that (the now correctly defined) core.stdc.stdint.int64_t alias should be used for that purpose (names matching perfectly), and that size_t interop is far more important and breaking it in 2.079+ by no means worth the questionable gain.
E.g., there are about 90 occurrences of size_t in the DMD C++ headers included by LDC. Most of them affect the function name mangling and would thus need to be changed to the ugly d_size_t typedef to fix all potential linking errors. So almost every mixed D/C++ code base supporting 64-bit macOS will have to adapt the code on the C++ and/or D side due to this breaking 2.079 ABI change, so I'd rather revert it while we still can.
The new/fixed aliases in druntime (cpp_size_t, int64_t, cpp_long, cpp_longlong etc.), coupled with the magic enums (__c_long etc.) for implicit convertibility, now give the user the right tools for proper portable C++ interop wrt. integers in new code.

@dlang-bot dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label May 23, 2018
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 fetch digger
dub run digger -- build "master + dmd#8285"

@kinke
Copy link
Contributor Author

kinke commented May 23, 2018

Requires dlang/druntime#2191.

@kinke kinke changed the title [WIP] Revert C++-mangling of long back to C++ long on 64-bit OS X [WIP] Revert mangling of D long back to C++ long on 64-bit OS X May 23, 2018
@kinke
Copy link
Contributor Author

kinke commented May 23, 2018

Mangling D long as C++ long consistently on all 64-bit Posix platforms allows for these tiny core.stdc.config adaptations:

@@ -117,14 +117,11 @@ else version( Posix )
 {
   static if( (void*).sizeof > int.sizeof )
   {
-    enum __c_long  : long;
-    enum __c_ulong : ulong;
-
     enum __c_longlong  : long;
     enum __c_ulonglong : ulong;

-    alias __c_long   cpp_long;
-    alias __c_ulong  cpp_ulong;
+    alias long  cpp_long;
+    alias ulong cpp_ulong;

     alias long  c_long;
     alias ulong c_ulong;
@@ -198,17 +195,10 @@ else version( SDC )

 static assert(is(c_long_double), "c_long_double needs to be declared for this platform/architecture.");

-version (Darwin)
-{
+version (Darwin) { version (D_LP64) {} else version = Darwin_32; }
+version (Darwin_32)
     alias cpp_size_t = cpp_ulong;
-    version (D_LP64)
-        alias cpp_ptrdiff_t = cpp_long;
-    else
-        alias cpp_ptrdiff_t = ptrdiff_t;
-}
 else
-{
     alias cpp_size_t = size_t;
-    alias cpp_ptrdiff_t = ptrdiff_t;
-}
+alias cpp_ptrdiff_t = ptrdiff_t;
 }

@kinke kinke force-pushed the osx64_long branch 2 times, most recently from 3c28c55 to d8cbe48 Compare May 24, 2018 19:39
@kinke kinke changed the title [WIP] Revert mangling of D long back to C++ long on 64-bit OS X Revert mangling of D long back to C++ long on 64-bit OS X May 24, 2018
@WalterBright
Copy link
Member

As before 2.079, which allowed for direct interop of D size_t with C++ size_t on all platforms except for 32-bit OS X (which is about to reach end of life AFAIK).

The purpose of this change seems to be all about size_t. As the next step, my plan was to fix the declaration of size_t in object.d to match the one used by the corresponding C++ compiler, so there shouldn't be further issues with it. That would seem to negate the point of this PR. This PR doesn't solve the size_t problem either, as while 32 bit OSX may be obsolete, there's nothing preventing another C++ platform having the same issue.

The bottom line is there is no "correct" solution to this problem as long as C++ has multiple types that map to the same size.

@kinke
Copy link
Contributor Author

kinke commented May 24, 2018

I fully agree (and my main point being that the ABI change required not just a few manual adaptations for no real gain). It'd be nice (and the problem solved) if you can come up with something better than alias size_t = cpp_size_t; having such a core type defined as magic __c_long enum on OSX64 doesn't feel right to me.

@WalterBright
Copy link
Member

It doesn't feel right to me, either, but there's nothing we can really do about it. Having size_t not always be a core type on current D platforms forces us to find and fix any issues that may produce. If we can't fix those issues, then this PR is a reasonable alternative.

I'm pretty pleased with the enum hack to deal with this, wish I'd thought of it years ago. It's sooo much better than the struct hack!

@kinke
Copy link
Contributor Author

kinke commented May 24, 2018

I'm pretty pleased with the enum hack to deal with this, wish I'd thought of it years ago. It's sooo much better than the struct hack!

Absolutely, having D size_t defined as magic struct would have never been an option. ;)

@wilzbach wilzbach removed the Review:WIP Work In Progress - not ready for review or pulling label Jul 1, 2018
@wilzbach
Copy link
Contributor

wilzbach commented Jul 1, 2018

Rebased this to master. As the druntime PR has been merged, I don't see anything blocking this?

@wilzbach wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jul 1, 2018
@dlang-bot dlang-bot merged commit 7c20703 into dlang:master Jul 4, 2018
@kinke kinke deleted the osx64_long branch July 4, 2018 12:54
@MartinNowak
Copy link
Member

What nobody did though is testing/fixing dmd's C++ <-> D interop based on this. I'm now getting linkage errors when building dmd with dmd-2.082.0. Guess that's the price of having dropped the OSX self-hosted tests on Travis-CI ¹, but I wouldn't expect it to be of much use despite the integer mangling struggles.

@jacob-carlborg
Copy link
Contributor

I'm now getting linkage errors when building dmd with dmd-2.082.0

Yeah, I run into the same problem today.

@kinke
Copy link
Contributor Author

kinke commented Oct 11, 2018

but I wouldn't expect it to be of much use despite the integer mangling struggles

The LDC self-hosting CI tests (on Windows/Linux/Mac) have proven quite useful, exactly to prevent C++ interop regressions.
LDC even has a special case for DMD host compiler versions in [2.079, 2.081] due to the temporary mangling change...

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

Labels

Merge:auto-merge Merge:72h no objection -> merge The PR will be merged if there are no objections raised.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants