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

[MSVCRT] core.stdcpp.typeinfo: Don't implement exception ctors#1709

Merged
andralex merged 1 commit intodlang:masterfrom
kinke:patch-1
Jan 14, 2017
Merged

[MSVCRT] core.stdcpp.typeinfo: Don't implement exception ctors#1709
andralex merged 1 commit intodlang:masterfrom
kinke:patch-1

Conversation

@kinke
Copy link
Contributor

@kinke kinke commented Dec 9, 2016

Implementing them (wrongly btw, ignoring the msg) leads to an implicit call to the super ctor of base class core.stdcpp.exception.std.exception, which is only declared in D. Even if the ctor signature was correct (see below), it's mangled differently (see dlang/dmd#5884), so there'd be an unresolved external anyway. So don't implement it, just declare it (also done for Glibc) to defer the linking error.

Quality-wise, the 2 core.stdcpp modules are at most at an alpha stage for MSVCRT. Other issues I've noted so far using VS 2015:

  • The signature of the core.stdcpp.exception.std.exception.this() ctor is wrong. There's no default, i.e., parameter-less one, only std::exception::exception(const char *_Message = "unknown", int x=1).
  • std::exception only contains a pointer, no additional bool field.
  • The signature of core.stdcpp.typeinfo.std.type_info.name() is wrong. It doesn't take any arguments.

The first 2 aren't crucial, as these C++ exceptions are most likely never constructed on the D side. But the last one will need to be fixed. @rainers: Which VS versions does DMD currently support?

Implementing them (wrongly btw, ignoring the msg) leads to an implicit call to the `super` ctor of base class `core.stdcpp.exception.std.exception`, which is only declared in D. Even if the ctor signature was correct (see below), it's mangled differently (see dlang/dmd#5884), so there'd be an unresolved external anyway. So don't implement it, just declare it (also done for Glibc) to defer the linking error.

Quality-wise, the 2 `core.stdcpp` modules are at most at an alpha stage for MSVCRT. Other issues I've noted so far using **VS 2015**:
* The signature of the `core.stdcpp.exception.std.exception.this()` ctor is wrong. There's no default, i.e., parameter-less one, only `std::exception::exception(const char *_Message = "unknown", int x=1)`.
* `std::exception` only contains a pointer, no additional `bool` field.
* The signature of `core.stdcpp.typeinfo.std.type_info.name()` is wrong. It doesn't take any arguments.

The first 2 aren't crucial, as these C++ exceptions are most likely never constructed on the D side. But the last one will need to be fixed. @rainers: Which VS versions does DMD currently support?
@rainers
Copy link
Member

rainers commented Dec 17, 2016

@rainers: Which VS versions does DMD currently support?

AFAICT anything starting with VS2005 should work with respect to C. Regarding C++ I suspect there were no tests with VS2008 or earlier. Last time I checked the build server runs VS2013. Considering no complaints about the makefiles Walter probably still uses VS2010.

Other issues I've noted so far using VS 2015:

The declarations match VS2010 or earlier (and to some degree VS2012).

@rainers
Copy link
Member

rainers commented Dec 17, 2016

So don't implement it, just declare it (also done for Glibc) to defer the linking error.

I agree, that is better than providing an incomplete implementation. VS2010 has these constructors declared inline in the header, though. If we still support these versions, we should verify the symbols are still available in the MS libraries.

@kinke
Copy link
Contributor Author

kinke commented Dec 17, 2016

VS2010 has these constructors declared inline in the header, though. If we still support these versions, we should verify the symbols are still available in the MS libraries.

Ctors defined in C++ can currently not be used from D anyway (and the other way around too!) due to the different mangling, generally and not just for MSVC. So caring about these C++ symbols will only become relevant after dlang/dmd#5884 is finished, which is not only about mangling, but also about how to make sure extern(C++) ctors implemented in D and called from C++ get an instance initialized with T.init. This could be solved by initializing to T.init at the start of the extern(C++) ctor, but that would imply some performance overhead (double initialization) when called from D.
Another approach could consist in keeping on using the D ctor mangling (T.__ctor()), but adding an additional C++ ctor trampoline (T.T()) which does the T.init blit and then calls the D ctor. D code would call the D T.__ctor() directly if the extern(C++) ctor is defined in the D source, and the C++ T.T() ctor if it is only declared.

This PR gets rid of linking errors wrt. super() for LDC, as we pull in every single druntime object when linking the druntime test runner and DMD apparently doesn't (ldc-developers/ldc#1917) (edit: or does the linking on a per-function basis as you mentioned lately IIRC).

@andralex
Copy link
Member

cool

@andralex
Copy link
Member

Hmmm, at some point we need to figure out a test rig for the interop code.

@andralex
Copy link
Member

Auto-merge toggled on

@andralex andralex merged commit b1182f9 into dlang:master Jan 14, 2017
@kinke kinke deleted the patch-1 branch January 14, 2017 17:00
kinke added a commit to kinke/ldc that referenced this pull request Jan 22, 2017
The most urgent linking errors have been fixed upstream by
dlang/druntime#1709 in the meantime.
kinke added a commit to kinke/ldc that referenced this pull request Jan 22, 2017
The most urgent linking errors have been fixed upstream by
dlang/druntime#1709 in the meantime.
kinke added a commit to kinke/ldc that referenced this pull request Jan 22, 2017
The most urgent linking errors have been fixed upstream by
dlang/druntime#1709 in the meantime.
kinke added a commit to kinke/ldc that referenced this pull request Jan 22, 2017
The most urgent linking errors have been fixed upstream by
dlang/druntime#1709 in the meantime.
kinke added a commit to kinke/ldc that referenced this pull request Feb 6, 2017
The most urgent linking errors have been fixed upstream by
dlang/druntime#1709 in the meantime.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants