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

classic Classic list List threaded Threaded
42 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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




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

smime.p7s (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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



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

smime.p7s (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 02/02/2021 10:14, Alexander Grund via Boost wrote:

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

Boost.Config is correct.

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

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

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

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

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

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

That is a bug in ProgramOptions.

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

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

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

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

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

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

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

Niall

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, 2 Feb 2021 at 11:59, Andrey Semashev via Boost
<[hidden email]> 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?

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
On 2/2/21 4:23 PM, Mathias Gaunard wrote:

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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

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





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

smime.p7s (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

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?



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

smime.p7s (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

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




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

smime.p7s (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
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.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

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