[boost] Wrong class export/visibility setting for shared libs in many Boost libs

[Alexander Grund via Boost]

Hi Boost maintainers,

TLDR: Check your classes if they use BOOST_*_DECL and change that to
BOOST_SYMBOL_VISIBLE


In Boost.ProgramOptions I discovered an issue as a user, where I was
unable to catch an exception thrown from the shared Boost library in an
executable when both are build with hidden visibility.

It turns out the exception class was marked with
BOOST_PROGRAM_OPTIONS_DECL which evaluates to "visible" when building
the DLL but to nothing when using it.
This makes the linker (in theory) mark the typeinfo as hidden which
means that the class thrown by Boost and the one tried to catch are
unrelated.

See https://gcc.gnu.org/wiki/Visibility

> Symbol visibility is "default" by default but if the linker
encounters just one definition with it hidden - just one - that typeinfo
symbol becomes permanently hidden (remember the C++ standard's ODR - one
definition rule).

This is pretty clear that the current approach done by some Boost
libraries doesn't/shouldn't work and BOOST_SYMBOL_VISIBLE should be used
instead (or BOOST_SYMBOL_IMPORT should expand to BOOST_SYMBOL_VISIBLE.
This depends on how MSCV/Win32 platforms handle class visibility as on
Win32 BOOST_SYMBOL_VISIBLE expands to nothing. Maybe someone can clarify
here.)

As another datapoint:
https://github.com/boostorg/config/blob/5bcaa933b55dd58797775d87605a6cdef4acd5a2/doc/macro_reference.qbk
documents BOOST_SYMBOL_VISIBLE as the macro to be used for this.

Libraries which are (very likely) affected by this (quick grep) include:

Chrono
Container
Context
Contract
Coroutine
Fiber
Filesystem
IOStreams
JSON
Locale
MPI
ProgramOptions
Python
Regex
Test
Thread
Timer

Seemingly Linux platforms are less strict in actually enforcing this,
due to reason unknown to me. See my SO question:
https://stackoverflow.com/questions/65951466/why-can-a-thrown-exception-with-hidden-visibility-still-be-caught

Reason why this went undetected so far are the above (works on some
Linux, fails on e.g. OSX) and that user programs may often not be build
with hidden visibility AND rely on the exact same typeinfo from the
Boost library (most prominent: exceptions, dynamic_cast)

Regards,
Alex

[Niall Douglas via Boost]

On 02/02/2021 08:35, Alexander Grund via Boost wrote:
> Hi Boost maintainers,
>
> 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.

> In Boost.ProgramOptions I discovered an issue as a user, where I was
> unable to catch an exception thrown from the shared Boost library in an
> executable when both are build with hidden visibility.

As per the instructions at https://gcc.gnu.org/wiki/Visibility, all
types requiring RTTI lookup to succeed outside their TU need to be
marked default visibility in all translation units. If you fail to do
this in any single translation unit, the typeinfo become hidden, and the
RTTI lookup will silently fail.

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.

We did, at the time, fix up most Boost libraries, circa 2005. Some
maintainers, at the time, refused to support -fvisibility=hidden,
feeling it to be an abomination. And libraries added since have done
their own thing. So I would not be surprised if quality of support has
become varying across libraries.

Niall

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

[Alexander Grund via Boost]

Am 02.02.21 um 10:22 schrieb Niall Douglas via Boost:

> On 02/02/2021 08:35, Alexander Grund via Boost wrote:
>> Hi Boost maintainers,
>>
>> 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. I opened an issue to discuss
this there: https://github.com/boostorg/config/issues/362
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


Could you elaborate why marking classes currently marked as BOOST_*_DECL
with BOOST_SYMBOL_VISIBLE would make visibility not have an effect?
Because this is essentially what I'm suggesting for Boost.Config:
BOOST_*_DECL is either BOOST_SYMBOL_IMPORT or BOOST_SYMBOL_EXPORT, the
former is (currently) empty, the latter is BOOST_SYMBOL_VISIBLE
(disregarding Win32, see above)

Note that this es exactly what e.g. CMake does:
https://github.com/Kitware/CMake/blob/36bb0e32d7e3976eed424078cf5cac459651f0f1/Modules/GenerateExportHeader.cmake#L309-L311

>> In Boost.ProgramOptions I discovered an issue as a user, where I was
>> unable to catch an exception thrown from the shared Boost library in an
>> executable when both are build with hidden visibility.
> As per the instructions at https://gcc.gnu.org/wiki/Visibility, all
> types requiring RTTI lookup to succeed outside their TU need to be
> marked default visibility in all translation units. If you fail to do
> this in any single translation unit, the typeinfo become hidden, and the
> RTTI lookup will silently fail.

Fully agreed so far.
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.
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.
Only Win32 must be considered too so classes should stay marked as
BOOST_*_DECL (for the declspec) but that must not expand to nothing on
Linux/OSX

> 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

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?

Alex

[Andrey Semashev via Boost]

On 2/2/21 11:35 AM, Alexander Grund via Boost wrote:
> Hi Boost maintainers,
>
> TLDR: Check your classes if they use BOOST_*_DECL and change that to
> BOOST_SYMBOL_VISIBLE
>
>
> In Boost.ProgramOptions I discovered an issue as a user, where I was
> unable to catch an exception thrown from the shared Boost library in an
> executable when both are build with hidden visibility.
>
> It turns out the exception class was marked with
> BOOST_PROGRAM_OPTIONS_DECL which evaluates to "visible" when building
> the DLL but to nothing when using it.
> This makes the linker (in theory) mark the typeinfo as hidden which
> means that the class thrown by Boost and the one tried to catch are
> unrelated.

This means that BOOST_PROGRAM_OPTIONS_DECL is incorrectly defined.

The basic gist is as follows:

1. On Windows, there is no visibility support. gcc and clang still
support visibility attributes (including BOOST_SYMBOL_VISIBLE), but I
don't think they have any effect. On Windows, you should be using
BOOST_SYMBOL_EXPORT/BOOST_SYMBOL_IMPORT, depending on when your library
is compiled or when user's code is compiled.
2. Again, on Windows, there is no need to export/import RTTI for it to
work, so you don't need to mark exceptions just to make them catchable.
You may still want to export/import a class (including an exception) if
you want to export/import all its methods, *including the implicitly
generated ones*. In particular, the compiler will probably not inline
any methods of the class, so it is more preferable to mark individual
methods rather than the whole class.
3. On other systems, where visibility is supported, BOOST_SYMBOL_VISIBLE
must be used both when compiling the library and user's code, and it
should be used to mark types whose RTTI is required to work across
library boundaries. This includes exceptions and types that participate
in dynamic_cast. Also, BOOST_SYMBOL_VISIBLE must be used to
export/import methods from the library.

