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

Alexander Grund

2 Feb 2021 2 Feb '21

8:35 a.m.

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/5bcaa933b55dd58797775d87605a6cdef4ac... 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... 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

Attachments:

smime.p7s (application/pkcs7-signature — 5.6 KB)

Show replies by date

Niall Douglas

2 Feb 2 Feb

9:22 a.m.

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

Alexander Grund

10:14 a.m.

...

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

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

Am 02.02.21 um 10:22 schrieb Niall Douglas via Boost: 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-cp... 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/36bb0e32d7e3976eed424078cf5cac459651f0...

...

...
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/5bcaa933b55dd58797775d87605a6cdef4ac... However this seems to be contradicting https://docs.microsoft.com/de-de/cpp/cpp/using-dllimport-and-dllexport-in-cp... 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

12:11 p.m.

On 2/2/21 1:14 PM, 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

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

Am 02.02.21 um 10:22 schrieb Niall Douglas via Boost: 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-cp...

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/36bb0e32d7e3976eed424078cf5cac459651f0...

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

12:44 p.m.

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

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/5bcaa933b55dd58797775d87605a6cdef4ac...

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

Alexander Grund

2:09 p.m.

...

...
Reason is that Win32 needs the declspec on classes: https://docs.microsoft.com/de-de/cpp/cpp/using-dllimport-and-dllexport-in-cp... 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

Am 02.02.21 um 13:44 schrieb Niall Douglas via Boost: 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?

Andrey Semashev

11:51 a.m.

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

11:59 a.m.

On 2/2/21 2:51 PM, Andrey Semashev wrote:

...

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.

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.

Mathias Gaunard

1:23 p.m.

On Tue, 2 Feb 2021 at 11:59, Andrey Semashev via Boost 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?

Andrey Semashev

1:30 p.m.

On 2/2/21 4:23 PM, Mathias Gaunard wrote:

...

On Tue, 2 Feb 2021 at 11:59, Andrey Semashev via Boost 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/13d8be91218950707bf38bdb1c554cf1605e4...

Niall Douglas

2:11 p.m.

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

Niall Douglas

12:34 p.m.

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

Andrey Semashev

1:02 p.m.

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.

Niall Douglas

2:05 p.m.

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

2:13 p.m.

...

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

2:21 p.m.

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

2:47 p.m.

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

Peter Dimov

3:01 p.m.

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.

Niall Douglas

3:12 p.m.

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

3:49 p.m.

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.

Andrey Semashev

3:20 p.m.

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.

Peter Dimov

2:57 p.m.

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.

Alexander Grund

2:06 p.m.

...

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.

Niall Douglas

2:21 p.m.

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

Alexander Grund

2:43 p.m.

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/15ec857dd65dfbb710d33bd567ee3f14c62...

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?

Peter Dimov

2:56 p.m.

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.

Niall Douglas

2:57 p.m.

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

Alexander Grund

3:25 p.m.

...

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

3:38 p.m.

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.

Alexander Grund

4:09 p.m.

...

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

...

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

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++? there it is required. Also for exceptions with methods implemented in the cpp file you'd need (currently) IMPORT and VISIBLE

Andrey Semashev

4:25 p.m.

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

4:40 p.m.

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

Andrey Semashev

5:06 p.m.

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

5:15 p.m.

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/0414abe3f4cf3bb617b80f2e3fb...

...

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/0414abe3f4cf3bb617b80f2e3fb... 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

6:01 p.m.

...

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

6:07 p.m.

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.

Alexander Grund

4:40 p.m.

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/36bb0e32d7e3976eed424078cf5cac459651f0...

Peter Dimov

2:09 p.m.

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.

Niall Douglas

2:27 p.m.

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

Andrey Semashev

3:24 p.m.

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.

Peter Dimov

2:05 p.m.

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.

1377

Age (days ago)

1377

Last active (days ago)

List overview

Download

40 comments

5 participants

participants (5)

Alexander Grund
Andrey Semashev
Mathias Gaunard
Niall Douglas
Peter Dimov