Opened 17 years ago

Closed 16 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 warmerdam)

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)

gdal_svn_trunk_hide_symbols.patch (3.5 KB ) - added by Even Rouault 17 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Even Rouault, 17 years ago

I'm attaching a patch that implements this.

Please review carefully as I'm a total beginner with autoconf stuff.

by Even Rouault, 17 years ago

comment:2 by warmerdam, 17 years ago

Cc: warmerdam added
Component: GDAL_RasterConfigBuild
Description: modified (diff)
Keywords: gcc added
Owner: changed from warmerdam to Mateusz Łoskot

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 Daniel Morissette, 17 years ago

Cc: Daniel Morissette added

comment:4 by Daniel Morissette, 17 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 Even Rouault, 17 years ago

Daniel, answering to your points :

  1. 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
  1. 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...
  1. 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 Daniel Morissette, 17 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 warmerdam, 17 years ago

I can live with the current situation where the optimization does not occur for debug builds.

comment:8 by hobu, 17 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 Even Rouault, 17 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 Mateusz Łoskot, 16 years ago

Cc: Mateusz Łoskot added
Owner: changed from Mateusz Łoskot to rouault,

comment:11 by warmerdam, 16 years ago

Owner: changed from rouault, to Even Rouault

comment:12 by hobu, 16 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 Even Rouault, 16 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 ?

comment:14 by warmerdam, 16 years ago

Done - r13352.

comment:15 by hobu, 16 years ago

Resolution: fixed
Status: newclosed

confirmed... closing.

Note: See TracTickets for help on using tickets.