So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:

- BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
- BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
by user;
- BOOST_SYMBOL_VISIBLE - in any other case.

Then you can use BOOST_PROGRAM_OPTIONS_DECL to mark methods and classes
that you want to be exported/imported from the library. If a particular
type does not need to be exported/imported, but whose RTTI must still be
accessible across library boundaries, mark that type with
BOOST_SYMBOL_VISIBLE, not BOOST_PROGRAM_OPTIONS_DECL. You can still use
BOOST_PROGRAM_OPTIONS_DECL to export/import select methods of that type,
if needed.

PS: I remember someone saying that on recent Windows dllexport/dllimport
attributes are no longer necessary. I don't quite know how that works,
but for backward compatibility we should probably keep using
BOOST_SYMBOL_EXPORT/BOOST_SYMBOL_IMPORT as described above.

[Andrey Semashev via Boost]

PPS: I forgot to add that recent gcc versions don't need RTTI to be
publicly visible to be able to catch exceptions or compare type_info, as
they now perform deep string comparison instead of address comparison.
However, older compiler versions are still susceptible to this, and I'm
not sure if the deep comparison behavior is used on all platforms.

[Andrey Semashev via Boost]

On 2/2/21 1:14 PM, Alexander Grund via Boost wrote:
> Am 02.02.21 um 10:22 schrieb Niall Douglas via Boost:
>> On 02/02/2021 08:35, Alexander Grund via Boost wrote:
>>> Hi Boost maintainers,
>>>
>>> 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. I opened an issue to discuss
> this there: https://github.com/boostorg/config/issues/362
> 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
>
>
> Could you elaborate why marking classes currently marked as BOOST_*_DECL
> with BOOST_SYMBOL_VISIBLE would make visibility not have an effect?
> Because this is essentially what I'm suggesting for Boost.Config:
> BOOST_*_DECL is either BOOST_SYMBOL_IMPORT or BOOST_SYMBOL_EXPORT, the
> former is (currently) empty, the latter is BOOST_SYMBOL_VISIBLE
> (disregarding Win32, see above)
>
> Note that this es exactly what e.g. CMake does:
> https://github.com/Kitware/CMake/blob/36bb0e32d7e3976eed424078cf5cac459651f0f1/Modules/GenerateExportHeader.cmake#L309-L311

IMHO, Boost.Config provides basic building blocks and nothing more, and
that is a good thing. Each library must define its own export/import
macro using these building blocks, and that macro should take into
account its library-specific macros, such as BOOST_*_DYN_LINK.

[Niall Douglas via Boost]

On 02/02/2021 11:51, Andrey Semashev via Boost wrote:

> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>
> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
> by user;
> - BOOST_SYMBOL_VISIBLE - in any other case.

This causes Boost.ProgramOptions symbols to be exported in any shared
object or executable which links in Boost.ProgramOptions i.e. you
reexport the same symbols.

So, as I already said, don't do that. Only symbols whose typeinfos need
to be seen during RTTI lookup should always be BOOST_SYMBOL_VISIBLE.
That is generally types thrown as exceptions, and anything else subject
to dynamic_cast.

Niall

[Niall Douglas via Boost]

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.

[Andrey Semashev via Boost]

On 2/2/21 3:34 PM, Niall Douglas via Boost wrote:
> On 02/02/2021 11:51, Andrey Semashev via Boost wrote:
>
>> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>>
>> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
>> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
>> by user;
>> - BOOST_SYMBOL_VISIBLE - in any other case.
>
> This causes Boost.ProgramOptions symbols to be exported in any shared
> object or executable which links in Boost.ProgramOptions i.e. you
> reexport the same symbols.

It does not because the symbols to be exported are (supposed to be) only
defined in Boost.ProgramOptions library.

[Mathias Gaunard via Boost]

That sounds like a step backwards.
Do you have more information about this change and the motivation behind it?

[Andrey Semashev via Boost]

On 2/2/21 4:23 PM, Mathias Gaunard wrote:
> On Tue, 2 Feb 2021 at 11:59, Andrey Semashev via Boost
> <bo...@lists.boost.org> wrote:
>>
>> On 2/2/21 2:51 PM, Andrey Semashev wrote:
>>> On 2/2/21 11:35 AM, Alexander Grund via Boost wrote:
>
>> PPS: I forgot to add that recent gcc versions don't need RTTI to be
>> publicly visible to be able to catch exceptions or compare type_info, as
>> they now perform deep string comparison instead of address comparison.
>> However, older compiler versions are still susceptible to this, and I'm
>> not sure if the deep comparison behavior is used on all platforms.
>
> That sounds like a step backwards.
> Do you have more information about this change and the motivation behind it?

Some motivation is given in the sources:

https://github.com/gcc-mirror/gcc/blob/13d8be91218950707bf38bdb1c554cf1605e4501/libstdc%2B%2B-v3/libsupc%2B%2B/typeinfo#L48

[Peter Dimov via Boost]

Alexander Grund wrote:

> Hi Boost maintainers,
>
> TLDR: Check your classes if they use BOOST_*_DECL and change that to
> BOOST_SYMBOL_VISIBLE

Don't. This will break your libraries on Windows.

[Niall Douglas via Boost]

On 02/02/2021 13:02, Andrey Semashev via Boost wrote:
> On 2/2/21 3:34 PM, Niall Douglas via Boost wrote:
>> On 02/02/2021 11:51, Andrey Semashev via Boost wrote:
>>
>>> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>>>
>>> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
>>> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
>>> by user;
>>> - BOOST_SYMBOL_VISIBLE - in any other case.
>>
>> This causes Boost.ProgramOptions symbols to be exported in any shared
>> object or executable which links in Boost.ProgramOptions i.e. you
>> reexport the same symbols.
>
> It does not because the symbols to be exported are (supposed to be) only
> defined in Boost.ProgramOptions library.

