Link error when using LLVM or Intel Toolset under Visual Studio #927

Baljak · 2017-06-29T09:13:44Z

When using the last LLVM and Intel C++ Compiler versions (4.0 and 17.0 respectively) under Visual Studio, the following error occurs (both with 2.1.1 and the current master):

error LNK2001: unresolved external symbol "public: __cdecl pybind11::error_already_set::error_already_set(class pybind11::error_already_set const &)" (??0error_already_set@pybind11@@QEAA@AEBV01@@Z) 1>C:\Dev\PythonLibs\Tests\Pybind11\build\bin\Release\pybind11test.cp36-win_amd64.pyd : fatal error LNK1120: 1 unresolved externals

The following code was used:

#include <pybind11/pybind11.h>

namespace py = pybind11;

struct S
{
	int value;

	int get() const { return value; }
	void set(int v) { value = v; }
};


PYBIND11_MODULE(pybind11test, m)
{
	py::class_<S>(m, "S")
		.def(py::init<>())
		.def("get", &S::get)
		.def("set", &S::set)
		.def_readwrite("spectrumType", &S::value);
}

I was able to fix this problem by changing line 704 in common.h (from the master branch):

error_already_set(const error_already_set &) = delete;

to

error_already_set(const error_already_set &) = default;

But I'm not sure this is the right way to fix the problem though.

The text was updated successfully, but these errors were encountered:

jagerman · 2017-07-17T19:58:00Z

This seems like a bug in the respective compilers.

Does it help if you make the deleted copy constructor private rather than public? (I suppose at the very least that might help figure out where the copy is actually happening).

Baljak · 2017-07-18T14:56:52Z

When making the copy constructor private, I get the same error on both compilers with no additional information. I switched to LLVM 4.0.1 which was recently released with no more success.

Baljak · 2017-07-20T07:53:48Z

Ok, I think this a not a compiler bug and that Intel and Clang are right here. See https://stackoverflow.com/questions/16432959/exception-and-copy-constructor-c.

Especially:
When the thrown object is a class object, the copy/move constructor and the destructor shall be accessible, even if the copy/move operation is elided (12.8).

So the copy constructor of error_already_set should be either defaulted or actually implemented, but not deleted.

`error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes pybind#927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit convoluted: it called `restore()` to restore the currently captured error, but then immediately cleared it. The only point seems to be because `PyErr_Restore` decrements the reference counts on the held objects, but we can simply do that manually. Or, actually, just *not* decrement them at all and let them be unreferenced (via the `py::object` destructor) when the `error_already_set` is itself destroyed. `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated.

jagerman · 2017-07-21T04:56:14Z

The thing about this that makes me thing it's a compiler bug is that with the explicit = delete; there should be a compilation failure if a copy constructor is required, not a linking failure. The compiler is essentially acting as though the = delete; isn't there at all. This is a bit weird: each of MSVC, clang, and ICC can handle this code just without any such error on their own; but for some reason ICC and clang are doing something that makes the MSVC linker want to link to the non-existent copy constructor.

I tried (and failed) to get the test suite building under MSVC/clang-cl.exe/cmake. I'm not sure if it's an issue with VS 2017, LLVM, cmake, or me, but I couldn't get the test suite compiled (it seems the cmake scripts adds -fexceptions -frtti to the compiler flags, but clang-cl.exe barfs on those, looking instead for the MSVC cl.exe flags (e.g. /EHsc); at that point I gave up.)

Now all of that said, I took a better look at error_already_set and I think it's considerably more complicated than it needs to be. I've submitted PR #954 to simplify it and, hopefully, fix this issue—given my compilation trouble above, feedback as to whether this works would be appreciated.

Baljak · 2017-07-21T08:28:21Z

I tried your PR and it works as expected, thanks a lot!

jagerman · 2017-07-21T14:54:31Z

Thanks for the report. I'm going to keep the bug open until the PR gets merged (mainly for bookkeeping, in case someone identifies a major problem with the PR, etc.).

