GNOME Bugzilla – Bug 688681
build: Make .symbols file canonical on all platforms
Last modified: 2013-01-18 18:34:52 UTC
See https://bugzilla.gnome.org/show_bug.cgi?id=687441 for motivation.
Created attachment 229409 [details] [review] build: Make .symbols file canonical on all platforms Previously: * On Unix, an -export-symbol-regex (g_)|(glib_) was used, and the .symbols files were used in "make check" * On Windows, the .symbols files are massaged into a format which MSVC expects as an export list Now: * On both Unix and Windows, the .symbols file is used as the list of symbols to export, period. Even symbols starting with g_ are not exported unless they are in the file. The abicheck.sh tools are deleted.
Review of attachment 229409 [details] [review]: I like the idea; it would be even nicer if we could get out of the preprocessing business altogether, and just have .export files in git. ::: gio/Makefile.am @@ +29,3 @@ mv $(builddir)/gio.def.tmp $(builddir)/gio.def +gio.export: gio.symbols $(top_srcdir)/glib/process-symbol-file.sh Makefile I guess technically, the .export files depend on glib/glibconfig.h as well...
(In reply to comment #2) > Review of attachment 229409 [details] [review]: > > I like the idea; it would be even nicer if we could get out of the > preprocessing business altogether, and just have .export files in git. Would you really want that though? We'd need separate symbol files for Windows and UNIX, but most functions exist on both. Also, at the moment the exported symbol set depends on --enable-debug, but maybe we should just suck it up and always export the debug symbols. > ::: gio/Makefile.am > @@ +29,3 @@ > mv $(builddir)/gio.def.tmp $(builddir)/gio.def > > +gio.export: gio.symbols $(top_srcdir)/glib/process-symbol-file.sh Makefile > > I guess technically, the .export files depend on glib/glibconfig.h as well... Will fix. Incidentally, I should note this patch is only lightly tested. I'll do a bit more build evaluation, and it'd be nice if danw did some win32 testing.
(In reply to comment #3) > > > > I like the idea; it would be even nicer if we could get out of the > > preprocessing business altogether, and just have .export files in git. > > Would you really want that though? We'd need separate symbol files for Windows > and UNIX, but most functions exist on both. > Maybe one could have the unix .export files as canonical sources in git, and somehow derive the windows ones from them. Or maybe not.
I'm not sure I'm crazy about this. What I would prefer instead (from a purely Linux-centric viewpoint) would be to explicitly annotate all public functions for export. We already have macros for new functions (the GLIB_AVAILABLE_IN stuff) so this would merely be an exercise in annotating all of our old APIs (which btw, strictly speaking, should have 'extern' on them anyway...). Then we could change the default symbol visibility to hidden. From what I understand this is how things are meant to work on Windows as well (ie: having to explicitly mark functions for export).
(In reply to comment #3) > and it'd be nice if danw did some win32 testing. You can test it yourself! It's ridiculously easy. yum install mingw and wine, then "./autogen.sh --build=x86_64-unknown-linux --host=i686-w64-mingw32 && make". (OK, it's not *quite* that easy; you'll need a handful of the mingw devel packages for iconv and zlib and stuff too, and if you actually want to run test programs out of the build tree you need to copy .dlls into the libtool .libs dirs. But other than that--ridiculously easy!) (In reply to comment #5) > I'm not sure I'm crazy about this. I am. (Crazy about it, that is, not sure that you are.) > What I would prefer instead (from a purely Linux-centric viewpoint) would be to > explicitly annotate all public functions for export. In effect, we already do; functions listed in installed headers are public, and functions not listed in installed headers aren't. It would be redundant to re-annotate them all by hand. (Though our ability to parse arbitrary .h files is not quite so good that I'd suggest getting rid of the symbols files altogether...) OTOH, if you're suggesting that we should extend the GLIB_AVAILABLE_IN_* macros all the way down to 2.0 (as opposed to just a single GLIB_PUBLIC_FUNCTION macro)... well, maybe there's something to be said for that. It would have the nice side effect of making sure that people added GLIB_AVAILABLE_IN_X_XX tags to any newly-added API, since if they forgot to, the function wouldn't be exported... > (which btw, strictly speaking, > should have 'extern' on them anyway...). says who?
(In reply to comment #5) > What I would prefer instead You don't think this patch gets us in a better state than the previous? I mean, you had to update the files anyways to pacify 'make check'. > Then we could change the default symbol visibility to hidden. I'm not opposed. But that's also honestly a *lot* of tedious work that hits every header file. I think we need to be more incremental here. For example, we could keep adding GLIB_AVAILABLE_IN to things over time, and do the switch when we're ready.
(In reply to comment #7) > (In reply to comment #5) > > > What I would prefer instead > > You don't think this patch gets us in a better state than the previous? > I think it does
Created attachment 229500 [details] [review] build: Make .symbols file canonical on all platforms Ok, this patch is a lot nicer because it unifies the tooling to do both the libtool and Windows builds, so it's actually a net code reduction. Tested on both jhbuild-Fedora 17/x86_64 and Fedora17-mingw32 cross.
So I wrote a dirty script (attached) to diff the symbols between the two builds, and here's what I got: $ diff -u /tmp/symbols.old /tmp/symbols.new --- /tmp/symbols.old 2012-11-20 14:32:37.350948389 -0500 +++ /tmp/symbols.new 2012-11-20 14:30:46.716854821 -0500 @@ -1564,7 +1564,6 @@ g_vsnprintf g_vsprintf g_warn_message -_fini g_module_build_path g_module_close g_module_error @@ -1573,11 +1572,8 @@ g_module_open g_module_supported g_module_symbol -_init -_fini g_thread_init g_thread_init_with_errorcheck_mutexes -_init g_array_get_type g_binding_flags_get_type g_binding_get_flags In other words, we were unintentionally exporting the _init and _fini, which is pretty lame, so...I'm happy to strip it.
Created attachment 229501 [details] dump symbols from glib
Created attachment 230103 [details] [review] Remove most use of G_GNUC_INTERNAL Now that we use an explicit list of symbols to export, the G_GNUC_INTERNAL is redundant.
<walters> desrt: so we are not applying my patch because of the concern "the case that we could add new public API (without a test case) and do a release without noticing that it's not exported" <desrt> walters: and because i don't see a benefit <desrt> i'm sorry -- i just don't agree that what you've done is better <walters> it reduces the duplication between unix/windows for one <desrt> and since we both seem to agree that what i'm proposing would be better than either the existing solution or your new one, why don't we just do that? * garnacho has quit (Remote closed the connection) <walters> if you will do the work for that, i have no objection <desrt> i'm doing it now <walters> ok <desrt> i'll probably stop to take a break after libglib...
Created attachment 230124 [details] [review] all libraries: Change export control mechanism Introduce a macro G_GNUC_PUBLIC that does the opposite of G_GNUC_PRIVATE: if the default symbol visibility is hidden, this marks a symbol as explicitly public. Make all of the other macros that we use to decorate our public symbols (GLIB_AVAILABLE_IN_..., GLIB_DEPREACTED, GLIB_VAR, etc.) implicitly include GLIB_PUBLIC. Switch the default symbol to -fvisibility=hidden. Annote all public headers with GLIB_PUBLIC (or GLIB_AVAILABLE_IN_ for functions that were introduced later).
As promised, the patch. I just worked through it all... It's in mega-patch form. We can split it up later if we agree to go forward with this approach. The patch doesn't remove the abi check (yet) so you can see that we're producing the same exports as we were before, with the exception of GIO which I caught three additional accidental exports in. One more in GMenuModel (clearly part of the same mistake that Colin caught part of before) and the _get_type() functions for the private classes GLocalFileMonitor and GLocalDirectoryMonitor. I think we should just hide those, as we did with the others. There are a couple of more difficult cases. A younger and sillier version of me believed that it was a good idea to export public ABI (but not headers) in order to hit internal functions from testcases and there is a good deal of that in the GVariant serialiser and typeinfo code that I was reminded of while doing this work.... I guess ideally we should hide those too.... There's also some oddity with inline functions.... the trashstack code is implemented in a pretty weird way... probably worth deprecating and turning into normal non-inlines... Going forward a nice check might be to ensure that GLIB_PUBLIC (or the GLIB_AVAILABLE_IN_ etc.) macros are only appearing in headers marked in the Makefile for installation. That would prevent accidentally marking a function in an internal header with the public macro... Another thought: GLIB_PUBLIC, when not building GLib, should probably evaluate to 'extern'. We could also consider renaming it to GLIB_EXPORT or GLIB_EXTERN.
Review of attachment 230124 [details] [review]: So what happens with MSVC in this scenario? If you ping the MSVC guy he may be able to figure out how to frob the project files so that it matches the gcc configuration. Also note need GLIB_PUBLIC to expand to __declspec(dllexport) with MSVC. ::: configure.ac @@ +3047,3 @@ #elif defined (__GNUC__) && defined (G_HAVE_GNUC_VISIBILITY) #define G_GNUC_INTERNAL __attribute__((visibility("hidden"))) +#define G_GNUC_PUBLIC __attribute__((visibility("default"))) This bit could be extracted into a separate patch, since it could be used outside GLib even if we decide not to use -fvisibility=hidden in GLib itself. @@ +3549,3 @@ -Werror=missing-prototypes -Werror=implicit-function-declaration \ -Werror=pointer-arith -Werror=init-self -Werror=format-security \ + -Werror=format=2 -Werror=missing-include-dirs -fvisibility=hidden]) I don't much like mixing something so fundamental as -fvisibility=hidden along with a list of compiler warnings. We should move this to a separate section that says "fill in the visibility flag for your compiler here". And decide there whether we want to just abort for unsupported compilers, or leak all of our internals. Note - one major concern with this is that libtool might somehow override -fvisibility=hidden later. A more sustainable fix here is to upstream this into libtool, or stop using it.
As far as abicheck.sh - one thing we could do is have a list of symbols that should not be exported. That way we will quickly detect toolchain failure or misconfiguration and the like, without having to maintain the full .symbols file too.
-fvisibility=hidden is a CFLAG and libtool has a tendency not to mess with those as much as linker flags... I definitely agree that it should be somewhere else, though. I was actually going to put it right beside the -Bsymbolic-functions check, but it turns out that those warnings were also beside it, so... :) For MSVC I just expect GLIB_PUBLIC to be a dllexport sort of thing... this is how it's supposed to work on Windows, so we're probably actually making the situation there easier...
About the abicheck -- I think we should just nuke the symbols file entirely. It's now completely redundant. The idea of having a check to test a few representative samples (a public function, a private function, a global variable, etc.) to make sure things are generally sane with each toolchain is a good one, though.
One other bit - I have no objection to separating out a patch to annotate everything with GLIB_PUBLIC, even if it doesn't mean anything yet.
Hi Chun-wei - can you help us with the MSVC perspective here?
Created attachment 230132 [details] [review] Introduce G_GNUC_PUBLIC macro This does the opposite of G_GNUC_PRIVATE: if the default symbol visibility is hidden, this marks a symbol as explicitly public. Make all of the other macros that we use to decorate our public symbols (GLIB_AVAILABLE_IN_..., GLIB_DEPREACTED, GLIB_VAR, etc.) implicitly include GLIB_PUBLIC.
Created attachment 230133 [details] [review] Annotate all public API with GLIB_PUBLIC/GLIB_AVAILABLE_IN In GLib itself, this macro at present means nothing. It will however be used by a future patch to switch the default symbol visibility.
Created attachment 230134 [details] [review] Use -fvisibility=hidden by default Now that we have all symbols annotated as GLIB_PUBLIC, we can switch the build to default to hiding symbols. The major advantage of this is that it's much harder to accidentally leak symbols.
Note the last patch immediately blows up gio/fam, since it subclasses GLocalFileMonitor, but g_local_file_monitor_get_type() is no longer a public symbol. It is exported in the current gio/gio.symbols file of course...
Beofre we consider applying the first two - Ryan, is there a reason to define a GLIB_PUBLIC that's simply an alias for G_GNUC_PUBLIC? If I was writing a GLib-using client library (but I didn't copy the whole LIBFOO_AVAILABLE_IN_ macros), I think it'd make sense to just use G_GNUC_PUBLIC, and GLib itself would be a model.
G_GNUC_PUBLIC implies that it's a GNU C feature. I expect that on Windows GLIB_PUBLIC would be defined to dllexport. Maybe what we really need is a macro that doesn't have 'GNUC' in the name, but still starts with G_, implying that others can use it. All the same, though, I'd still prefer to use a macro starting with GLIB_ for the actual tagging. Reason: after we install GLib, this macro should evaluate to nothing (other than perhaps 'extern'). Other packages need not be interested in how we manage our symbols...
btw: I like your idea of defining GLIB_PUBLIC to mean nothing and then committing the patch that adds all the GLIB_PUBLIC everywhere. That patch is going to be full of conflicts pretty quickly... the sooner we apply it the better.
Review of attachment 230133 [details] [review]: Hi, From what I can see by what is going on, a few points I guess I would like to bring up regarding the MSVC part. This is not unlike what Cairo did some time ago. -It is indeed going to be __declspec(dllexport) and __declspec(dllimport) on MSVC. A similar thing is done for GLIB_VAR in gtype.h, for references as it currently exports variables only. So, if we need to export variables, this is the only correct way to go AFAIK -I think we will need a macro for each of the libraries in GLib, say GLIB_PUBLIC, GOBJECT_PUBLIC, GMODULE_PUBLIC and GIO_PUBLIC. This is because when, for example, GIO links, it needs to link against GLib and GObject, the macros for GLib and GObject need to expand to __declspec (dllimport) and the macro for GIO needs to be __declspec (dllexport). We can control these things by using the current macros we already define during compilation, such as GLIB_COMPILATION -It might be a good idea to define these macros in a (or few) new small header file(s), which needs to be included/defined before anything else so that the needed symbols can be exported properly. This probably means all platforms as well after we switched the default symbol visibility. -I will probably need to go further to see any other points that might ring bells, but that is what I have on top of my head at the moment With blessings.
(In reply to comment #27) > G_GNUC_PUBLIC implies that it's a GNU C feature. I expect that on Windows > GLIB_PUBLIC would be defined to dllexport. > > Maybe what we really need is a macro that doesn't have 'GNUC' in the name, but > still starts with G_, implying that others can use it. G_PUBLIC? > All the same, though, I'd still prefer to use a macro starting with GLIB_ for > the actual tagging. Reason: after we install GLib, this macro should evaluate > to nothing (other than perhaps 'extern'). Other packages need not be > interested in how we manage our symbols... True; OK. The indirection here is annoying, but I guess that's life with C as it is today. There's so many tedious unobvious hoops to jump through to do a library "right" =/
(In reply to comment #29) > -I think we will need a macro for each of the libraries in GLib, say > GLIB_PUBLIC, GOBJECT_PUBLIC, GMODULE_PUBLIC and GIO_PUBLIC. This is because > when, for example, GIO links, it needs to link against GLib and GObject, the > macros for GLib and GObject need to expand to __declspec (dllimport) and the > macro for GIO needs to be __declspec (dllexport). We can control these things > by using the current macros we already define during compilation, such as > GLIB_COMPILATION Right...ugh. > -It might be a good idea to define these macros in a (or few) new small header > file(s), which needs to be included/defined before anything else so that the > needed symbols can be exported properly. This probably means all platforms as > well after we switched the default symbol visibility. We should do a first pass where we ensure that every single C file has as its first non-comment line: #include "config.h" Then change it to: #include "glib-compilation.h" which is just: #include "config.h" #ifdef GLIB_COMPILATION #define GLIB_PUBLIC G_PUBLIC #else #define GLIB_PUBLIC #endif
Ongoing work here can be tracked in this git branch: http://git.gnome.org/browse/glib/log/?h=wip/symbol-visibility
Created attachment 230220 [details] [review] gio: GLIB_AVAILABLE_IN to more APIs Based on a patch by Ryan Lortie <desrt@desrt.ca>.
Created attachment 230221 [details] [review] G_PUBLIC_API: New macro which directs compiler to export a symbol This will be used by a future GLib patch to control GLib's own symbol exports, but it can also be used by libraries depending on GLib.
Created attachment 230222 [details] [review] buildsystem: Prepare _G*_API internal macros for symbol visibility To switch GLib to have private-by-default symbols, we need to have a macro which expands to G_PUBLIC_API only when compiling a particular "sublibrary" in GLib (e.g. gmodule, gio). This patch implements that with a two-level scheme. First, each sublibrary has its own preprocessor macro e.g. _GTHREAD_API. Then, the Makefiles are modified to define this to G_PUBLIC_API only when building that library - so when building libgthread, _GMODULE_API is left undefined. Now, we could simply add _G*_API to every entry point, but because we've already been annotating all the headers with GLIB_AVAILABLE_IN_XX, let's modify those macros to automatically depend on a new "_G_API". Then the Makefiles further define _G_API to the target such as _GTHREAD_API. This indirection avoids a (glib version)*(sublibrary) explosion of preprocessor definitions in gversionmacros.h.
Created attachment 230223 [details] [review] Annotate all public API with _G*_API This macro will be used to mark these symbols as public exports.
Created attachment 230224 [details] [review] Use -fvisibility=hidden by default Now that we have all symbols annotated as GLIB_PUBLIC, we can switch the build to default to hiding symbols. The major advantage of this is that it's much harder to accidentally leak symbols. This patch only adds it to CFLAGS for the sublibraries; we don't want to affect things like gio/fam, nor the executables.
Created attachment 230225 [details] [review] Remove most use of G_GNUC_INTERNAL Rebased
The first patch should just go in now - it's clearly right. The next 3 patches can also go in now, without any negative effect (I believe). The last two are the big switch.
Comment on attachment 230220 [details] [review] gio: GLIB_AVAILABLE_IN to more APIs i'll trust that you actually got all the numbers right...
(In reply to comment #39) > The next 3 patches can also go in now, without any negative effect (I believe). only if we're sure we are going this route...
(In reply to comment #41) > (In reply to comment #39) > > The next 3 patches can also go in now, without any negative effect (I believe). > > only if we're sure we are going this route... Well...if we decide not to, all that happens is we have a lot of _G*_API annotations in the headers that don't hurt anything. They don't benefit anything either, of course.
Comment on attachment 230220 [details] [review] gio: GLIB_AVAILABLE_IN to more APIs Attachment 230220 [details] pushed as 52c608d - gio: GLIB_AVAILABLE_IN to more APIs
<walters> one thing i forgot to mention is the evil bit with libgthread <desrt> oh? <walters> i think at the moment the patch series just drops the exports from libgthread <walters> it's broken because the prototypes are in glib/gthread.h <desrt> oh. interesting point. This bit needs to be fixed before we land...actually, we need to rerun the diff-symbols script above.
Review of attachment 230222 [details] [review]: I like the approach you've taken here (except for the use of a leading underscore) but I think it has a problem: You use _G_API from the GLIB_AVAILABLE_IN_* macros and define that from each submodule that you're building -- so far so good. Problem is that you then define (for example) _G_API to _GIO_API when building gio... This will also impact the symbols in the libglib public headers that are marked as GLIB_AVAILABLE_IN_* -- they'll seen by the compiler as if they had been marked as _GIO_API. In general, I think we should try very hard to avoid having to have separate macros per-submodule. Nobody has tested MSVC yet, but from what I read on MSDN, it's probably not necessary...
On the other hand, we could just bring in GIO_AVAILABLE_IN_* and so on... not sure I'm crazy about multiplying that already large set of macros by 3 or 4, though...
I think your comment 31 is getting pretty close to what would be a good solution, assuming the worst of MSVC. A slight variation on that could look like that: 1) Forbid the use of the single-include header from within the module itself. We could enforce this with per-subdir _COMPILE defines and a #error ifdef in the header. 2) Demand the use of the single-include header from outside the module. We already do that... 3) At the top of the single-include header for each module, #define GLIB_PUBLIC to __declspec(dllimport) and #undef it at the end. This will deal with the case of the header being included by people outside of the library. 4) In gmacros (or whatever place we decide to define GLIB_PUBLIC) only define it to __declspec(dllexport) if it's not been defined yet. That means that we will only define it if we didn't include via <glib.h>. config.h may be another fine place to take care of this -- I don't think we need a separate glib-compilation header... (although using that as a way to deal with precompiled headers might be a nice approach, but I think there are other reasons that we don't want to go there).
Thinking about this a bit more, the essence of this problem boils down to the existence of this theoretical public header in gio.h: -- gthingy.h #include <glib-object.h> G_AVAILABLE_IN_2_36 GType g_thingy_get_type () -- Notice that we pass directly from glib-object.h to using G_AVAILABLE_IN_ to define public symbols. Things in glib-object.h will also be using G_AVAILABLE_IN_. We need G_AVAILABLE_IN_ to mean __declspec(dllexport) in glib-object.h and __declspec(dllexport) in gthingy.h. This means that either: (a) we must have some sort of epilogue in our public headers (b) when including cross-submodule, we have a utility header that has the epilogue (c) we can't use global G_AVAILABLE_IN_ macros in all the submodules; we have to have per-submodule macros (d) we just universally dllexport and hope that's OK. Of the options, I find only (a) or (d) to be acceptable.
(In reply to comment #46) > On the other hand, we could just bring in GIO_AVAILABLE_IN_* and so on... not > sure I'm crazy about multiplying that already large set of macros by 3 or 4, > though... We could write a simple Python script to generate it at build time.
um... what is the advantage of all this over just using a .symbols file and letting libtool do the work from there?
(In reply to comment #50) > um... what is the advantage of all this over just using a .symbols file and > letting libtool do the work from there? Not maintaining a symbols file.
Ok so after trying this patch series on mingw32, none of the symbols get exported in the .dll. Cairo appears to basically generate a .def file from the headers: http://cgit.freedesktop.org/cairo/tree/src/Makefile.am?id=153b11612f34294241429b53722839984f367f2e#n65
According to some docs I find, we should be using dllexport with mingw despite it being based on GCC... http://www.mingw.org/wiki/sampleDLL
There are some further complications here; looking at the .symbols file, there's a different ABI for win64 vs win32 - basically since win64 is a new ABI, the old backwards compatibility non-_utf8() entry points are gone. This sort of thing is why I still believe we should go with the initial patch to unify on the .symbols file - it is *clearly* an improvement. So the current state of http://git.gnome.org/browse/glib/log/?h=wip/symbol-visibility is that we use that patch first, and then build on it so that we use -fvisbility=hidden only on Unix. Further work on dropping the symbols file would have to account for: * __declspec(dllexport) vs visbility(default) * symbols for libgthread living in glib/gthread.h * win64 vs win32 ABI differences * GLib test cases which import bits of the source code (e.g. glib/tests/gwakeup.c) And likely more.
After a lot of back and forth with branches and talking things over with Colin and Dieter on IRC I've pushed a patch set implementing the "tag all public API" approach. I've yet to do any "destructive" work to follow up: - the symbols files still exist - we still have a regexp for export control - we still have abicheck as part of 'make check' - G_GNUC_INTERNAL is still in use I want to pause now to get some more feedback before proceeding with the 'destructive' changes above. Some notes: There is no reason to keep G_GNUC_INTERNAL as it is completely redundant now that 'hidden' is the default visibility. The symbols files also have no reason to continue existing. Getting rid of them was, in fact, the entire point of this exercise. The abicheck script would obviously go along with the symbols file. It may be worth keeping the regexp for export control, for two reasons: - clean up our exports a bit on platforms that don't support GNU visibility, but note that this would be a very partial solution because we are still exporting G_GNUC_INTERNAL API in that case (of which there is quite a lot) - removing it means we export _init and _fini. Any strong opinions on this, Colin? (since you mention it in comment 10). - extra safety if someone tries to GLIB_AVAILABLE_IN_* a non-'g_' symbol (although, honestly, I'd prefer a testcase that fails the build in that case over a silent filtering). I consider these reasons pretty weak and I'd love to see us get to a place where we only have one single export control mechanism...
Hi, For people's references, I have updated the GLib Visual C++ projects to stop using the .def files to generate the .lib files from the DLLs in commit 4ba56f36 as well. With blessings.
Current patches are at: http://git.gnome.org/browse/glib/log/?h=wip/visibility-cleanup They look fine to me. I'm actually curious how the g_thread_* API problem was handled, going to look at that now.
(In reply to comment #57) > They look fine to me. pushed. > I'm actually curious how the g_thread_* API problem was handled, going to look > at that now. (as mentioned earlier on IRC) I fixed the issue by #include "config.h" in the .c file. This means that the declarations get the proper _GLIB_EXTERN treatment.
Comment on attachment 230225 [details] [review] Remove most use of G_GNUC_INTERNAL Attachment 230225 [details] pushed as 6f8f1f7 - Remove most use of G_GNUC_INTERNAL
I think we're done here.