BOOST_SYMBOL_VISIBLE is always default visibility, no matter what.

Therefore anything in a Boost.ProgramOptions header would be made
visible and thus exported, which equals exporting all the extern
declarations. That, in turn, would cause ABI collision if somebody tries
linking in a non-matching Boost, under the assumption that an internal
Boost has been hidden from linkage (which "works", so far).

ELF is generally completely borked when it comes to this stuff. We made
it quack like a dog fifteen years ago, but a duck it never can be
without fixing the ELF specification.

Niall

[Alexander Grund via Boost]

> This means that BOOST_PROGRAM_OPTIONS_DECL is incorrectly defined.
>

<snip>

>
> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>
> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
> by user;
> - BOOST_SYMBOL_VISIBLE - in any other case.
>
> Then you can use BOOST_PROGRAM_OPTIONS_DECL to mark methods and
> classes that you want to be exported/imported from the library. If a
> particular type does not need to be exported/imported, but whose RTTI
> must still be accessible across library boundaries, mark that type
> with BOOST_SYMBOL_VISIBLE, not BOOST_PROGRAM_OPTIONS_DECL. You can
> still use BOOST_PROGRAM_OPTIONS_DECL to export/import select methods
> of that type, if needed.
>
> PS: I remember someone saying that on recent Windows
> dllexport/dllimport attributes are no longer necessary. I don't quite
> know how that works, but for backward compatibility we should probably
> keep using BOOST_SYMBOL_EXPORT/BOOST_SYMBOL_IMPORT as described above.

I thought that BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT are exactly
for this: You define *_DECL to *_EXPORT when building the library and
*_IMPORT otherwise (and <empty> for static linking)

Why do individual libraries now have to deal with the Windows vs
non-Windows case? Is there ANY Boost library which does this?

From the BOOST_SYMBOL_VISIBLE description: "Needed for classes that are
not otherwise exported, but are used by RTTI". This sounds to me like
either a BOOST_SYMBOL_EXPORT or BOOST_SYMBOL_IMPORT should work.
Now I'm left to wonder why individual libraries must now distinguish
between OSes additionally to their *_DYN_LINK.

And if Boost.Config is correct: What does BOOST_SYMBOL_IMPORT (on
non-Windows) mean then? If the default visibility is set to hidden
(-fvisibility=hidden), then this macro will NOT import an exported
symbol, it will create a new, private one.
This doesn't sound intentional.

[Alexander Grund via Boost]

Am 02.02.21 um 13:44 schrieb Niall Douglas via Boost:

>> 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".

Then why do the MSVC docs say otherwise? Or what am I missing from the
posted link?

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

>> BOOST_SYMBOL_IMPORT

> 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.

I'm either misunderstanding you are this is contradiction  what you
wrote above. If the current behavior of expanding BOOST_*_DECL to either
BOOST_SYMBOL_EXPORT or BOOST_SYMBOL_IMPORT when DYN_LINK is defined is
wrong, and using BOOST_SYMBOL_VISIBLE is wrong and not using anything
does not work per the GCC docs and experience on OSX, then what is the
correct way?

[Niall Douglas via Boost]

On 02/02/2021 11:59, Andrey Semashev via Boost wrote:

> PPS: I forgot to add that recent gcc versions don't need RTTI to be
> publicly visible to be able to catch exceptions or compare type_info, as
> they now perform deep string comparison instead of address comparison.
> However, older compiler versions are still susceptible to this, and I'm
> not sure if the deep comparison behavior is used on all platforms.

My memory, which may be wrong, is that older GCCs/binutils shortly after
my patch was merged to mainline did do deep string comparisons, but it
got reverted in newer GCCs/bintuils. And clang does not do whatever GCC
does.

I may be confused, and instead am thinking that GCC does the right
thing, but clang does not.

There also may be separate treatment in each of the linkers. For
example, I believe GNU gold always does deep string comparison.

And there also may be separate treatment in the stack unwinding
implementation which calculates exception catches, than in the linker.
Oh, and don't forget the command line linker is not the shared library
process loading linker, and both those may be different here as well.

It's hard to remember details given the passage of time. I generally
assume the worst case possible, and to date I've not had a problem.

ELF is borked badly in this area. PE and MachO and indeed any other
binary format gets all this stuff right. ELF could be upgraded to not be
borked, by someone with infinite patience and better political skills
than I have.

Niall

[Peter Dimov via Boost]

Andrey Semashev wrote:
> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>
> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed by
> user;
> - BOOST_SYMBOL_VISIBLE - in any other case.

BOOST_SYMBOL_EXPORT is defined to BOOST_SYMBOL_VISIBLE on non-Windows in
order to make the third case unnecessary. Alexander suggests that on macOS
(but not Linux), it's also necessary to define BOOST_SYMBOL_IMPORT to
BOOST_SYMBOL_VISIBLE (instead of to nothing) on non-Windows. This seems
sensible.

In other words,

- BOOST_SYMBOL_EXPORT - when the library is being built;
- BOOST_SYMBOL_IMPORT - when the library is being consumed by user.

[Alexander Grund via Boost]

>> It does not because the symbols to be exported are (supposed to be) only
>> defined in Boost.ProgramOptions library.
> BOOST_SYMBOL_VISIBLE is always default visibility, no matter what.
>
> Therefore anything in a Boost.ProgramOptions header would be made
> visible and thus exported, which equals exporting all the extern
> declarations. That, in turn, would cause ABI collision if somebody tries
> linking in a non-matching Boost, under the assumption that an internal
> Boost has been hidden from linkage (which "works", so far).

So how do you propose to setup a library such that exceptions can be
caught by consumers of that library in a portable way?

IMO the scenario you describe is impossible to support: Either
everything is hidden, then exceptions can't be caught/dynamic_cast not
used (according to docs on OSX) or it is VISIBLE, then you have
potential collisions.

[Peter Dimov via Boost]

Alexander Grund wrote:

> So how do you propose to setup a library such that exceptions can be
> caught by consumers of that library in a portable way?

All exceptions need to be BOOST_SYMBOL_VISIBLE, even in header-only
libraries.

Non-exception interface classes need to be _DECL if the library isn't
header-only.

