Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

[WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and undefined D vtable entries #2788

Closed
wants to merge 1 commit into from

Conversation

kinke
Copy link
Contributor

@kinke kinke commented Sep 8, 2019

Fixes linker errors for the LDC druntime test runner.

Used source refs: libstdc++, libc++, Visual Studio 2017 headers

version (GenericBaseException)
{
///
~this() nothrow;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not declaring the dtor means it gets an implicit one (base class has one), and that one collides with the C++ one. [For MSVC, everything is defined inline in the C++ headers, so the implicit one does the job.]

///
~this() nothrow;
///
override const(char)* what() const nothrow;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to have the vtable generated on the D side feature the right override in that vtable slot.

final bool before(const type_info rhs) const;
final const(char)* name(__type_info_node* p = &__type_info_root_node) const;
final bool before(const type_info rhs) const nothrow;
final const(char)* name(__type_info_node* p = &__type_info_root_node) const nothrow;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually the wrong signature for VS 2017 (which takes no params), but I guess DMD has to maintain VS 2010 compatibility or whatever.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have -extern-std=, so we can version the C++ code according to the C++ version it binds to.

@@ -85,13 +85,13 @@ else version (CppRuntime_Microsoft)

class bad_cast : exception
{
this(const(char)* msg = "bad cast");
this(const(char)* msg = "bad cast") nothrow { super(msg); }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defined inline in C++ header.

void dtor1(); // consume destructor slot in vtbl[]
void dtor2(); // consume destructor slot in vtbl[]
void dtor1() {} // consume destructor slot in vtbl[]
void dtor2() {} // consume destructor slot in vtbl[]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vtable on the D side needs dummy definitions to put into the first 2 vtable slots.

Copy link
Contributor

@TurkeyMan TurkeyMan Sep 10, 2019

Choose a reason for hiding this comment

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

O_o ... this is like 12 months out of date. Put an actual destructor.

//~this();
override const(char)* what() const;
this() nothrow {}
~this() nothrow;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, implicit dtor colliding with C++ one when linking in libstdc++.

@kinke
Copy link
Contributor Author

kinke commented Sep 8, 2019

Due to Phobos including druntime, Phobos would need to be adapted, i.e., linking shared Phobos against lib[std]c++ too.

@kinke kinke force-pushed the stdcpp3 branch 2 times, most recently from 0b43ad6 to 96b0394 Compare September 8, 2019 19:42
@kinke kinke marked this pull request as ready for review September 8, 2019 21:36
@kinke kinke changed the title [stable] Fix missing/duplicate symbol definitions in core.stdcpp.{exception,typeinfo} [blocked] [stable] Fix missing/duplicate symbol definitions in core.stdcpp.{exception,typeinfo} Sep 8, 2019
@dlang-bot
Copy link
Contributor

dlang-bot commented Sep 8, 2019

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 coverage diff by visiting the details link of the codecov check)
  • 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 run digger -- build "master + druntime#2788"

@Geod24
Copy link
Member

Geod24 commented Sep 8, 2019

Due to Phobos including druntime, Phobos would need to be adapted, i.e., linking shared Phobos against lib[std]c++ too.

That's a huge issue IMO. We can't link druntime/phobos with the C++ standard library for the sake of those bindings.

@kinke
Copy link
Contributor Author

kinke commented Sep 8, 2019

It only affects the shared libraries and should thus just be an additional dependency to lib[std]c++.{so,dylib}. When using the static libraries, you only need the C++ runtime when using core.stdcpp.{exception,typeinfo} [edit: and possibly other modules in that package when instantiating the templates]. If that's not acceptable (I don't have a strong opinion about this), then core.stdcpp.* should probably be extracted out of druntime.

@JohanEngelen
Copy link
Contributor

I also think it is bad if we need to link with the C++ standard library, even if it is only for the shared libraries. In my opinion that is too high a price to pay for the convenience of having C++ bindings in the D std library. Simply moving the bindings into their own separate package seems much more appropriate.

@kinke
Copy link
Contributor Author

kinke commented Sep 9, 2019

Pinging @TurkeyMan.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Sep 10, 2019

These are the exact 2 modules that I didn't write... I think the need a complete overhaul, but i'll give this patch a better look when I'm home.
Generally; local methods should be extern(D) so they don't collide with C++, and declare proper ctor/dtor's because they mangle and consume the vtable properly now.

These functions aren't templates, and we don't want druntime to depend on the C++ runtime. All these functions are basically 1-liners, so we should just mirror the C++ implementation, and not extern.
C++ bindings must obviously not cause link errors, or infer a hard dependency on libc++. Care is required, as I've done for all the other modules.

@kinke kinke requested a review from schveiguy as a code owner May 6, 2020 19:12
@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 6, 2020
@kinke kinke changed the base branch from stable to master May 6, 2020 19:12
@kinke kinke changed the title [blocked] [stable] Fix missing/duplicate symbol definitions in core.stdcpp.{exception,typeinfo} [WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and missing D vtable entries May 6, 2020
@dlang-bot dlang-bot added the WIP Work In Progress - not ready for review or pulling label May 6, 2020
@kinke kinke force-pushed the stdcpp3 branch 2 times, most recently from 5f1e950 to 665b571 Compare May 6, 2020 19:50
@TurkeyMan
Copy link
Contributor

Where's this at now? I can't really see from the CI output what's going on here.
Are there multiple-definition errors? It looks like that's what I'd expect...

@kinke
Copy link
Contributor Author

kinke commented May 7, 2020

Had some time to re-look into this and tried the extern(D) way as you suggested. Now the druntime testrunner compiles and links fine for LDC (ldc-developers/ldc#3158), i.e., the D vtables can be populated; no idea what's going on with the failing subset of Mac stdcpp CI jobs here.

See the 2nd paragraph in ldc-developers/ldc#3158 (comment) as to why extern(D) isn't the perfect solution when it comes to virtual functions and trying to avoid a C++ runtime dependency while not conflicting with its symbols. For LDC, @weak extern(C++) might be the solution (ldc-developers/ldc#3424).

@TurkeyMan
Copy link
Contributor

Yeah, I was gonna say, weak linkage is the obvious answer, but Walter has rejected it for years.
Perhaps we can push for it again? We need to standardise a weak attribute, it's a PITA being compiler specific!

@kinke
Copy link
Contributor Author

kinke commented May 7, 2020

but Walter has rejected it for years

I guess that's solvable by providing him with a finished implementation. ;) - None of my business though.

@kinke kinke changed the title [WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and missing D vtable entries [WIP] core.stdcpp.{exception,typeinfo}: Fix D/C++ symbol conflicts and undefined D vtable entries May 17, 2020
@dlang-bot dlang-bot added the Needs Rebase needs a `git rebase` performed label Aug 22, 2021
@ibuclaw
Copy link
Member

ibuclaw commented Aug 25, 2021

Revived as #3552

@ibuclaw ibuclaw closed this Aug 25, 2021
@kinke kinke deleted the stdcpp3 branch September 5, 2021 10:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue Needs Rebase needs a `git rebase` performed stalled WIP Work In Progress - not ready for review or pulling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants