Opened 16 years ago
Closed 15 years ago
#1810 closed enhancement (fixed)
[PATCH] Adds --with-hide-internal-symbols options to configure
Reported by: | Even Rouault | Owned by: | Even Rouault |
---|---|---|---|
Priority: | normal | Milestone: | 1.5.0 |
Component: | ConfigBuild | Version: | svn-trunk |
Severity: | normal | Keywords: | gcc |
Cc: | warmerdam, Daniel Morissette, Mateusz Łoskot |
Description (last modified by )
Quoting a recent discussion raised on gdal devel list :
Pre-scriptum : it may be usefull to read http://gcc.gnu.org/wiki/Visibility to understand some points of this discussion.
Reducing the number of symbols has a lot of benefits as stated in the above link :
- speed up loading of shared objects
- better opportunities for code optimizations
- reduce the size of SO.
- much lower chance of symbol collisions.
I've looked at the number of exported symbols by libgdal.so. It reaches the impressive number of 10 000 on my Linux system ! You can test on your own system with : nm -C -D libgdal.so | wc -l Then, I've grabbed FWTools136.exe and looked at gdal_fw.so. It "only" exports 3 549 symbols. The reason is that on Windows, you have to explicitely declare that a symbol is visible from outside the library. This is done in GDAL with the CPL_DLL macro that expands to
__declspec(dllexport)
for MSC.
However with GCC, symbols are implicitely considered as public/exported unless otherwise specified. Since GCC 4.0, we have a way to change that behaviour through the -fvisibility=hidden flag. Thus we can define this flag and modify the CPL_DLL declaration in cpl_port.h like hereafter to explicitely export public symbols:
#ifndef CPL_DLL #if defined(_MSC_VER) && !defined(CPL_DISABLE_DLL) # define CPL_DLL __declspec(dllexport) #else # if defined(__GNUC__) && __GNUC__ >= 4 # define CPL_DLL __attribute__ ((visibility("default"))) # else # define CPL_DLL # endif #endif #endif
I give it a try on my system. This required only another change in shapefil.h that doesn't use CPL_DLL but SHPAPI_CALL that is equivalent.
#ifndef SHPAPI_CALL # if defined(__GNUC__) && __GNUC__ >= 4 # define SHPAPI_CALL __attribute__ ((visibility("default"))) # define SHPAPI_CALL1(x) __attribute__ ((visibility("default"))) x # else # define SHPAPI_CALL # endif #endif
Result : only 3 442 symbols ! Roughly like on Windows :-)
Another great benefit is that it should prevent subtle and annoying problems when linking an executable with a GDAL library build with internal libtiff/libpng/libjpeg/etc... and one of these libraries.Now most of them have disappeared form libgdal.so. This may be becoming necessary with new libtiff 4.0 that has become the internal GDAL libtiff, whereas most *nix systems/distributios still uses 3.8.2. People will probably want to use BigTIFF capabilities without messing up their system libtiff. Mixing symbols from both versions is definitely not a good idea as there has been ABI changes. The case for internal libgeotiff is a bit different since it uses CPL_DLL too in its header. So, symbols for internal libgeotiff are still exported. We should fine a way to distinguish the case when it's built in GDAL tree from when it's build standalone.
So, to sum up, if we want to integrate this in GDAL, we have to :
- change cpl_port.h and shapefil.h
- add a test for the support of -fvisibility=hidden in configure
Any thoughts on all of this ?
(I also tried to use -fvisibility-inlines-hidden but it fails on my system since there's a bug in GCC 4.1 (apparently fixed in GCC 4.2). Anyway, it should not make a large difference since GDAL code is not using lot of inline functions.)
Attachments (1)
Change History (16)
comment:1 by , 16 years ago
by , 16 years ago
Attachment: | gdal_svn_trunk_hide_symbols.patch added |
---|
comment:2 by , 16 years ago
Cc: | added |
---|---|
Component: | GDAL_Raster → ConfigBuild |
Description: | modified (diff) |
Keywords: | gcc added |
Owner: | changed from | to
Patch applied (r12110), the configure changes look fine. I did a few tweaks to text to keep the option to one line and grouped the end-of-configure report a bit differently. I also upstreamed the shapefil.h change into shapelib so the next time we pull that down we won't lose it.
I don't have gcc4 on my system so I can't easily check this. I see the buildbot.osgeo.org uses gcc 4.0.2, so I'm reassigning this to Mateusz to modify buildbot to use this configure option.
comment:3 by , 16 years ago
Cc: | added |
---|
comment:4 by , 16 years ago
FYI I tested the new --with-hide-internal-symbols on my system (GCC 4.0.3 on Ubuntu 6.06), here are a few notes:
1- The option has no effect if I set CFG=debug. Is this the intended behavior?
2- Once I removed CFG=debug, the number of exported symbols went from 12084 to 3358 on my system.
3- The new build with the reduced number of symbols worked fine with MapServer and PHP MapScript.
comment:5 by , 16 years ago
Daniel, answering to your points :
- No, it wasn't intended, but I'm wondering if it's not the right thing to do after all. This hide-internal-symbols thing is kind of an optimization, isn't it ? Anyway, I think it's a side effect of :
CFLAGS="$CFLAGS -fvisibility=hidden" CXXFLAGS="$CXXFLAGS -fvisibility=hidden"
And I feel that it's related to some of the first lines in GDALmake.opt.in :
#Flags to build optimized relese version CXX_OPTFLAGS = @CXXFLAGS@ C_OPTFLAGS = @CFLAGS@ # Flags to build debug version CXX_DBGFLAGS = -g -DDEBUG C_DBGFLAGS = -g -DDEBUG # Flags to build profiling version CXX_PROFFLAGS = -g3 -pg -DDEBUG C_PROFFLAGS = -g3 -pg -DDEBUG
- Yes, that's expected ! 3358 is still an impressive number, but it would require a more in-depth analysis to see if we can hide more symbols. I've seen a few symbols at some places that were exported as public (like GTIFGetOGISDefn, GTIFSetFromOGISDefn, GTIFMemBufFromWkt, GTIFWktFromMemBuf that are defined an frmt/gtiff/gt_wkt_srs.cpp and are only used in it and in frmt/gtiff/gt_overview.cpp) but I'm doubtful someone will ever want to track them... And who knows if someone hasn't started using them in his own code...
- Good news ! That was expected since MapServer runs already on MS Windows and that the effect of the patch is that we have the same symbols exported on Windows and Posix-like systems now.
comment:6 by , 16 years ago
About including this in debug builds or not, I wasn't sure what's best either. I had seen the lines in GDALmake.opt but had not noticed that only the OPTFLAGS lines had AC_SUBST stuff in them and the DBGFLAGS and PROFFLAGS didn't. I guess that's one more argument in favor of keeping the -fvisibility=hidden only for optimized builds.
comment:7 by , 16 years ago
I can live with the current situation where the optimization does not occur for debug builds.
comment:8 by , 16 years ago
This option seems to break the old-gen python bindings on osx (and maybe other places). I don't know that we should enable it by default in that context unless we can figure out how to correct it.
comment:9 by , 16 years ago
Hobu,
could you paste the error messages so we can figure out what happens exactly ? It may be caused by some symbols that are used by the old-gen python bindings but are not declared as 'CPL_DLL' in the .h
comment:10 by , 16 years ago
Cc: | added |
---|---|
Owner: | changed from | to
comment:11 by , 16 years ago
Owner: | changed from | to
---|
comment:12 by , 15 years ago
Here's the *right* fix for the old-gen bindings problem:
Index: gdal_wrap.c =================================================================== --- gdal_wrap.c (revision 13312) +++ gdal_wrap.c (working copy) @@ -21,6 +21,7 @@ #define SWIGPYTHON #include <string.h> #include <stdlib.h> +#include "cpl_config.h" /*********************************************************************** * $Header:$ * swig_lib/python/python.cfg @@ -61,7 +62,10 @@ # endif # endif #else -# define SWIGEXPORT(a,b) a b +# if defined(USE_GCC_VISIBILITY_FLAG) + void init_gdal() __attribute__ ((visibility("default"))); +# endif +# define SWIGEXPORT(a,b) a b #endif #ifdef SWIG_GLOBAL
Getting swig1.1 to do this is likely very challenging. I propose that we complain to the user if they have requested both --with-ogpython *and* --with-hide-internal-symbols. This seems like the least disruptive option.
Things work ok with the ng bindings because they don't pick up GDAL's CFLAGS/CPPFLAGS unless explicitly passed to setup.py.
comment:13 by , 15 years ago
Patch commited in r13350 to prevent the user from using --with-ogpython *and* --with-hide-internal-symbols.
Frank, please could you regenerate configure ?
I'm attaching a patch that implements this.
Please review carefully as I'm a total beginner with autoconf stuff.