The question is what needs to be done for exception classes that are also
_DECL for some reason (e.g. their what() member function is in the compiled
library.)

It makes sense that everything EXPORTed/IMPORTed needs to also be VISIBLE; I
can't think of any scenarios in which it doesn't need to be. So defining
BOOST_SYMBOL_IMPORT to BOOST_SYMBOL_VISIBLE on non-Windows seems sensible.
Others seem to disagree; I'm not sure what's the downside though.

[Niall Douglas via Boost]

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

> Why do individual libraries now have to deal with the Windows vs
> non-Windows case? Is there ANY Boost library which does this?

Outcome is correct on this. I think any of Dave's original libraries are
correct on this, or at least, were once.

> From the BOOST_SYMBOL_VISIBLE description: "Needed for classes that are
> not otherwise exported, but are used by RTTI". This sounds to me like
> either a BOOST_SYMBOL_EXPORT or BOOST_SYMBOL_IMPORT should work.
> Now I'm left to wonder why individual libraries must now distinguish
> between OSes additionally to their *_DYN_LINK.

I suggest reading the GCC visibility guide again. You appear to be
conflating how ELF does things, and how everything-except-ELF does things.

We have a hacky, poor quality, map of everything-except-ELF DLL
import/export semantics onto the much inferior, and quite broken, ELF
semantics. It works well enough, most of the time, if you are 100%
perfect in avoiding all the poorly documented footguns. But let me be
clear, what Windows, MachO, everything-except-ELF do is the *correct*
design. What ELF does is a travesty to writing production quality code.
But things are what they are.

I completely agree that this sucks, and you'd think fifteen years of
footgun pain would make someone fix it. But solving this is a political,
not engineering, problem and it went beyond my personal reserves of
patience and forebaring.

> And if Boost.Config is correct: What does BOOST_SYMBOL_IMPORT (on
> non-Windows) mean then? If the default visibility is set to hidden
> (-fvisibility=hidden), then this macro will NOT import an exported
> symbol, it will create a new, private one.
> This doesn't sound intentional.

Hidden symbols are subject to much more aggressive optimisation than
default visibility symbols. So yes, this is intentional.

Niall

[Niall Douglas via Boost]

On 02/02/2021 14:09, Peter Dimov via Boost wrote:
> Andrey Semashev wrote:
>> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>>
>> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
>> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
>> by user;
>> - BOOST_SYMBOL_VISIBLE - in any other case.
>
> BOOST_SYMBOL_EXPORT is defined to BOOST_SYMBOL_VISIBLE on non-Windows in
> order to make the third case unnecessary. Alexander suggests that on
> macOS (but not Linux), it's also necessary to define BOOST_SYMBOL_IMPORT
> to BOOST_SYMBOL_VISIBLE (instead of to nothing) on non-Windows. This
> seems sensible.

MachO has the same symbol visibility semantics as PE i.e. correct.

For pure compatibility with Linux reasons, Apple engineers have mapped
the GNU visibility attributes to the MachO equivalent of
__declspec(dllexport) and __declspec(dllimport). I argued with them at
the time that that was daft, but Linux compatibility without forcing
source changes was important to them.

I honestly don't know if you need to mark thrown types as dllexport in
MachO. I don't use Mac OS enough to know. But I would be very surprised
if you did, as MachO isn't broken here.

I also have anecdata that Louis found "exceptions work right over shared
libraries" on Mac OS despite them not working on Linux without
annotation. I remember him once reaching out to me to ask why.

Niall

[Alexander Grund via Boost]

Am 02.02.21 um 15:21 schrieb Niall Douglas via Boost:

> On 02/02/2021 14:06, Alexander Grund via Boost wrote:
>
>> Why do individual libraries now have to deal with the Windows vs
>> non-Windows case? Is there ANY Boost library which does this?
> Outcome is correct on this. I think any of Dave's original libraries are
> correct on this, or at least, were once.

Ok, found an exception class there:
https://github.com/boostorg/outcome/blob/15ec857dd65dfbb710d33bd567ee3f14c62009e2/include/boost/outcome/bad_access.hpp#L43

It is marked with BOOST_SYMBOL_VISIBLE, so we seem to agree, that this
is required for all user visible classes that are errors/exceptions or
virtual (and hence might be subject to dynamic_cast)
So this leaves the IMPORT/EXPORT only for classes, which are non-virtual
and have non-inline members.

Correct so far?

What about classes, which have a non-inline function, e.g. the recently
mentioned exception class with a non-inline `what()`?
Wouldn't that need to be IMPORT/EXPORT declared for Windows? And VISIBLE
for Linux (to be caught)? So would you need both here?

> Hidden symbols are subject to much more aggressive optimisation than
> default visibility symbols. So yes, this is intentional.

I'm aware of that. But I'm looking for a correct solution, i.e. one that
works across platforms. That it seemingly works on Linux seems to be
caused by a change to string comparison instead of "real" RTTI
comparison. The question is: Should it work, or has it always worked?
I mean we have one TU (in the shared lib) where the symbol is visible
and one (in the exe) where it is not. So how could the RTTI here be the
same?

[Niall Douglas via Boost]

On 02/02/2021 14:21, Peter Dimov via Boost wrote:

> It makes sense that everything EXPORTed/IMPORTed needs to also be
> VISIBLE; I can't think of any scenarios in which it doesn't need to be.
> So defining BOOST_SYMBOL_IMPORT to BOOST_SYMBOL_VISIBLE on non-Windows
> seems sensible. Others seem to disagree; I'm not sure what's the
> downside though.

The downside is that extern function symbols from other libraries then
get published.

I seem to be having difficulty communicating this to anyone here, so
here's a godbolt:

https://godbolt.org/z/jv5Y75

The extern functions thirdpartylib_*() are #include'd from a third party
library. This extern + inline linkage is as you might get from in-class
functions in a header-only class.

You can see in the disassembly that:

- thirdpartylib_hidden, which is when -fvisibility=hidden is on, and
THIRDPARTYLIB_DECL is nothing, has hidden visibility.

- thirdpartylib_nothidden, which is when -fvisibility=hidden is on, and
THIRDPARTYLIB_DECL is BOOST_SYMBOL_VISIBLE, has default visibility.

Now do nm -D of that binary:

