[config] GCC symbol visibility across shared libraries
There are several outstanding tickets (2114, 2309, 3697) dealing with GCC symbol visibility across shared libraries (DSO's on UNIX-like systems, DLL's on Windows-like systems). A particular problem that is affecting several Boost libraries (system, filesystem, threads, etc) is that with compiler switch -fvisibility=hidden an exception thrown in one library cannot be caught in another library or in the main program. For more background, see http://gcc.gnu.org/wiki/Visibility Ticket 2114 provided patches to fix the problem, and work started on getting them ready for trunk. But that work seems to have stalled. See http://svn.boost.org/trac/boost/ticket/2114 What needs to be done to restart this effort? It would be better is discussion could occur on this list, rather than as comments attached to the ticket. That will ensure more eyeballs look at this. --Beman
On Wed, May 5, 2010 at 6:44 AM, Beman Dawes <bdawes@acm.org> wrote:
Ticket 2114 provided patches to fix the problem, and work started on getting them ready for trunk. But that work seems to have stalled. See http://svn.boost.org/trac/boost/ticket/2114
What needs to be done to restart this effort?
It would be better is discussion could occur on this list, rather than as comments attached to the ticket. That will ensure more eyeballs look at this.
There were a couple of discussions on the list before the discussion moved to the ticket: http://thread.gmane.org/gmane.comp.lib.boost.devel/183550 http://thread.gmane.org/gmane.comp.lib.boost.devel/177304 But they also stalled :-( Best, Stjepan
Hi Beman, On Wednesday, 5. May 2010 15:44:27 you wrote:
There are several outstanding tickets (2114, 2309, 3697) dealing with GCC symbol visibility across shared libraries (DSO's on UNIX-like systems, DLL's on Windows-like systems).
A particular problem that is affecting several Boost libraries (system, filesystem, threads, etc) is that with compiler switch -fvisibility=hidden an exception thrown in one library cannot be caught in another library or in the main program.
For more background, see http://gcc.gnu.org/wiki/Visibility
Ticket 2114 provided patches to fix the problem, and work started on getting them ready for trunk. But that work seems to have stalled. See http://svn.boost.org/trac/boost/ticket/2114
What needs to be done to restart this effort?
Well, your mail ;-)) I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)... The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT for Windows and gcc > 4. Those lack the needed test cases at the moment (lack of time on my side). The patch replaces __declspec(dllexport) with BOOST_SYMBOL_EXPORT __declspec(dllimport) with BOOST_SYMBOL_IMPORT and removes visibility declarations from forward declarations in order to eliminate gcc warnings. msvc does not care, but does only need visibility information when the class in declared. Tested with - gcc-4.3, 4.4, 4.5 - msvc 9.0 I can do a msvc 10.0 run if needed.
It would be better is discussion could occur on this list, rather than as comments attached to the ticket. That will ensure more eyeballs look at this.
Yes, please take a careful look at the patch. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)...
Your patch has been VERY helpful, and I've used it as the starting point! Thanks!
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default"))) The need for this macro arises in header only classes like this: class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... }; If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context. If you don't decorate my_exception at all, VC++ is happy, but GCC shared libraries compiled with -fvisibility=hidden can't successfully throw exceptions up to callers outside the shared library. The only way around that I can see is to use a separate macro that is only defined for GCC. In addition to that change, I've moved the definitions from win32.hpp into the specific compilers involved. These visibility macros are compiler extensions to C++, and are starting to show up in platforms other than Windows. I've also edited the docs changes somewhat. So far, I've only made changes to Boost.System. See the attached. (I've also made a lot of tweaks to Boost.System tests, but those are of limited interest so aren't included in the attached diff.) Unless there are objections, I'll commit these to trunk, and wait until tests have cycled for several days before doing anything with other libraries. We will want to alert developers of other libraries, as some of them will prefer to make their own changes. Others will be happy if someone else makes the changes. Comments appreciated! --Beman
Beman Dawes wrote:
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this:
class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... };
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context.
I don't understand the problem in that case. I mark classes with __declspec(dllexport). Is it that VC's linker notices that there are no symbols in the DLL for a header-only class and complains?
If you don't decorate my_exception at all, VC++ is happy, but GCC shared libraries compiled with -fvisibility=hidden can't successfully throw exceptions up to callers outside the shared library. The only way around that I can see is to use a separate macro that is only defined for GCC.
Given what you've asserted, I think BOOST_SYMBOL_VISIBLE is reasonable. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Wed, May 19, 2010 at 9:55 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Beman Dawes wrote:
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this:
class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... };
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context.
I don't understand the problem in that case. I mark classes with __declspec(dllexport). Is it that VC's linker notices that there are no symbols in the DLL for a header-only class and complains?
At the least, you this warning: c:\boost\trunk\boost\system\system_error.hpp(26) : warning C4275: non dll-interface class 'std::runtime_error' used as base for dll-interface class 'boost::system::system_error' But for one of the Boost.System tests, VC++ gets so confused that it fails to inline the functions defined in the class, and you get a bunch of unresolved externals. --Beman
Beman Dawes wrote:
On Wed, May 19, 2010 at 9:55 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Beman Dawes wrote:
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this:
class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... };
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context.
I don't understand the problem in that case.
At the least, you this warning:
c:\boost\trunk\boost\system\system_error.hpp(26) : warning C4275: non dll-interface class 'std::runtime_error' used as base for dll-interface class 'boost::system::system_error'
I hate that warning!
But for one of the Boost.System tests, VC++ gets so confused that it fails to inline the functions defined in the class, and you get a bunch of unresolved externals.
Wow! Let me get this straight, then. Since the class is header only, it needn't be exported from a DLL, hence using dllexport is wrongheaded for MSVC. However, GCC's -fvisibility=hidden interferes because of the need to explicitly mark exception types as visible. Have I got things right so far? Is there any other case in which a symbol must be marked visible explicitly where BOOST_SYMBOL_EXPORT is not appropriate? If not, BOOST_SYMBOL_VISIBLE is the wrong name. It should be BOOST_VISIBLE_EXCEPTION_TYPE or something like that to be clear that it is for use with exception types only. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Thu, May 20, 2010 at 7:23 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Beman Dawes wrote:
On Wed, May 19, 2010 at 9:55 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Beman Dawes wrote:
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this:
class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... };
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context.
I don't understand the problem in that case.
At the least, you this warning:
c:\boost\trunk\boost\system\system_error.hpp(26) : warning C4275: non dll-interface class 'std::runtime_error' used as base for dll-interface class 'boost::system::system_error'
I hate that warning!
But for one of the Boost.System tests, VC++ gets so confused that it fails to inline the functions defined in the class, and you get a bunch of unresolved externals.
Wow!
Yeah, this is really bad. My guess is that this is an example of the case a couple of folks mentioned could require decorating members with declspec. Much better to not decorate at all for VC++ IMO.
Let me get this straight, then. Since the class is header only, it needn't be exported from a DLL, hence using dllexport is wrongheaded for MSVC. However, GCC's -fvisibility=hidden interferes because of the need to explicitly mark exception types as visible. Have I got things right so far?
Yes, but it isn't just exception types.
Is there any other case in which a symbol must be marked visible explicitly where BOOST_SYMBOL_EXPORT is not appropriate?
IIUC, what is actually at stake is whether RTTI info is visible. So it apparently also impacts things like static members of a class template and certain dynamic_casts. See http://gcc.gnu.org/wiki/Visibility, particularly the section "Problems with C++ exceptions (please read!)". But be aware that I'm just feeling my way through this - I'm not primarily a UNIX, Posix, or GCC programmer so don't have even superficial knowledge of how shared libraries work in that environment.
If not, BOOST_SYMBOL_VISIBLE is the wrong name. It should be BOOST_VISIBLE_EXCEPTION_TYPE or something like that to be clear that it is for use with exception types only.
Apparently the problem is wider that just exception types:-) I'm not wedded to BOOST_SYMBOL_VISIBLE. It seemed to work with BOOST_SYMBOL_EXPORT/IMPORT, but I'm not wedded to those either. But they seem appropriate, and I didn't see any reason to deviate from Jürgen's patch. Thanks for the comments, --Beman
I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)...
Your patch has been VERY helpful, and I've used it as the starting point! Thanks!
FYI, there is an open ticket on this: https://svn.boost.org/trac/boost/ticket/2114 which still requires some work (mostly tests and updated docs) before rolling out.
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this:
class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... };
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context. If you don't decorate my_exception at all, VC++ is happy, but GCC shared libraries compiled with -fvisibility=hidden can't successfully throw exceptions up to callers outside the shared library. The only way around that I can see is to use a separate macro that is only defined for GCC.
Yeh, that's a tricky one, 'cos VC++ will warn almost whatever you do. I *think* the correct thing to do for header only classes is to mark them as __declspec(dllimport) - that way if you contain or inherit from them in an exported class you won't get more warnings - the main culprit here is noncopyable which is unusable without copious warnings from VC++. Oh and I think I've had warnings when containing a shared_ptr in an exported class too... in fact come to think of it if I write an exported class like this: class BOOST_SYMBOL_EXPORT foo { private: some_boost_type member; }; Won't I have issues unless "some_boost_type" is declared visible/exported? Bare in mind that this could be practically any template declared in Boost :-( Just thinking out loud yours, John.
Hi ! On Wednesday, 19. May 2010 17:56:58 you wrote:
I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)...
Your patch has been VERY helpful, and I've used it as the starting point! Thanks!
FYI, there is an open ticket on this: https://svn.boost.org/trac/boost/ticket/2114 which still requires some work (mostly tests and updated docs) before rolling out.
That is what this thread is all about. At least the Boost.Config part. The Boost.Build magic needed to support this when building the libs will have to be overhauled, too. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
Yeh, that's a tricky one, 'cos VC++ will warn almost whatever you do. I *think* the correct thing to do for header only classes is to mark them as __declspec(dllimport) - that way if you contain or inherit from them in an exported class you won't get more warnings - the main culprit here is noncopyable which is unusable without copious warnings from VC++. Oh and I think I've had warnings when containing a shared_ptr in an exported class too... in fact come to think of it if I write an exported class like this:
class BOOST_SYMBOL_EXPORT foo { private: some_boost_type member; };
Won't I have issues unless "some_boost_type" is declared visible/exported? Bare in mind that this could be practically any template declared in Boost :-(
Just thinking out loud yours, John. _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost AMQP changed to decorating the methods themselves (and only the ones designed for consumption outside the module) for reasons similar to
John Maddock wrote: this. Suggest you talk to Steve Huston, or search the AMQP mail list archive. James
On Wed, May 19, 2010 at 3:34 PM, James Mansion <james@mansionfamily.plus.com> wrote:
John Maddock wrote:
Yeh, that's a tricky one, 'cos VC++ will warn almost whatever you do. I *think* the correct thing to do for header only classes is to mark them as __declspec(dllimport) - that way if you contain or inherit from them in an exported class you won't get more warnings - the main culprit here is noncopyable which is unusable without copious warnings from VC++. Oh and I think I've had warnings when containing a shared_ptr in an exported class too... in fact come to think of it if I write an exported class like this:
class BOOST_SYMBOL_EXPORT foo { private: some_boost_type member; };
Won't I have issues unless "some_boost_type" is declared visible/exported? Bare in mind that this could be practically any template declared in Boost :-(
Just thinking out loud yours, John. _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
AMQP changed to decorating the methods themselves (and only the ones designed for consumption outside the module) for reasons similar to this. Suggest you talk to Steve Huston, or search the AMQP mail list archive.
We have a very minor problem that apparently can be fixed by the introduction of one new config macro and will have very little impact on most Boost libraries. I'd hate to do anything that forces massive changes like decorating every member. Particularly if the only practical problem users are running into is failing to catch a library's exceptions when thrown by GCC across a DSO/DLL boundary. --Beman
On Wed, May 19, 2010 at 11:56 AM, John Maddock <boost.regex@virgin.net> wrote:
FYI, there is an open ticket on this: https://svn.boost.org/trac/boost/ticket/2114 which still requires some work (mostly tests and updated docs) before rolling out.
As Jürgen points out, that's one of the three or four open tickets we are trying to resolve.
...
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context. If you don't decorate my_exception at all, VC++ is happy, but GCC shared libraries compiled with -fvisibility=hidden can't successfully throw exceptions up to callers outside the shared library. The only way around that I can see is to use a separate macro that is only defined for GCC.
Yeh, that's a tricky one, 'cos VC++ will warn almost whatever you do. I *think* the correct thing to do for header only classes is to mark them as __declspec(dllimport) - that way if you contain or inherit from them in an exported class you won't get more warnings - the main culprit here is noncopyable which is unusable without copious warnings from VC++.
I've tried VC++ with __declspec(dllexport), __declspec(dllimport), and BOOST_SYSTEM_DECL (which will be __declspec(dllexport) or __declspec(dllimport) depending on the context. Surprisingly, __declspec(dllexport) is the only decoration that passes all Boost.System tests. But all of the decorations tested produce VC++ warning messages.
Oh and I think I've had warnings when containing a shared_ptr in an exported class too... in fact come to think of it if I write an exported class like this:
class BOOST_SYMBOL_EXPORT foo { private: some_boost_type member; };
Won't I have issues unless "some_boost_type" is declared visible/exported? Bare in mind that this could be practically any template declared in Boost :-(
While I didn't run into that particular problem, Boost.System has minimal needs. One of the advantages of introducing a separate macro is that it won't be present for most compilers, including VC++. Thus only GCC will actually get any decoration, so there will be no warnings or other unintended consequences for other compilers. --Beman
Beman Dawes wrote:
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)...
Your patch has been VERY helpful, and I've used it as the starting point! Thanks!
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this:
class BOOST_SYMBOL_VISIBLE my_exception : public std::runtime_error { ... };
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context. If you don't decorate my_exception at all, VC++ is happy, but GCC shared libraries compiled with -fvisibility=hidden can't successfully throw exceptions up to callers outside the shared library. The only way around that I can see is to use a separate macro that is only defined for GCC.
Note: some of this funtionality, or something similar, might already be found in boost/serialization/force_include.hpp Robert Ramey
On Wednesday, 19. May 2010 18:36:21 you wrote:
Beman Dawes wrote:
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
AFAICT, a third macro is needed for GCC. I've named it BOOST_SYMBOL_VISIBLE and define it as __attribute__((visibility("default")))
The need for this macro arises in header only classes like this: class BOOST_SYMBOL_VISIBLE my_exception : public
std::runtime_error { ... };
Okay, I'm not used to exceptions (legacy code...) .
If you use BOOST_SYMBOL_EXPORT, VC++ will warn because __declspec(dllexport) isn't valid in this context. If you don't decorate my_exception at all, VC++ is happy, but GCC shared libraries compiled with -fvisibility=hidden can't successfully throw exceptions up to callers outside the shared library. The only way around that I can see is to use a separate macro that is only defined for GCC.
Note: some of this funtionality, or something similar, might already be found in boost/serialization/force_include.hpp
Yes, this is be # define BOOST_USED __attribute__ ((used)) isn't it ? I've skipped most of serialization/archive due the non-standard definition of the export/import macros. And it is the only library which has a special case for Borland #if defined(__BORLANDC__) #define BOOST_SERIALIZATION_DECL(T) T __export I'm not sure if this is still needed and why you decided to use this syntax. I hope that you can shed some light on this and maybe unify those macros, too. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
Jürgen Hunold wrote:
I've skipped most of serialization/archive due the non-standard definition of the export/import macros. And it is the only library which has a special case for Borland
#if defined(__BORLANDC__) #define BOOST_SERIALIZATION_DECL(T) T __export
I'm not sure if this is still needed and why you decided to use this syntax. I hope that you can shed some light on this and maybe unify those macros, too.
I only did this as there wasn't anything else to do it. That is, I wasn't aware that there was some "standard" syntax. I don't remember much about it - it was maybe 8 years ago. But I do remember that reconciling the gcc/msvc/borland syntax was a pain and I'm sure that's what drove the syntax I settled on. Robert Ramey
Hi Robert ! On Wednesday, 19. May 2010 21:55:36 you wrote:
I only did this as there wasn't anything else to do it. That is, I wasn't aware that there was some "standard" syntax. I don't remember much about it - it was maybe 8 years ago. But I do remember that reconciling the gcc/msvc/borland syntax was a pain and I'm sure that's what drove the syntax I settled on.
Thanks, that explains everything. I have gcc/msvc to experiment and test and was not sure about the whole borland specific code. Maybe we can use the "standard" syntax in the future. I hope I can make a patch when the visibility support is in. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
On Wed, May 19, 2010 at 12:36 PM, Robert Ramey <ramey@rrsd.com> wrote:
...
Note: some of this funtionality, or something similar, might already be found in boost/serialization/force_include.hpp
Interesting... Although BOOST_USED is solving a somewhat different problem than BOOST_SYMBOL_VISIBLE, the solution follows the same pattern - introduce a new macro for the particular attribute needed, and define that macro with no value for the compilers that don't need it and/or don't recognize the attribute extension. Thanks, --Beman
Hi Beman ! On Wednesday, 19. May 2010 15:30:24 you wrote:
On Thu, May 6, 2010 at 12:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)...
Your patch has been VERY helpful, and I've used it as the starting point! Thanks!
You're welcome. I'm glad to help.
In addition to that change, I've moved the definitions from win32.hpp into the specific compilers involved. These visibility macros are compiler extensions to C++, and are starting to show up in platforms other than Windows.
The rationale for the win32.hpp was that _every_ compiler has to support ___declspec on this platform. I don't know if clang on linux supports __declspec or uses gcc __attribute. So I think win32.hpp + platform compiler setup might be the way. Or even unix.hpp with __attribute usage for all gcc compatible compilers. Else we have a high redundancy defining the same macros in every compilers header.
I've also edited the docs changes somewhat.
Thanks, writing docs is a weak point (at least for me ;-))
So far, I've only made changes to Boost.System. See the attached. (I've also made a lot of tweaks to Boost.System tests, but those are of limited interest so aren't included in the attached diff.)
looks good.
Unless there are objections, I'll commit these to trunk, and wait until tests have cycled for several days before doing anything with other libraries. We will want to alert developers of other libraries, as some of them will prefer to make their own changes. Others will be happy if someone else makes the changes.
My only objection is the redundancy introduced by moving the logic out of win32.hpp. I don't like lots of identic definitions flying around. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
On Wed, May 19, 2010 at 2:16 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
...
In addition to that change, I've moved the definitions from win32.hpp into the specific compilers involved. These visibility macros are compiler extensions to C++, and are starting to show up in platforms other than Windows.
The rationale for the win32.hpp was that _every_ compiler has to support ___declspec on this platform.
Every compiler that wants to be compatible with other current Windows compilers will support ___declspec today, but the introduction of attribute syntax in C++0x may mean changes in the future. Also, some past compilers used on Windows didn't support ___declspec, and Microsoft has changed the spelling of the syntax at least once. I also have more dislike for having to change the platform headers when compilers change than the other way around. I don't have any way to test some of the platform headers easily either.
I don't know if clang on linux supports __declspec or uses gcc __attribute. So I think win32.hpp + platform compiler setup might be the way. Or even unix.hpp with __attribute usage for all gcc compatible compilers. Else we have a high redundancy defining the same macros in every compilers header.
I'm still not convinced.
I've also edited the docs changes somewhat.
Thanks, writing docs is a weak point (at least for me ;-))
So far, I've only made changes to Boost.System. See the attached. (I've also made a lot of tweaks to Boost.System tests, but those are of limited interest so aren't included in the attached diff.)
looks good.
Unless there are objections, I'll commit these to trunk, and wait until tests have cycled for several days before doing anything with other libraries. We will want to alert developers of other libraries, as some of them will prefer to make their own changes. Others will be happy if someone else makes the changes.
My only objection is the redundancy introduced by moving the logic out of win32.hpp. I don't like lots of identic definitions flying around.
What do others think? --Beman
On Wed, May 19, 2010 at 2:16 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
...
... I've moved the definitions from win32.hpp into the specific compilers involved. These visibility macros are compiler extensions to C++, and are starting to show up in platforms other than Windows.
The rationale for the win32.hpp was that _every_ compiler has to support ___declspec on this platform. I don't know if clang on linux supports __declspec or uses gcc __attribute. So I think win32.hpp + platform compiler setup might be the way. Or even unix.hpp with __attribute usage for all gcc compatible compilers. Else we have a high redundancy defining the same macros in every compilers header.
In the end, I decide you were right about redundancy, so reverted to something much closer to your original patch. Since win32.hpp (and other platform configs) come after compiler configs, any compilers that need special handling can still do so. Thanks, --Beman
On Thu, May 6, 2010 at 7:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
Hi Beman,
[snip]
I've got a working patch set lying around. Please find it attached. Real life got in the way (as usual)...
The patch introduces two Boost.Config macros - BOOST_SYMBOL_EXPORT - BOOST_SYMBOL_IMPORT
for Windows and gcc > 4. Those lack the needed test cases at the moment (lack of time on my side). The patch replaces __declspec(dllexport) with BOOST_SYMBOL_EXPORT __declspec(dllimport) with BOOST_SYMBOL_IMPORT and removes visibility declarations from forward declarations in order to eliminate gcc warnings. msvc does not care, but does only need visibility information when the class in declared. Tested with - gcc-4.3, 4.4, 4.5 - msvc 9.0
I can do a msvc 10.0 run if needed.
It would be better is discussion could occur on this list, rather than as comments attached to the ticket. That will ensure more eyeballs look at this.
Yes, please take a careful look at the patch.
Hi Jurgen, boosters, Some comments on patch. 1) The first version of BOOST_SYMBOL_EXPORT/IMPORT macro had slightly different definition for gcc (something like this): #define BOOST_SYMBOL_EXPORT __attribute__((visibility("default")) #define BOOST_SYMBOL_IMPORT Assume that I've built boost filesystem library with hidden visibility. After that I've used boost filesystem functionality in my custom shared library foo (also build with hidden visibility). Assume also that boost filesystem was used as an implementation detail. You will never find any references to filesystem in foo's public interface. In case when __attribute__((visibility("default")) is always defined, foo's ABI contains boost filesystem's exported symbols also. In case if BOOST_SYMBOL_EXPORT/IMPORT macros defined as above, foo's ABI contains only it's own exported symbols. In my opinion, shared library ABI should not contain any implementation details. So, I would rather vote for using export/import macros defined as above. Please, correct me if I'm wrong or misunderstood something. 2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined? This will decrease #ifdef branches in libraries configuration files (config.hpp). Regards
Alexander Arhipenko
On Thu, May 6, 2010 at 7:42 PM, Jürgen Hunold wrote:
The patch replaces __declspec(dllexport) with BOOST_SYMBOL_EXPORT __declspec(dllimport) with BOOST_SYMBOL_IMPORT and removes visibility declarations from forward declarations
The first version of BOOST_SYMBOL_EXPORT/IMPORT macro had slightly different definition for gcc (something like this): #define BOOST_SYMBOL_EXPORT __attribute__((visibility("default")) #define BOOST_SYMBOL_IMPORT
Assume that I've built boost filesystem library with hidden visibility. After that I've used boost filesystem functionality in my custom shared library foo (also build with hidden visibility). Assume also that boost filesystem was used as an implementation detail. You will never find any references to filesystem in foo's public interface.
Each dynamic library should define its own import/export macro to adorn the symbols of that library that should be exported/visible. Those macros should be based upon BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT and whether the dynamic library is being built or used: #ifdef BUILDING_MYLIB # define MYLIB_EXPORT BOOST_SYMBOL_EXPORT #else # define MYLIB_EXPORT BOOST_SYMBOL_IMPORT #endif Using MYLIB_EXPORT for all public symbols in MyLib, and defining BUILDING_MYLIB only when compiling MyLib, will do what you want. Arguably, "EXPORT" in "MYLIB_EXPORT" is misleading, at least for MyLib's clients. Perhaps "MYLIB_VISIBLE" is a better name, leaving BOOST_SYMBOL_VISIBLE, or whatever it may be called, for the non-import/export cases.
2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined?
If that's not what's being done -- I haven't looked at the patch itself -- then I agree. Those macros should always be defined and each library must decide which to use as shown above. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Thu, May 20, 2010 at 10:21 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Alexander Arhipenko
On Thu, May 6, 2010 at 7:42 PM, Jürgen Hunold wrote:
The patch replaces __declspec(dllexport) with BOOST_SYMBOL_EXPORT __declspec(dllimport) with BOOST_SYMBOL_IMPORT and removes visibility declarations from forward declarations
The first version of BOOST_SYMBOL_EXPORT/IMPORT macro had slightly different definition for gcc (something like this): #define BOOST_SYMBOL_EXPORT __attribute__((visibility("default")) #define BOOST_SYMBOL_IMPORT
Assume that I've built boost filesystem library with hidden visibility. After that I've used boost filesystem functionality in my custom shared library foo (also build with hidden visibility). Assume also that boost filesystem was used as an implementation detail. You will never find any references to filesystem in foo's public interface.
Each dynamic library should define its own import/export macro to adorn the symbols of that library that should be exported/visible. Those macros should be based upon BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT and whether the dynamic library is being built or used:
#ifdef BUILDING_MYLIB # define MYLIB_EXPORT BOOST_SYMBOL_EXPORT #else # define MYLIB_EXPORT BOOST_SYMBOL_IMPORT #endif
Using MYLIB_EXPORT for all public symbols in MyLib, and defining BUILDING_MYLIB only when compiling MyLib, will do what you want.
See http://svn.boost.org/trac/boost/wiki/Guidelines/Separate for the current guidelines, which most Boost libraries seem to be following. Thus for Boost.System, the name for the macro is BOOST_SYSTEM_DECL We aren't proposing any change to that; I think both Jürgen and I were trying to make fairly minimal changes to current Boost practice and headers.
Arguably, "EXPORT" in "MYLIB_EXPORT" is misleading, at least for MyLib's clients. Perhaps "MYLIB_VISIBLE" is a better name, leaving BOOST_SYMBOL_VISIBLE, or whatever it may be called, for the non-import/export cases.
2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined?
If that's not what's being done -- I haven't looked at the patch itself -- then I agree. Those macros should always be defined and each library must decide which to use as shown above.
At the moment that isn't being proposed. I'm not against doing that, and considered always defining all three of the new macros (BOOST_SYMBOL_EXPORT/IMPORT/VISIBLE). That would imply changing all compiler config headers, so I'd need help getting them right for compilers I'm not familiar with. Let me look at the impact of that change on library code before making any final decision. --Beman
Beman Dawes wrote:
On Thu, May 20, 2010 at 10:21 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Each dynamic library should define its own import/export macro to adorn the symbols of that library that should be exported/visible. Those macros should be based upon BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT and whether the dynamic library is being built or used:
#ifdef BUILDING_MYLIB # define MYLIB_EXPORT BOOST_SYMBOL_EXPORT #else # define MYLIB_EXPORT BOOST_SYMBOL_IMPORT #endif
Using MYLIB_EXPORT for all public symbols in MyLib, and defining BUILDING_MYLIB only when compiling MyLib, will do what you want.
See http://svn.boost.org/trac/boost/wiki/Guidelines/Separate for the current guidelines, which most Boost libraries seem to be following.
The #ifdef BOOST_HAS_DECLSPEC surrounds everything, but would be false for GCC, right? I'd suggest that the BOOST_HAS_DECLSPEC check be managed by the config system so that BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT are defined appropriately (compressed into one place here): #define BOOST_SYMBOL_EXPORT #define BOOST_SYMBOL_IMPORT #define BOOST_SYMBOL_VISIBLE #ifdef BOOST_HAS_DECLSPEC # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT __declspec(dllexport) # undef BOOST_SYMBOL_IMPORT # define BOOST_SYMBOL_IMPORT __declspec(dllimport) #elif defined BOOST_HAS_VISIBILITY # undef BOOST_SYMBOL_VISIBLE # define BOOST_SYMBOL_VISIBLE __attribute__(visibility("default")) # ifdef BOOST_DEFAULT_VISIBILITY_IS_HIDDEN # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT BOOST_SYMBOL_VISIBLE # endif #endif BOOST_HAS_VISIBILITY would be undefined for most compilers, but would be defined for GCC (when __GNUC__ is defined, perhaps after some particular release, but I don't know when it was introduced). BOOST_DEFAULT_VISIBILITY_IS_HIDDEN must be defined for GCC, when __GNUC__ >= 4, but only if using -fvisiblity=hidden. I haven't found a way to detect that, so I guess it has to be assumed or defined via the command line. Then, each library would use this instead: #define BOOST_WHATEVER_DECL #if defined BOOST_ALL_DYN_LINK || defined BOOST_WHATEVER_DYN_LINK # undef BOOST_WHATEVER_DECL # ifdef BOOST_WHATEVER_SOURCE # define BOOST_WHATEVER_DECL BOOST_SYMBOL_EXPORT # else # define BOOST_WHATEVER_DECL BOOST_SYMBOL_IMPORT # endif #endif Defining BOOST_WHATEVER_SOURCE in each library source file is rather awkward, but it does work. (I prefer to define that in the build command line via makefile or whatever. You can't forget it then.)
2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined?
If that's not what's being done -- I haven't looked at the patch itself -- then I agree. Those macros should always be defined and each library must decide which to use as shown above.
At the moment that isn't being proposed. I'm not against doing that, and considered always defining all three of the new macros (BOOST_SYMBOL_EXPORT/IMPORT/VISIBLE). That would imply changing all compiler config headers, so I'd need help getting them right for compilers I'm not familiar with.
While the change I've shown is pervasive, the result is much simpler and cleaner for each library. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Thursday, 20. May 2010 17:36:54 you wrote:
Beman Dawes wrote:
We aren't proposing any change to that; I think both Jürgen and I were trying to make fairly minimal changes to current Boost practice and headers.
Right. I wanted to take as small steps as possible and don't risk to break anything.
The #ifdef BOOST_HAS_DECLSPEC surrounds everything, but would be false for GCC, right?
Therefore I replace those checks with BOOST_SYMBOL_EXPORT.
I'd suggest that the BOOST_HAS_DECLSPEC check be managed by the config system so that BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT are defined appropriately (compressed into one place here):
#define BOOST_SYMBOL_EXPORT #define BOOST_SYMBOL_IMPORT #define BOOST_SYMBOL_VISIBLE
#ifdef BOOST_HAS_DECLSPEC # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT __declspec(dllexport) # undef BOOST_SYMBOL_IMPORT # define BOOST_SYMBOL_IMPORT __declspec(dllimport) #elif defined BOOST_HAS_VISIBILITY # undef BOOST_SYMBOL_VISIBLE # define BOOST_SYMBOL_VISIBLE __attribute__(visibility("default")) # ifdef BOOST_DEFAULT_VISIBILITY_IS_HIDDEN # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT BOOST_SYMBOL_VISIBLE # endif #endif
BOOST_HAS_VISIBILITY would be undefined for most compilers, but would be defined for GCC (when __GNUC__ is defined, perhaps after some particular release, but I don't know when it was introduced).
I had the same idea, but decided to take small steps. First, get visibility in with a minimal diff, then add support to Boost.Build and then refactor. It is __GNUC_ > 4, by the way ;-))
= 4, but only if using -fvisiblity=hidden. I haven't found a way to detect that, so I guess it has to be assumed or defined via the command
BOOST_DEFAULT_VISIBILITY_IS_HIDDEN must be defined for GCC, when __GNUC__ line.
No. Visibility can always be explicitly specified. So no BOOST_DEFAULT_VISIBILITY_IS_HIDDEN macro is needed. I would simply always #define BOOST_SYMBOL_EXPORT/IMPORT to the correct values.
Then, each library would use this instead:
#define BOOST_WHATEVER_DECL #if defined BOOST_ALL_DYN_LINK || defined BOOST_WHATEVER_DYN_LINK # undef BOOST_WHATEVER_DECL # ifdef BOOST_WHATEVER_SOURCE # define BOOST_WHATEVER_DECL BOOST_SYMBOL_EXPORT # else # define BOOST_WHATEVER_DECL BOOST_SYMBOL_IMPORT # endif #endif
Defining BOOST_WHATEVER_SOURCE in each library source file is rather awkward, but it does work. (I prefer to define that in the build command line via makefile or whatever. You can't forget it then.)
Well, most (all) libraries already have it the source. It is just a matter of taste. And msvc will tell you very fast that you have no exported symbols...
While the change I've shown is pervasive, the result is much simpler and cleaner for each library.
Yes, that is the goal. But I still prefer to do small steps. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
Jürgen Hunold wrote:
Rob Stewart wrote:
#define BOOST_SYMBOL_EXPORT #define BOOST_SYMBOL_IMPORT #define BOOST_SYMBOL_VISIBLE
#ifdef BOOST_HAS_DECLSPEC # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT __declspec(dllexport) # undef BOOST_SYMBOL_IMPORT # define BOOST_SYMBOL_IMPORT __declspec(dllimport) #elif defined BOOST_HAS_VISIBILITY # undef BOOST_SYMBOL_VISIBLE # define BOOST_SYMBOL_VISIBLE __attribute__(visibility("default")) # ifdef BOOST_DEFAULT_VISIBILITY_IS_HIDDEN # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT BOOST_SYMBOL_VISIBLE # endif #endif
BOOST_HAS_VISIBILITY would be undefined for most compilers, but would be defined for GCC (when __GNUC__ is defined, perhaps after some particular release, but I don't know when it was introduced).
I had the same idea, but decided to take small steps. First, get visibility in with a minimal diff, then add support to Boost.Build and then refactor. It is __GNUC_ > 4, by the way ;-))
That's correct for BOOST_DEFAULT_VISIBILITY_IS_HIDDEN (for -fvisibility and the pragma), but the attribute was available before that.
= 4, but only if using -fvisiblity=hidden. I haven't found a way to detect that, so I guess it has to be assumed or defined via
BOOST_DEFAULT_VISIBILITY_IS_HIDDEN must be defined for GCC, when __GNUC__ the command line.
No. Visibility can always be explicitly specified. So no BOOST_DEFAULT_VISIBILITY_IS_HIDDEN macro is needed. I would simply always #define BOOST_SYMBOL_EXPORT/IMPORT to the correct values.
Excellent point. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Thu, May 20, 2010 at 11:36 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Beman Dawes wrote:
On Thu, May 20, 2010 at 10:21 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Each dynamic library should define its own import/export macro to adorn the symbols of that library that should be exported/visible. Those macros should be based upon BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT and whether the dynamic library is being built or used:
#ifdef BUILDING_MYLIB # define MYLIB_EXPORT BOOST_SYMBOL_EXPORT #else # define MYLIB_EXPORT BOOST_SYMBOL_IMPORT #endif
Using MYLIB_EXPORT for all public symbols in MyLib, and defining BUILDING_MYLIB only when compiling MyLib, will do what you want.
See http://svn.boost.org/trac/boost/wiki/Guidelines/Separate for the current guidelines, which most Boost libraries seem to be following.
The #ifdef BOOST_HAS_DECLSPEC surrounds everything,
Yes, currently. But if BOOST_SYMBOL_EXPORT/IMPORT are always defined, we can deprecate BOOST_HAS_DECLSPEC. It no longer will serve any useful purpose except in legacy code that hasn't been updated to reflect the macros always being present.
but would be false for GCC, right?
BOOST_HAS_DECLSPEC wouldn't be defined for GCC <4 (or whatever release it was they started supporting decoration, but then will be defined for releases from then on.
I'd suggest that the BOOST_HAS_DECLSPEC check be managed by the config system
If the three macros are always defined by the config system, there is no long a need for the BOOST_HAS_DECLSPEC check
so that BOOST_SYMBOL_EXPORT and BOOST_SYMBOL_IMPORT are defined appropriately (compressed into one place here):
#define BOOST_SYMBOL_EXPORT #define BOOST_SYMBOL_IMPORT #define BOOST_SYMBOL_VISIBLE
#ifdef BOOST_HAS_DECLSPEC # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT __declspec(dllexport) # undef BOOST_SYMBOL_IMPORT # define BOOST_SYMBOL_IMPORT __declspec(dllimport) #elif defined BOOST_HAS_VISIBILITY # undef BOOST_SYMBOL_VISIBLE # define BOOST_SYMBOL_VISIBLE __attribute__(visibility("default")) # ifdef BOOST_DEFAULT_VISIBILITY_IS_HIDDEN # undef BOOST_SYMBOL_EXPORT # define BOOST_SYMBOL_EXPORT BOOST_SYMBOL_VISIBLE # endif #endif
BOOST_HAS_VISIBILITY would be undefined for most compilers, but would be defined for GCC (when __GNUC__ is defined, perhaps after some particular release, but I don't know when it was introduced).
BOOST_DEFAULT_VISIBILITY_IS_HIDDEN must be defined for GCC, when __GNUC__ >= 4, but only if using -fvisiblity=hidden. I haven't found a way to detect that, so I guess it has to be assumed or defined via the command line.
Then, each library would use this instead:
#define BOOST_WHATEVER_DECL #if defined BOOST_ALL_DYN_LINK || defined BOOST_WHATEVER_DYN_LINK # undef BOOST_WHATEVER_DECL # ifdef BOOST_WHATEVER_SOURCE # define BOOST_WHATEVER_DECL BOOST_SYMBOL_EXPORT # else # define BOOST_WHATEVER_DECL BOOST_SYMBOL_IMPORT # endif #endif
Here is the code I'm experimenting with for Boost.System: // define BOOST_SYSTEM_DECL and BOOST_SYSTEM_VISIBLE #if defined(BOOST_ALL_DYN_LINK) || defined(BOOST_SYSTEM_DYN_LINK) # if defined(BOOST_SYSTEM_SOURCE) # define BOOST_SYSTEM_DECL BOOST_SYMBOL_EXPORT # else # define BOOST_SYSTEM_DECL BOOST_SYMBOL_IMPORT # endif # define BOOST_SYSTEM_VISIBLE BOOST_SYMBOL_VISIBLE #else # define BOOST_SYSTEM_DECL # define BOOST_SYSTEM_VISIBLE #endif
Defining BOOST_WHATEVER_SOURCE in each library source file is rather awkward, but it does work. (I prefer to define that in the build command line via makefile or whatever. You can't forget it then.)
Defining it source files relieves a burden from users who choose to setup their own builds. If you forget it during library development, you are quickly reminded once you start testing.
2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined?
If that's not what's being done -- I haven't looked at the patch itself -- then I agree. Those macros should always be defined and each library must decide which to use as shown above.
At the moment that isn't being proposed. I'm not against doing that, and considered always defining all three of the new macros (BOOST_SYMBOL_EXPORT/IMPORT/VISIBLE). That would imply changing all compiler config headers, so I'd need help getting them right for compilers I'm not familiar with.
While the change I've shown is pervasive, the result is much simpler and cleaner for each library.
Yes, agreed. I've now got that on my do list. --Beman
Beman Dawes wrote:
Here is the code I'm experimenting with for Boost.System:
// define BOOST_SYSTEM_DECL and BOOST_SYSTEM_VISIBLE #if defined(BOOST_ALL_DYN_LINK) || defined(BOOST_SYSTEM_DYN_LINK) # if defined(BOOST_SYSTEM_SOURCE) # define BOOST_SYSTEM_DECL BOOST_SYMBOL_EXPORT # else # define BOOST_SYSTEM_DECL BOOST_SYMBOL_IMPORT # endif # define BOOST_SYSTEM_VISIBLE BOOST_SYMBOL_VISIBLE #else # define BOOST_SYSTEM_DECL # define BOOST_SYSTEM_VISIBLE #endif
Why create your own BOOST_SYSTEM_VISIBLE? Why not just use BOOST_SYMBOL_VISIBLE for all libraries? _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Thu, May 20, 2010 at 3:05 PM, Stewart, Robert <Robert.Stewart@sig.com> wrote:
Beman Dawes wrote:
Here is the code I'm experimenting with for Boost.System:
// define BOOST_SYSTEM_DECL and BOOST_SYSTEM_VISIBLE #if defined(BOOST_ALL_DYN_LINK) || defined(BOOST_SYSTEM_DYN_LINK) # if defined(BOOST_SYSTEM_SOURCE) # define BOOST_SYSTEM_DECL BOOST_SYMBOL_EXPORT # else # define BOOST_SYSTEM_DECL BOOST_SYMBOL_IMPORT # endif # define BOOST_SYSTEM_VISIBLE BOOST_SYMBOL_VISIBLE #else # define BOOST_SYSTEM_DECL # define BOOST_SYSTEM_VISIBLE #endif
Why create your own BOOST_SYSTEM_VISIBLE? Why not just use BOOST_SYMBOL_VISIBLE for all libraries?
Good catch! With BOOST_SYMBOL_VISIBLE always defined, BOOST_SYSTEM_VISIBLE serves no purpose. Thanks, --Beman
Here's a notable distinction between GCC's visibility attribute and MSVC's __declspec: The latter is accepted on forward declarations while the latter produces a warning[*] and is otherwise ignored. That is, for portability, BOOST_SYMBOL_EXPORT must be applied to a UDT definition and not to a declaration. * foo.hpp:n: warning: type attributes are honored only at type definition _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
Hi Robert ! On Thursday, 20. May 2010 21:20:58 you wrote:
Here's a notable distinction between GCC's visibility attribute and MSVC's __declspec: The latter is accepted on forward declarations while the latter produces a warning[*] and is otherwise ignored. That is, for portability, BOOST_SYMBOL_EXPORT must be applied to a UDT definition and not to a declaration.
* foo.hpp:n: warning: type attributes are honored only at type definition
Yes, this is a known issue. My patch removes the macros from some declarations. msvc seems to work without them anyway. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
On Thu, May 20, 2010 at 9:54 AM, Alexander Arhipenko <arhipjan@gmail.com> wrote:
On Thu, May 6, 2010 at 7:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
Hi Beman,
Some comments on patch.
1) The first version of BOOST_SYMBOL_EXPORT/IMPORT macro had slightly different definition for gcc (something like this): #define BOOST_SYMBOL_EXPORT __attribute__((visibility("default")) #define BOOST_SYMBOL_IMPORT
Assume that I've built boost filesystem library with hidden visibility. After that I've used boost filesystem functionality in my custom shared library foo (also build with hidden visibility). Assume also that boost filesystem was used as an implementation detail. You will never find any references to filesystem in foo's public interface.
In case when __attribute__((visibility("default")) is always defined, foo's ABI contains boost filesystem's exported symbols also. In case if BOOST_SYMBOL_EXPORT/IMPORT macros defined as above, foo's ABI contains only it's own exported symbols.
In my opinion, shared library ABI should not contain any implementation details. So, I would rather vote for using export/import macros defined as above. Please, correct me if I'm wrong or misunderstood something.
My patch was based on Jürgen's original " #define BOOST_SYMBOL_IMPORT". We need to get input from him as to why that changed in his second patch.
2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined? This will decrease #ifdef branches in libraries configuration files (config.hpp).
I just tried that on <boost/system/config.hpp>, and it is quite a bit shorter (12 loc vs 19 loc), reads better, and is probably easier to teach. So unless someone objects, I'll follow your suggestion. Thanks, --Beman
Hi ! On Thursday, 20. May 2010 17:30:40 you wrote:
On Thu, May 20, 2010 at 9:54 AM, Alexander Arhipenko <arhipjan@gmail.com> wrote:
On Thu, May 6, 2010 at 7:42 PM, Jürgen Hunold <juergen.hunold@ivembh.de> wrote:
Hi Beman,
Some comments on patch.
1) The first version of BOOST_SYMBOL_EXPORT/IMPORT macro had slightly different definition for gcc (something like this): #define BOOST_SYMBOL_EXPORT __attribute__((visibility("default")) #define BOOST_SYMBOL_IMPORT
Assume that I've built boost filesystem library with hidden visibility. After that I've used boost filesystem functionality in my custom shared library foo (also build with hidden visibility). Assume also that boost filesystem was used as an implementation detail. You will never find any references to filesystem in foo's public interface.
Ah yes, now I remember the discussion on trac and the old ML thread.
In case when __attribute__((visibility("default")) is always defined, foo's ABI contains boost filesystem's exported symbols also. In case if BOOST_SYMBOL_EXPORT/IMPORT macros defined as above, foo's ABI contains only it's own exported symbols.
In my opinion, shared library ABI should not contain any implementation details. So, I would rather vote for using export/import macros defined as above. Please, correct me if I'm wrong or misunderstood something.
That is a tricky one. I'm not a gcc guru myself, I just use the setup which works with our own codebase. But I think that ld somehow has to know which symbols to import from boost filesystem so at least weak symbols have to be present in foo.so. This should be visible in nm output. I doubt that visibility support goes that far. And our own experience with it have been quite disappointing for the proposed decrease in librarie size and link time... We should investigate that further. I hope I can do some tests over the weekend...
My patch was based on Jürgen's original " #define BOOST_SYMBOL_IMPORT". We need to get input from him as to why that changed in his second patch.
Which one ? The second on trac ? Long ago ...
2) What do you think about having BOOST_SYMBOL_EXPORT/IMPORT always defined? This will decrease #ifdef branches in libraries configuration files (config.hpp).
I just tried that on <boost/system/config.hpp>, and it is quite a bit shorter (12 loc vs 19 loc), reads better, and is probably easier to teach. So unless someone objects, I'll follow your suggestion.
Yes, please go that way. I was lacking the courage to do such invasive changes at this stage. The goal should be to reduce the per-library code as much as possible. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! Ingenieurgesellschaft für * voice: ++49 511 262926 57 ! Verkehrs- und Eisenbahnwesen mbH * fax : ++49 511 262926 99 ! Lister Straße 15 * juergen.hunold@ivembh.de ! www.ivembh.de * * Geschäftsführer: ! Sitz des Unternehmens: Hannover * Prof. Dr.-Ing. Thomas Siefer ! Amtsgericht Hannover, HRB 56965 * PD Dr.-Ing. Alfons Radtke !
participants (8)
-
Alexander Arhipenko -
Beman Dawes -
James Mansion -
John Maddock -
Jürgen Hunold -
Robert Ramey -
Stewart, Robert -
Stjepan Rajko