`error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes pybind#927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit convoluted: it called `restore()` to restore the currently captured error, but then immediately cleared it. The only point seems to be because `PyErr_Restore` decrements the reference counts on the held objects, but we can simply do that manually. Or, actually, just *not* decrement them at all and let them be unreferenced (via the `py::object` destructor) when the `error_already_set` is itself destroyed. `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated.

`error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes pybind#927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit convoluted: it called `restore()` to restore the currently captured error, but then immediately cleared it. The only point seems to be because `PyErr_Restore` decrements the reference counts on the held objects, but we can simply do that manually. (Python documentation suggests that `PyErr_Fetch` doesn't have to be matched with a `PyErr_Restore`). This updates the code to simply release the references on the three objects (preserving the gil acquire). `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated. This also fixes a minor potential issue with the destruction: it is technically possible for `value` to be null (though this seems likely to be rare in practice); this updates the check to look at `type` which will always be non-null for a `Fetch`ed exception.

`error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes pybind#927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit more paranoid that needed: it called `restore()` to restore the currently captured error, but then immediately cleared it, using the `PyErr_Restore` to release the references. That's unnecessary: it's valid for us to release the references manually. This updates the code to simply release the references on the three objects (preserving the gil acquire). `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated. This also fixes a minor potential issue with the destruction: it is technically possible for `value` to be null (though this seems likely to be rare in practice); this updates the check to look at `type` which will always be non-null for a `Fetch`ed exception. This also adds error_already_set round-trip throw tests to the test suite.

`error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes #927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit more paranoid that needed: it called `restore()` to restore the currently captured error, but then immediately cleared it, using the `PyErr_Restore` to release the references. That's unnecessary: it's valid for us to release the references manually. This updates the code to simply release the references on the three objects (preserving the gil acquire). `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated. This also fixes a minor potential issue with the destruction: it is technically possible for `value` to be null (though this seems likely to be rare in practice); this updates the check to look at `type` which will always be non-null for a `Fetch`ed exception. This also adds error_already_set round-trip throw tests to the test suite.

Baljak closed this as completed Jul 20, 2017

Baljak reopened this Jul 20, 2017

jagerman mentioned this issue Jul 21, 2017

Simplify error_already_set #954

Merged

Baljak closed this as completed Jul 21, 2017

jagerman reopened this Jul 21, 2017

jagerman closed this as completed in #954 Jul 29, 2017

rwgk mentioned this issue Feb 9, 2023

FWD pybind11 google/pybind11clif#927

Closed

loumalouomega mentioned this issue Oct 25, 2024

[External] Update pybind11 to last version KratosMultiphysics/Kratos#12788

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Link error when using LLVM or Intel Toolset under Visual Studio #927

Link error when using LLVM or Intel Toolset under Visual Studio #927

Baljak commented Jun 29, 2017 •

edited

Loading

jagerman commented Jul 17, 2017

Uh oh!

Baljak commented Jul 18, 2017 •

edited

Loading

Uh oh!

Baljak commented Jul 20, 2017 •

edited

Loading

Uh oh!

jagerman commented Jul 21, 2017 •

edited

Loading

Uh oh!

Baljak commented Jul 21, 2017

Uh oh!

jagerman commented Jul 21, 2017

Uh oh!

Link error when using LLVM or Intel Toolset under Visual Studio #927

Link error when using LLVM or Intel Toolset under Visual Studio #927

Comments

Baljak commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jagerman commented Jul 17, 2017

Uh oh!

Baljak commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Baljak commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagerman commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Baljak commented Jul 21, 2017

Uh oh!

jagerman commented Jul 21, 2017

Uh oh!

Baljak commented Jun 29, 2017 •

edited

Loading

Baljak commented Jul 18, 2017 •

edited

Loading

Baljak commented Jul 20, 2017 •

edited

Loading

jagerman commented Jul 21, 2017 •

edited

Loading