w _ITM_deregisterTMCloneTable
w _ITM_registerTMCloneTable
0000000000201028 B __bss_start
w __cxa_finalize
w __gmon_start__
0000000000201028 D _edata
0000000000201030 B _end
0000000000000600 T _fini
0000000000000490 T _init
00000000000005c9 T thirdpartylib_nothidden

As you can see, the symbol for thirdpartylib_nothidden has leaked into
the export set for this library. thirdpartylib_nothidden ought not to be
exported from this library, unless it has typeinfo which needs to be
visible.

Meanwhile thirdpartylib_hidden is NOT leaked from the exported symbols
of this library, and is instead local only:

0000000000200e60 d _DYNAMIC
0000000000201000 d _GLOBAL_OFFSET_TABLE_
w _ITM_deregisterTMCloneTable
w _ITM_registerTMCloneTable
00000000000006f8 r __FRAME_END__
000000000000060c r __GNU_EH_FRAME_HDR
0000000000201028 d __TMC_END__
0000000000201028 B __bss_start
w __cxa_finalize
0000000000000570 t __do_global_dtors_aux
0000000000200e58 t __do_global_dtors_aux_fini_array_entry
0000000000201020 d __dso_handle
0000000000200e50 t __frame_dummy_init_array_entry
w __gmon_start__
0000000000201028 D _edata
0000000000201030 B _end
0000000000000600 T _fini
0000000000000490 T _init
0000000000201028 b completed.7698
00000000000004e0 t deregister_tm_clones
00000000000005b0 t frame_dummy
0000000000000520 t register_tm_clones
00000000000005d8 t test
00000000000005ba t thirdpartylib_hidden
00000000000005c9 T thirdpartylib_nothidden

There is a small 't' instead of a capital 'T', indicating the symbol is
local only.

Niall

[Niall Douglas via Boost]

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

> It is marked with BOOST_SYMBOL_VISIBLE, so we seem to agree, that this
> is required for all user visible classes that are errors/exceptions or
> virtual (and hence might be subject to dynamic_cast)

You're right, in the very strict sense, that any vptr containing class
would need to be BOOST_SYMBOL_VISIBLE to ensure dynamic_cast works.

Far better though, in my opinion, to replace all dynamic_cast with
static_cast, and don't mark all vptr containing classes as always visible.

This is because there are real problems with link times, ABI collision
and poor codegen with BOOST_SYMBOL_VISIBLE. Those impact all users far
more frequently than dynamic_cast, which nobody should be using in any case.

As I've already mentioned, if you really want dynamic_cast to always
work, don't use -fvisibility=hidden. Easy.

>> Hidden symbols are subject to much more aggressive optimisation than
>> default visibility symbols. So yes, this is intentional.
>
> I'm aware of that. But I'm looking for a correct solution, i.e. one that
> works across platforms. That it seemingly works on Linux seems to be
> caused by a change to string comparison instead of "real" RTTI
> comparison. The question is: Should it work, or has it always worked?
> I mean we have one TU (in the shared lib) where the symbol is visible
> and one (in the exe) where it is not. So how could the RTTI here be the
> same?

I don't think it is for library authors to force semantics onto users in
this area.

If an end user asks for -fvisibility=hidden, they opt into everything
that entails, footguns and all. All the library author can do is a best
effort to not screw things up within their library if end users enable that.

Most Boost libraries clearly document whether they depend on RTTI or
not. If they do, you can't use either -fvisibility=hidden nor -fno-rtti.
It would be super nice if Boost.Build automatically detected -fno-rtti
and -fvisibility=hidden and simply did not compile those libraries which
claim they require RTTI, but we need that metadata json thingy working
first.

[Peter Dimov via Boost]

Alexander Grund wrote:

> What about classes, which have a non-inline function, e.g. the recently
> mentioned exception class with a non-inline `what()`?
> Wouldn't that need to be IMPORT/EXPORT declared for Windows? And VISIBLE
> for Linux (to be caught)? So would you need both here?

I suspect that for exported exception classes the right thing to do would
indeed be to declare them both _DECL and _VISIBLE. _EXPORT mapping to
_VISIBLE is kind of a hack (that often does the right thing) but it's not
enough here.

However, repeating the visibility attribute may be an error.

[Peter Dimov via Boost]

Niall Douglas wrote:

> So, as I already said, don't do that. Only symbols whose typeinfos need to
> be seen during RTTI lookup should always be BOOST_SYMBOL_VISIBLE. That is
> generally types thrown as exceptions, and anything else subject to
> dynamic_cast.

One problem here is that the library doesn't know which of its types are
subject to dynamic_cast in user code.

[Peter Dimov via Boost]

Niall Douglas wrote:

> I seem to be having difficulty communicating this to anyone here, so
> here's a godbolt:
>
> https://godbolt.org/z/jv5Y75

But you've provided a definition for thirdpartylib_nothidden. This is not
the case under discussion. The case under discussion is making
BOOST_SYMBOL_IMPORT imply _VISIBLE, which translates to a default visibility
attribute on the _declaration_ of thirdpartylib_nothidden.

[Peter Dimov via Boost]

E.g. https://godbolt.org/z/vM6Gxd

[Niall Douglas via Boost]

On 02/02/2021 15:01, Peter Dimov via Boost wrote:
> Niall Douglas wrote:
>
>> I seem to be having difficulty communicating this to anyone here, so
>> here's a godbolt:
>>
>> https://godbolt.org/z/jv5Y75
>
> But you've provided a definition for thirdpartylib_nothidden. This is
> not the case under discussion. The case under discussion is making
> BOOST_SYMBOL_IMPORT imply _VISIBLE, which translates to a default
> visibility attribute on the _declaration_ of thirdpartylib_nothidden.

If the Boost library where BOOST_XXX_DECL = BOOST_SYMBOL_VISIBLE is
being applied has public headers which never, ever provide an
implementation for any function, then BOOST_XXX_DECL =
BOOST_SYMBOL_VISIBLE would not cause export of third party symbols in
consuming libraries.

I would be surprised if any such C++ library, let alone Boost library,
exists, however.

I would point out that if Boost.ProgramOptions fixes its exception types
not being BOOST_SYMBOL_VISIBLE, the principle problem the OP reported
goes away without further changes.

Changing the ABI signature of the entire of Boost I think is a radical
step needing public announcement for at least two Boost major releases
before implementation.

