Re: Wrong class export/visibility setting for shared libs in many Boost libs

From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2021-02-02 12:44:42

On 02/02/2021 10:14, Alexander Grund via Boost wrote:

>>> TLDR: Check your classes if they use BOOST_*_DECL and change that to
>>> BOOST_SYMBOL_VISIBLE
>> That's identical to making -fvisibility=hidden no longer have effect. So
>> please Boost maintainers DO NOT DO THE ABOVE.
>
> I was partially wrong. IMO this is a bug in Boost.Config which should
> not define BOOST_SYMBOL_IMPORT to nothing.

Boost.Config is correct.

> Reason is that Win32 needs the declspec on classes:
> https://docs.microsoft.com/de-de/cpp/cpp/using-dllimport-and-dllexport-in-cpp-classes?view=msvc-160

You only need dllimport for variables. If you don't apply it to types,
MSVC "does the right thing".

> Could you elaborate why marking classes currently marked as BOOST_*_DECL
> with BOOST_SYMBOL_VISIBLE would make visibility not have an effect?

You would export an imported library's exported symbols in your own
library's exported symbols set.

If you want that, choose -fvisibility=default, because if you do as you
suggest, it's the same thing. People asking for -fvisibility=hidden
specifically are requesting you don't export unrelated symbols from your
library's symbol export set. You can go ahead and do it anyway, but
users would rightly complain that Boost is messing up their links. If
you consider that some use a visibility hidden internal Boost to prevent
collision with end users using their own Boost, I can foresee a bunch of
angry users.

> The exception in question is marked with BOOST_*_DECL which is empty for
> consumers and hence hidden visibility if the consumer builds with hidden
> visibility.

That is a bug in ProgramOptions.

> Marking the class with BOOST_SYMBOL_VISIBLE is fully in agreement with
> your statement, isn't it? Because currently the consumer TU won't have
> default visibility for the class.

I vaguely remember me and Dave added a special macro for exception
types, so you could annotate your exception type with it, and "the right
thing" was done. I don't see it there now however.

>> Myself and Dave Abrahams designed the current Boost macro markup
>> according to https://gcc.gnu.org/wiki/Visibility (actually, the GCC
>> documents' end was written according to what we had done in Boost, but I
>> digress) and it is correct, *if Boost libraries apply the macros
>> correctly* as per the guide.
> 
> What is the guide? Currently the Boost.Config documents
> BOOST_SYMBOL_VISIBLE as to be used for exceptions:
> https://github.com/boostorg/config/blob/5bcaa933b55dd58797775d87605a6cdef4acd5a2/doc/macro_reference.qbk#L1555

I remember writing some parts of that documentation snippet, but other
parts look unfamiliar. It was all fifteen years ago now.

If the docs say do as they do, it seems reasonable to me.

> However this seems to be contradicting
> https://docs.microsoft.com/de-de/cpp/cpp/using-dllimport-and-dllexport-in-cpp-classes?view=msvc-160
> because BOOST_SYMBOL_VISIBLE is empty on Win32 but the classes should be
> marked declspec(dllimport/dllexport), or am I missing anything here?

As I mentioned, if you don't dllimport on Win32 for types, it's benign.
Technically it makes the link very slightly slower, and I suppose if you
do dllimport then you trap accidentally marking a symbol for import and
export in separate TUs during linking. But if the config is the way it
is right now, there was probably good reason for it, at the time.

Niall