You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2011/03/22 16:10:48 UTC
Re: Review Request: WIP: enable -fvisibility=hidden
-fvisibility-inlines-hidden
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/
-----------------------------------------------------------
(Updated 2011-03-22 08:10:48.607304)
Review request for qpid and Andrew Stitcher.
Changes
-------
Initial stab at enabling symbol visibility, runs into warnings
Summary (updated)
-------
WIP: enable -fvisibility=hidden -fvisibility-inlines-hidden
Failing with warnings: warning: lowering visibility of ... to match its type
Google suggests warning is ok, but no apparent way to disable it.
Try build with warnings off to see if the results are good.
Diffs
-----
trunk/qpid/cpp/configure.ac 1084053
trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
trunk/qpid/cpp/src/Makefile.am 1084053
trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
Diff: https://reviews.apache.org/r/520/diff
Testing
-------
Thanks,
Alan
Re: Review Request: WIP: Enable -fvisibility=hidden,
libraries export only public symbols
Posted by Steve Huston <sh...@riverace.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/#review351
-----------------------------------------------------------
Re why we didn't do class-level exports for the original Windows work, it was mainly to avoid creeping externs where we didn't want them. In Visual C++ if you declspec(dllexport) a class, all ancestors also have to be declspec(dllexport). I think there was also an issue with Boost and templates related to this, but I can't find the info I was looking for on it so may not be right. For the Visual C++ info, also see http://msdn.microsoft.com/en-us/library/81h27t8c%28v=VS.90%29.aspx
- Steve
On 2011-03-22 15:24:43, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/520/
> -----------------------------------------------------------
>
> (Updated 2011-03-22 15:24:43)
>
>
> Review request for qpid, Andrew Stitcher and Steve Huston.
>
>
> Summary
> -------
>
> WIP: Enable -fvisibility=hidden, libraries export only public symbols
>
> Making progress. Working thru remaining link errors.
>
> - I seem to need class level EXTERN decls to resolve undefined vtable/typeinfo.
> - need to ensure throwable types have extern typeinfo - need class level EXTERN?
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/configure.ac 1084053
> trunk/qpid/cpp/include/qmf/ImportExport.h 1084053
> trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/Exception.h 1084053
> trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
> trunk/qpid/cpp/include/qpid/RangeSet.h 1084053
> trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/framing/SequenceNumber.h 1084053
> trunk/qpid/cpp/include/qpid/framing/SequenceSet.h 1084053
> trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/sys/IOHandle.h 1084053
> trunk/qpid/cpp/include/qpid/sys/posix/Mutex.h 1084053
> trunk/qpid/cpp/include/qpid/sys/posix/PrivatePosix.h 1084053
> trunk/qpid/cpp/include/qpid/types/ImportExport.h 1084053
> trunk/qpid/cpp/src/Makefile.am 1084053
> trunk/qpid/cpp/src/qmf.mk 1084053
> trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
> trunk/qpid/cpp/src/qpid/sys/rdma/ImportExport.h PRE-CREATION
> trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.h 1084053
> trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1084053
>
> Diff: https://reviews.apache.org/r/520/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: WIP: Enable -fvisibility=hidden,
libraries export only public symbols
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/
-----------------------------------------------------------
(Updated 2011-03-22 15:24:43.208433)
Review request for qpid, Andrew Stitcher and Steve Huston.
Changes
-------
Making progres...
Summary (updated)
-------
WIP: Enable -fvisibility=hidden, libraries export only public symbols
Making progress. Working thru remaining link errors.
- I seem to need class level EXTERN decls to resolve undefined vtable/typeinfo.
- need to ensure throwable types have extern typeinfo - need class level EXTERN?
Diffs
-----
trunk/qpid/cpp/configure.ac 1084053
trunk/qpid/cpp/include/qmf/ImportExport.h 1084053
trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
trunk/qpid/cpp/include/qpid/Exception.h 1084053
trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
trunk/qpid/cpp/include/qpid/RangeSet.h 1084053
trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
trunk/qpid/cpp/include/qpid/framing/SequenceNumber.h 1084053
trunk/qpid/cpp/include/qpid/framing/SequenceSet.h 1084053
trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
trunk/qpid/cpp/include/qpid/sys/IOHandle.h 1084053
trunk/qpid/cpp/include/qpid/sys/posix/Mutex.h 1084053
trunk/qpid/cpp/include/qpid/sys/posix/PrivatePosix.h 1084053
trunk/qpid/cpp/include/qpid/types/ImportExport.h 1084053
trunk/qpid/cpp/src/Makefile.am 1084053
trunk/qpid/cpp/src/qmf.mk 1084053
trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
trunk/qpid/cpp/src/qpid/sys/rdma/ImportExport.h PRE-CREATION
trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.h 1084053
trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1084053
Diff: https://reviews.apache.org/r/520/diff
Testing
-------
Thanks,
Alan
Re: Review Request: WIP: Enable -fvisibility=hidden,
libraries export only public symbols
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/
-----------------------------------------------------------
(Updated 2011-03-22 15:21:38.905747)
Review request for qpid and Andrew Stitcher.
Summary (updated)
-------
WIP: Enable -fvisibility=hidden, libraries export only public symbols
Making progress. Working thru remaining link errors.
- I seem to need class level EXTERN decls to resolve undefined vtable/typeinfo.
- need to ensure throwable types have extern typeinfo - class level EXTERN?
Diffs (updated)
-----
trunk/qpid/cpp/configure.ac 1084053
trunk/qpid/cpp/include/qmf/ImportExport.h 1084053
trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
trunk/qpid/cpp/include/qpid/Exception.h 1084053
trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
trunk/qpid/cpp/include/qpid/RangeSet.h 1084053
trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
trunk/qpid/cpp/include/qpid/framing/SequenceNumber.h 1084053
trunk/qpid/cpp/include/qpid/framing/SequenceSet.h 1084053
trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
trunk/qpid/cpp/include/qpid/sys/IOHandle.h 1084053
trunk/qpid/cpp/include/qpid/sys/posix/Mutex.h 1084053
trunk/qpid/cpp/include/qpid/sys/posix/PrivatePosix.h 1084053
trunk/qpid/cpp/include/qpid/types/ImportExport.h 1084053
trunk/qpid/cpp/src/Makefile.am 1084053
trunk/qpid/cpp/src/qmf.mk 1084053
trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
trunk/qpid/cpp/src/qpid/sys/rdma/ImportExport.h PRE-CREATION
trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.h 1084053
trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1084053
Diff: https://reviews.apache.org/r/520/diff
Testing
-------
Thanks,
Alan
Re: Review Request: WIP: enable -fvisibility=hidden
-fvisibility-inlines-hidden
Posted by Andrew Stitcher <as...@apache.org>.
> On 2011-03-22 12:14:43, Andrew Stitcher wrote:
> > Looking at the gcc documentation, it seems that the way __attribute((visibility=..)) is supposed to be used is the opposite way round to which we use in our headers (and in Visual Studio/Windows) your supposed to set a default which is more visible and then override it with less visible member functions.
> >
> > Doing it that way looks like it would avoid the warnings you are seeing (I haven't tried this though so I may have it wrong in practice)
>
> Alan Conway wrote:
> I didn't get the same impression, I'll read again. The big advantage in doing it this way is that windows does it this way and the windows folks have already defined and tested our public API for us with macros we can re-use. I also feel like it makes more sense to me to directly define the public API rather than indirectly defining it as the inverse of the non-public functions. Seems like it is more error prone to try to mark every non-public function - since that's a constantly changing set - than mark the stable set of public API functions.
I completely agree with this, just stating my impression of the gcc documentation
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/#review347
-----------------------------------------------------------
On 2011-03-22 08:10:48, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/520/
> -----------------------------------------------------------
>
> (Updated 2011-03-22 08:10:48)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Summary
> -------
>
> WIP: enable -fvisibility=hidden -fvisibility-inlines-hidden
>
> Failing with warnings: warning: lowering visibility of ... to match its type
> Google suggests warning is ok, but no apparent way to disable it.
> Try build with warnings off to see if the results are good.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/configure.ac 1084053
> trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
> trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
> trunk/qpid/cpp/src/Makefile.am 1084053
> trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
>
> Diff: https://reviews.apache.org/r/520/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: WIP: enable -fvisibility=hidden
-fvisibility-inlines-hidden
Posted by Alan Conway <ac...@redhat.com>.
> On 2011-03-22 12:14:43, Andrew Stitcher wrote:
> > Looking at the gcc documentation, it seems that the way __attribute((visibility=..)) is supposed to be used is the opposite way round to which we use in our headers (and in Visual Studio/Windows) your supposed to set a default which is more visible and then override it with less visible member functions.
> >
> > Doing it that way looks like it would avoid the warnings you are seeing (I haven't tried this though so I may have it wrong in practice)
I didn't get the same impression, I'll read again. The big advantage in doing it this way is that windows does it this way and the windows folks have already defined and tested our public API for us with macros we can re-use. I also feel like it makes more sense to me to directly define the public API rather than indirectly defining it as the inverse of the non-public functions. Seems like it is more error prone to try to mark every non-public function - since that's a constantly changing set - than mark the stable set of public API functions.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/#review347
-----------------------------------------------------------
On 2011-03-22 08:10:48, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/520/
> -----------------------------------------------------------
>
> (Updated 2011-03-22 08:10:48)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Summary
> -------
>
> WIP: enable -fvisibility=hidden -fvisibility-inlines-hidden
>
> Failing with warnings: warning: lowering visibility of ... to match its type
> Google suggests warning is ok, but no apparent way to disable it.
> Try build with warnings off to see if the results are good.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/configure.ac 1084053
> trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
> trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
> trunk/qpid/cpp/src/Makefile.am 1084053
> trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
>
> Diff: https://reviews.apache.org/r/520/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>
Re: Review Request: WIP: enable -fvisibility=hidden
-fvisibility-inlines-hidden
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/520/#review347
-----------------------------------------------------------
Looking at the gcc documentation, it seems that the way __attribute((visibility=..)) is supposed to be used is the opposite way round to which we use in our headers (and in Visual Studio/Windows) your supposed to set a default which is more visible and then override it with less visible member functions.
Doing it that way looks like it would avoid the warnings you are seeing (I haven't tried this though so I may have it wrong in practice)
- Andrew
On 2011-03-22 08:10:48, Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/520/
> -----------------------------------------------------------
>
> (Updated 2011-03-22 08:10:48)
>
>
> Review request for qpid and Andrew Stitcher.
>
>
> Summary
> -------
>
> WIP: enable -fvisibility=hidden -fvisibility-inlines-hidden
>
> Failing with warnings: warning: lowering visibility of ... to match its type
> Google suggests warning is ok, but no apparent way to disable it.
> Try build with warnings off to see if the results are good.
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/configure.ac 1084053
> trunk/qpid/cpp/include/qmf/engine/QmfEngineImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/CommonImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/ImportExportDefs.h PRE-CREATION
> trunk/qpid/cpp/include/qpid/agent/QmfAgentImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/client/ClientImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/console/ConsoleImportExport.h 1084053
> trunk/qpid/cpp/include/qpid/messaging/ImportExport.h 1084053
> trunk/qpid/cpp/src/Makefile.am 1084053
> trunk/qpid/cpp/src/qpid/broker/BrokerImportExport.h 1084053
>
> Diff: https://reviews.apache.org/r/520/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan
>
>