Niall

[Andrey Semashev via Boost]

On 2/2/21 5:05 PM, Niall Douglas via Boost wrote:
> On 02/02/2021 13:02, Andrey Semashev via Boost wrote:
>> On 2/2/21 3:34 PM, Niall Douglas via Boost wrote:
>>> On 02/02/2021 11:51, Andrey Semashev via Boost wrote:
>>>
>>>> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>>>>
>>>> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
>>>> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
>>>> by user;
>>>> - BOOST_SYMBOL_VISIBLE - in any other case.
>>>
>>> This causes Boost.ProgramOptions symbols to be exported in any shared
>>> object or executable which links in Boost.ProgramOptions i.e. you
>>> reexport the same symbols.
>>
>> It does not because the symbols to be exported are (supposed to be) only
>> defined in Boost.ProgramOptions library.
>
> BOOST_SYMBOL_VISIBLE is always default visibility, no matter what.

Right. But it applies to the symbol definition. If there is no
definition then it has no effect.

> Therefore anything in a Boost.ProgramOptions header would be made
> visible and thus exported, which equals exporting all the extern
> declarations. That, in turn, would cause ABI collision if somebody tries
> linking in a non-matching Boost, under the assumption that an internal
> Boost has been hidden from linkage (which "works", so far).

Linking non-matching Boost versions is not supported and should be
assumed to not work, ever. Public visibility or not. Therefore, this is
not a valid use case.

[Andrey Semashev via Boost]

On 2/2/21 5:09 PM, Peter Dimov via Boost wrote:
> Andrey Semashev wrote:
>> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>>
>> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
>> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
>> by user;
>> - BOOST_SYMBOL_VISIBLE - in any other case.
>
> BOOST_SYMBOL_EXPORT is defined to BOOST_SYMBOL_VISIBLE on non-Windows in
> order to make the third case unnecessary.

Oh, ok.

> Alexander suggests that on
> macOS (but not Linux), it's also necessary to define BOOST_SYMBOL_IMPORT
> to BOOST_SYMBOL_VISIBLE (instead of to nothing) on non-Windows. This
> seems sensible.
>
> In other words,
>
> - BOOST_SYMBOL_EXPORT - when the library is being built;
> - BOOST_SYMBOL_IMPORT - when the library is being consumed by user.

Ok.

[Alexander Grund via Boost]

> Far better though, in my opinion, to replace all dynamic_cast with
> static_cast, and don't mark all vptr containing classes as always visible.
>
> This is because there are real problems with link times, ABI collision
> and poor codegen with BOOST_SYMBOL_VISIBLE. Those impact all users far
> more frequently than dynamic_cast, which nobody should be using in any case.

I'll ignore the controversial last sentence for the sake of the discussion.

The scope for BOOST_SYMBOL_VISIBLE (or the fixed _DECL) is narrow: It is
(according to my suggestion) to be applied to classes which the user may
catch or dynamic_cast.
Everything else (i.e. most of Boost) stays hidden with all benefits.

> I don't think it is for library authors to force semantics onto users in
> this area.
>
> If an end user asks for -fvisibility=hidden, they opt into everything
> that entails, footguns and all. All the library author can do is a best

> effort to not screw things up within their library if end users enable that.#

He opted into exporting/making visible only classes which need this.
And classes marked with BOOST_SYMBOL_IMPORT likely do need this.
"Likely" because of the already mentioned difference between ELF and PE.

By not declaring any class which may be caught or RTTI used (through
dynamic_cast) as VISIBLE we FORCE the user to not use -fvisibility=hidden
IMO this is unacceptable as there is no opt-out or partial change or
anything the user can do to make this work.

> Most Boost libraries clearly document whether they depend on RTTI or
> not. If they do, you can't use either -fvisibility=hidden nor -fno-rtti.

The Boost library does not depend on RTTI, the interaction between the
user library and the boost library does.

[Andrey Semashev via Boost]

On 2/2/21 5:06 PM, Alexander Grund via Boost wrote:
>
>> This means that BOOST_PROGRAM_OPTIONS_DECL is incorrectly defined.
>>
> <snip>
>>
>> So, the bottom line, BOOST_PROGRAM_OPTIONS_DECL must be defined to:
>>
>> - BOOST_SYMBOL_EXPORT - on Windows, when the library is being built;
>> - BOOST_SYMBOL_IMPORT - on Windows, when the library is being consumed
>> by user;
>> - BOOST_SYMBOL_VISIBLE - in any other case.
>>
>> Then you can use BOOST_PROGRAM_OPTIONS_DECL to mark methods and
>> classes that you want to be exported/imported from the library. If a
>> particular type does not need to be exported/imported, but whose RTTI
>> must still be accessible across library boundaries, mark that type
>> with BOOST_SYMBOL_VISIBLE, not BOOST_PROGRAM_OPTIONS_DECL. You can
>> still use BOOST_PROGRAM_OPTIONS_DECL to export/import select methods
>> of that type, if needed.
>>
>> PS: I remember someone saying that on recent Windows
>> dllexport/dllimport attributes are no longer necessary. I don't quite
>> know how that works, but for backward compatibility we should probably
>> keep using BOOST_SYMBOL_EXPORT/BOOST_SYMBOL_IMPORT as described above.
>
> I thought that BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT are exactly
> for this: You define *_DECL to *_EXPORT when building the library and
> *_IMPORT otherwise (and <empty> for static linking)

You may want to use BOOST_SYMBOL_VISIBLE for static linking. You
definitely want to use BOOST_SYMBOL_VISIBLE to export RTTI, even for
static linking.

> Why do individual libraries now have to deal with the Windows vs
> non-Windows case? Is there ANY Boost library which does this?

Turns out, libraries don't need to distinguish Windows from non-Windows.
As pointed out by Peter, BOOST_SYMBOL_EXPORT already expands to
BOOST_SYMBOL_VISIBLE on non-Windows.

> From the BOOST_SYMBOL_VISIBLE description: "Needed for classes that are
> not otherwise exported, but are used by RTTI". This sounds to me like
> either a BOOST_SYMBOL_EXPORT or BOOST_SYMBOL_IMPORT should work.
> Now I'm left to wonder why individual libraries must now distinguish
> between OSes additionally to their *_DYN_LINK.
>
> And if Boost.Config is correct: What does BOOST_SYMBOL_IMPORT (on
> non-Windows) mean then? If the default visibility is set to hidden
> (-fvisibility=hidden), then this macro will NOT import an exported
> symbol, it will create a new, private one.
> This doesn't sound intentional.

