Fix weak vtables warnings #3418
Conversation
| @@ -5,5 +5,6 @@ $(project)_libraries := IceStorm | |||
| IceStorm_targetdir := $(libdir) | |||
| IceStorm_dependencies := Ice | |||
| IceStorm_sliceflags := --include-dir IceStorm | |||
| IceStorm_cppflags := $(api_exports_cppflags) | |||
There was a problem hiding this comment.
There is no -DICESTORM_API_EXPORTS because all the code in this library is generated code.
| { | ||
| public: | ||
| ICESTORM_SERVICE_API static std::shared_ptr<Service> create( | ||
| ~Service() override; |
There was a problem hiding this comment.
That's the only relevant header in IceStormService - the only exported class.
| # lmdb is not necessary for the icestormadmin sources. However, we want to build all objects with the same flags to | ||
| # reuse common object files in the different programs. | ||
| # we also include api_exports_cppflags because we "export" IceStormService to IceGrid. | ||
| $(project)_cppflags := $(if $(lmdb_includedir),-I$(lmdb_includedir)) $(api_exports_cppflags) |
There was a problem hiding this comment.
We want to use the same cppflags for all sources in each project.
| @@ -5,5 +5,6 @@ $(project)_libraries := IceGrid | |||
| IceGrid_targetdir := $(libdir) | |||
| IceGrid_dependencies := Glacier2 Ice | |||
| IceGrid_sliceflags := --include-dir IceGrid | |||
| IceGrid_cppflags := -DICEGRID_API_EXPORTS $(api_exports_cppflags) | |||
There was a problem hiding this comment.
-DICEGRID_API_EXPORTS was missing. It doesn't really matter with gcc/clang since IMPORTS/EXPORTS expand to the same... but it's clearer.
| @@ -4,6 +4,7 @@ $(project)_libraries := IceBox | |||
| $(project)_programs := icebox iceboxadmin | |||
| $(project)_dependencies := Ice | |||
| $(project)_sliceflags := --include-dir IceBox | |||
| $(project)_cppflags := $(api_exports_cppflags) | |||
There was a problem hiding this comment.
This Makefile.mk is not great as it produces both the IceBox library and the IceBox exe.
There was a problem hiding this comment.
You can set per component flags too, something like
# For icebox exe
icebox_cppflags := ...
# For iceboxadmin exe
iceboxadmin_cppflags := ...
There was a problem hiding this comment.
We can, but this conflicts with the shared object folder / object reuse per project.
There was a problem hiding this comment.
I don't see how this apply here, the icebox exe objects should not be used anywhere else.
| @@ -29,13 +29,16 @@ namespace IceInternal | |||
| class CancellationHandler | |||
| { | |||
| public: | |||
| virtual ~CancellationHandler(); | |||
There was a problem hiding this comment.
Not necessary - since not exported - but simpler than suppressing the warning via a pragma.
With clang, a class without any out-of-line virtual function has a "weak vtable", meaning clang stores this vtable in all translation units.
I believe this is an issue only with shared libraries, where having 2 vtables for the same class in different objects (say exe and shared library) leads to
dynamic_castandcatch(const exception&)failures at runtime.This PR addresses this issue by turning on
-Wweak-vtablesand fixing many weak vtables warnings in shared libraries.The usual fix is to define the virtual destructor out of line:
It's only meaningful when MyClass is exported (= part of the public API, or the semi-public API that plugins can use). And putting a virtual function out of line doesn't work with template classes.
Fixes #3409.