Mark exceptions with export to fix exceptions in shared libraries

[yeswalrus]

In a setup with two shared libraries, A and B, if A throws an exception, B should be able to catch it. To do this, the exception must have external linkage, otherwise A and B will both have their own separate copies of the type. See https://gcc.gnu.org/wiki/Visibility for more information, but the short version is that all exceptions defined in libraries should be declared with __attribute__ ((visibility("default")))

[eacousineau]

Thanks!
FTR Motivating Gitter discussion: https://gitter.im/pybind/Lobby?at=5cb937183b6cb0686a177d53

[YannickJadoul]

Nice!

Something's wrong on Windows, though :-(

non dll-interface class 'std::runtime_error' used as base for dll-interface class 'pybind11::builtin_exception'

Any idea what this problem could be and how it could be solved?

[yeswalrus]

https://stackoverflow.com/questions/24511376/how-to-dllexport-a-class-derived-from-stdruntime-error
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4275?view=vs-2019
Looks like C4275 is noise and can safely be silenced.
It's not the only warning however:
c:\projects\pybind11\include\pybind11\pytypes.h(350): warning C4251: 'pybind11::error_already_set::type': class 'pybind11::object' needs to have dll-interface to be used by clients of class 'pybind11::error_already_set'

Frankly I'm surprised Linux wasn't complaining about this one too

[eacousineau]

I'm not sure if you'll be able to have this land by hoisting the visibility of py::object and py::handle. I believe those are intentionally sealed to minimize binary size (if LTO works, I guess?) and prevent version clashes.

Can I ask why you hoisted the visibility on those types?

[yeswalrus]

Because compilers will throw warnings if I don't, specifically MSVC C4251.

the error_already_set exception contains py::object instances, which derives from py::handle. The way I understand it, because these objects are expected to be passed across shared library boundaries, they must be exported to ensure correct behavior on the receiving end.

It might be the case that we can simply silence the warning, but as I understand it, that might be dangerous. I don't believe that this will impact binary size, but that's worth double checking

[yeswalrus]

master sozie-pybind11_tests.cpython-36m-x86_64-linux-gnu.so: 1975048
branch sozie-pybind11_tests.cpython-36m-x86_64_linux-gnu.so:1979240

So it looks like it does increase binary size by a small amount. However, I think it' still more correct to do this - any time an object passes between shared libraries, it ought to be marked as exported, and these explicitly do.

[eacousineau]

Aye, that makes sense.

Out of my dogma-fueled paranoia on type privacy, I may suggest reworking error_already_set to not use pybind11s Python API, and just use the C API directly (e.g. PyObject* to store, Py_XINCREF on T() and Py_XDECREF on ~T()), just to minimize how much is neccessary to export the error to preserve RTTI.

However, I'd also suggest waiting until @wjakob weighs in to actually pull the trigger on making that change.

[wjakob]

This looks generally good to me. Two questions/comments:

1. do pyobject_tag, object and handle really need PYBIND11_EXPORT? Does ist still work if this is removed? The goal of this PR is to ensure that the std::type_info of an exception class is exported so that inter-module throws are caught correctly, and I don't see how this is related. If this is just to silence a warning, can you #pragma disable it instead?

2. Let's leave error_already_set as is, please :).

[wjakob]

ping?

[wjakob]

@yeswalrus, are you still planning to submit this PR? If so, could you respond to my feedback above?

Thanks,
Wenzel

[Skylion007]

This was fixed in a later PR: #2999