BOOST_SYMBOL_IMPORT doesn't need to expand to BOOST_SYMBOL_VISIBLE when
the marked symbols are only defined in the library, which is the case
normally. When the user consumes the library, no markup is needed as the
compiler will simply leave unresolved references to those symbols, to be
resolved by the linker.
Basically, BOOST_SYMBOL_IMPORT is only there for compatibility with
Windows.


When you want to export an exception, my suggestion would be to mark the
exception class with BOOST_SYMBOL_VISIBLE and mark individual methods
you want to export with BOOST_*_DECL. As I said before, marking the
whole class with BOOST_*_DECL provides no benefits and can cause problems.

[Andrey Semashev via Boost]

On 2/2/21 6:12 PM, Niall Douglas via Boost wrote:
> On 02/02/2021 15:01, Peter Dimov via Boost wrote:
>> Niall Douglas wrote:
>>
>>> I seem to be having difficulty communicating this to anyone here, so
>>> here's a godbolt:
>>>
>>> https://godbolt.org/z/jv5Y75
>>
>> But you've provided a definition for thirdpartylib_nothidden. This is
>> not the case under discussion. The case under discussion is making
>> BOOST_SYMBOL_IMPORT imply _VISIBLE, which translates to a default
>> visibility attribute on the _declaration_ of thirdpartylib_nothidden.
>
> If the Boost library where BOOST_XXX_DECL = BOOST_SYMBOL_VISIBLE is
> being applied has public headers which never, ever provide an
> implementation for any function, then BOOST_XXX_DECL =
> BOOST_SYMBOL_VISIBLE would not cause export of third party symbols in
> consuming libraries.
>
> I would be surprised if any such C++ library, let alone Boost library,
> exists, however.

The practice of having a declaration marked with BOOST_*_DECL in a
header and a definition, also marked with BOOST_*_DECL, in a separate
.cpp file is ubiquitous for any separately compiled Boost library.

[Alexander Grund via Boost]

>> Why do individual libraries now have to deal with the Windows vs
>> non-Windows case? Is there ANY Boost library which does this?
>
> Turns out, libraries don't need to distinguish Windows from
> non-Windows. As pointed out by Peter, BOOST_SYMBOL_EXPORT already
> expands to BOOST_SYMBOL_VISIBLE on non-Windows.

For classes (exceptions and virtual ones) defined in the header
BOOST_SYMBOL_VISIBLE must be used.

Peter and I found a simple way to reproduce the behavior I was seeing on
Mac: Use libc++
`./b2 link=shared libs/program_options/test/ toolset=clang
stdlib=libc++` is enough to reproduce the problem.

I'm actually surprised that this hasn't come up before. No tests for all
boost with libc++?

>>  From the BOOST_SYMBOL_VISIBLE description: "Needed for classes that
>> are not otherwise exported, but are used by RTTI". This sounds to me
>> like either a BOOST_SYMBOL_EXPORT or BOOST_SYMBOL_IMPORT should work.
>> Now I'm left to wonder why individual libraries must now distinguish
>> between OSes additionally to their *_DYN_LINK.
>>
>> And if Boost.Config is correct: What does BOOST_SYMBOL_IMPORT (on
>> non-Windows) mean then? If the default visibility is set to hidden
>> (-fvisibility=hidden), then this macro will NOT import an exported
>> symbol, it will create a new, private one.
>> This doesn't sound intentional.
>
> BOOST_SYMBOL_IMPORT doesn't need to expand to BOOST_SYMBOL_VISIBLE
> when the marked symbols are only defined in the library, which is the
> case normally. When the user consumes the library, no markup is needed
> as the compiler will simply leave unresolved references to those
> symbols, to be resolved by the linker.
>  Basically, BOOST_SYMBOL_IMPORT is only there for compatibility with
> Windows.

Agreed with the first part.
But quite many are defined in headers, especially the exceptions, so
there it is required.
Also for exceptions with methods implemented in the cpp file you'd need
(currently) IMPORT and VISIBLE

[Andrey Semashev via Boost]

On 2/2/21 7:09 PM, Alexander Grund via Boost wrote:
>
> Peter and I found a simple way to reproduce the behavior I was seeing on
> Mac: Use libc++
> `./b2 link=shared libs/program_options/test/ toolset=clang
> stdlib=libc++` is enough to reproduce the problem.
>
> I'm actually surprised that this hasn't come up before. No tests for all
> boost with libc++?

I suspect, Mac OS (more precisely, its linker) is the crucial part, not
libc++. clang+libc++ on Linux is relatively well tested (or at least
used to be, when Travis was more usable), but Mac OS is less tested
because of slow turnaround.

>> BOOST_SYMBOL_IMPORT doesn't need to expand to BOOST_SYMBOL_VISIBLE
>> when the marked symbols are only defined in the library, which is the
>> case normally. When the user consumes the library, no markup is needed
>> as the compiler will simply leave unresolved references to those
>> symbols, to be resolved by the linker.
>>  Basically, BOOST_SYMBOL_IMPORT is only there for compatibility with
>> Windows.
> Agreed with the first part.
> But quite many are defined in headers, especially the exceptions, so
> there it is required.

For classes defined in a header, neither EXPORT nor IMPORT are needed.
Only VISIBLE is, if you need publicly visible RTTI.

[Peter Dimov via Boost]

Andrey Semashev вроте:

> I suspect, Mac OS (more precisely, its linker) is the crucial part, not
> libc++. clang+libc++ on Linux is relatively well tested (or at least
used to be, when Travis was more usable), but Mac OS is less tested because
of slow turnaround.

Nope. libc++ on Linux also fails. Apparently, libstdc++ masks the error
because it uses strcmp (even for g++ 4.4 which is the earliest I test on
Travis.)

[Alexander Grund via Boost]

Am 02.02.21 um 17:25 schrieb Andrey Semashev via Boost:

> On 2/2/21 7:09 PM, Alexander Grund via Boost wrote:
>>
>> Peter and I found a simple way to reproduce the behavior I was seeing
>> on Mac: Use libc++
>> `./b2 link=shared libs/program_options/test/ toolset=clang
>> stdlib=libc++` is enough to reproduce the problem.
>>
>> I'm actually surprised that this hasn't come up before. No tests for
>> all boost with libc++?
>
> I suspect, Mac OS (more precisely, its linker) is the crucial part,
> not libc++. clang+libc++ on Linux is relatively well tested (or at
> least used to be, when Travis was more usable), but Mac OS is less
> tested because of slow turnaround.

The above command which tests Boost.ProgramOptions using clang + libc++
on Linux shows the same failure I see on OSX due to using DECL instead
of VISIBLE on the exception class defined in the header. So this setting
is obviously untested, which I find odd.

IIRC OSX uses libc++ too which explains the correlation.

>>> BOOST_SYMBOL_IMPORT doesn't need to expand to BOOST_SYMBOL_VISIBLE
>>> when the marked symbols are only defined in the library, which is
>>> the case normally. When the user consumes the library, no markup is
>>> needed as the compiler will simply leave unresolved references to
>>> those symbols, to be resolved by the linker.
>>>  Basically, BOOST_SYMBOL_IMPORT is only there for compatibility with
>>> Windows.
>> Agreed with the first part.
>> But quite many are defined in headers, especially the exceptions, so
>> there it is required.
>
> For classes defined in a header, neither EXPORT nor IMPORT are needed.
> Only VISIBLE is, if you need publicly visible RTTI.

Ok great, so we have a few actual bugs now: Not using VISIBLE for those.

Next issue are classes declared in the header and defined in a cpp file
which should be (able to be) caught by consumers. Due to the definition
in the CPP I think they need the DECL (->EXPORT/IMPORT) but also VISIBLE.

For comparison again: CMake defines the IMPORT macro to VISIBLE, so
that's a datapoint at least:
https://github.com/Kitware/CMake/blob/36bb0e32d7e3976eed424078cf5cac459651f0f1/Modules/GenerateExportHeader.cmake#L309-L311

[Andrey Semashev via Boost]

On 2/2/21 7:40 PM, Peter Dimov via Boost wrote:
> Andrey Semashev вроте:
>
>> I suspect, Mac OS (more precisely, its linker) is the crucial part,
>> not libc++. clang+libc++ on Linux is relatively well tested (or at least
> used to be, when Travis was more usable), but Mac OS is less tested
> because of slow turnaround.
>
> Nope. libc++ on Linux also fails. Apparently, libstdc++ masks the error
> because it uses strcmp (even for g++ 4.4 which is the earliest I test on
> Travis.)

Am I correct that BOOST_PROGRAM_OPTIONS_DECL is defined to
BOOST_SYMBOL_IMPORT, which expands to nothing, and it breaks when
Boost.ProgramOptions library throws an exception marked with it?

I'm asking because I have a similar setup in Boost.Filesystem, and it is
tested with clang+libc++ on Linux, and the tests passed last time I checked.

One potential problem that I noticed is that if an exception is marked
with BOOST_*_DECL and static linking is enabled, BOOST_*_DECL is empty
and the exception RTTI is left hidden. This can be a problem if the user
links with Boost statically (e.g. wraps it into a library of his own)
and then the exception crosses the library boundary.

[Peter Dimov via Boost]

Andrey Semashev wrote:

> Am I correct that BOOST_PROGRAM_OPTIONS_DECL is defined to
> BOOST_SYMBOL_IMPORT,

Yes:
https://github.com/boostorg/program_options/blob/0414abe3f4cf3bb617b80f2e3fb655b0fd8c1ddb/include/boost/program_options/config.hpp#L42

> which expands to nothing, and it breaks when Boost.ProgramOptions library
> throws an exception marked with it?

Yes:
https://github.com/boostorg/program_options/blob/0414abe3f4cf3bb617b80f2e3fb655b0fd8c1ddb/include/boost/program_options/errors.hpp#L47

I haven't verified this failure yet, but Alexander reports that this is what
happens.

(In this specific case, the exceptions are entirely header-only, so they
should have been just marked as VISIBLE. It's not clear what happens when
they have compiled parts, e.g. what().)

[Alexander Grund via Boost]

> Am I correct that BOOST_PROGRAM_OPTIONS_DECL is defined to
> BOOST_SYMBOL_IMPORT, which expands to nothing, and it breaks when
> Boost.ProgramOptions library throws an exception marked with it?
>
> I'm asking because I have a similar setup in Boost.Filesystem, and it
> is tested with clang+libc++ on Linux, and the tests passed last time I
> checked.

I just checked that and yes, the setup is the same and yes tests pass.

Surprised by that I checked the difference: The destructor is
implemented in the cpp file (in fact the whole class is, which is why
DECL is the right choice here).
When I change the MWE in Boost.throw_exception (for example)  so that
the class is marked DECL then the test fails. Implementing the
destructor in the cpp file makes it work.

> One potential problem that I noticed is that if an exception is marked
> with BOOST_*_DECL and static linking is enabled, BOOST_*_DECL is empty
> and the exception RTTI is left hidden. This can be a problem if the
> user links with Boost statically (e.g. wraps it into a library of his
> own) and then the exception crosses the library boundary.

True. So exceptions should be marked VISIBLE, not DECL or both if the
DECL behavior on Windows is required (i.e. the class is implemented in a
cpp file)
Ned pointed out something on Slack: VISIBLE works similar to EXPORT and
makes all inline members VISIBLE too, which might not be wanted.

So IMO conclusion is: Mark exception classes faced at users at least
VISIBLE and test with libc++.

[Peter Dimov via Boost]

Alexander Grund wrote:

> I just checked that and yes, the setup is the same and yes tests pass.
>
> Surprised by that I checked the difference: The destructor is implemented
> in the cpp file (in fact the whole class is, which is why
DECL is the right choice here).

...

> So IMO conclusion is: Mark exception classes faced at users at least
> VISIBLE and test with libc++.

The conclusion based on this data is rather, use VISIBLE instead of DECL for
exception classes implemented entirely in the header. Which is intuitively
the right thing to do anyway, as there's nothing to import or export.

So it seems that the fault indeed lies in the library rather than in our
definition of the macros.