Created attachment 29290 [details]
gzipped patch to get rid of about 1700 PLT entries

195 files changed, 12964 insertions(+), 7 deletions(-)

Hmm, does this "functions" file contain the a list of the functions that should 
be exported from libgtk? If so, it should probably be unified with gtk.def, 
i.e. one should be produced from the other.

basically that is correct. (well technically it only requires to have the
functions exported that are used internally, but "all exported" is fine).

Question is if the ABI has all the same functions on gtk.def vs linux
platforms... something I'm not entirely convinced of yet.

The difference between your functions file and gtk.def seems to be that gtk.def 
contains also names of exported variables, and of course the platform-dependent 
entries are different (gtk_file_system_unix_* vs. gtk_file_system_win32_*, and 
no gtk_plug_* or gtk_socket_* on Win32).

I.e. the commonality is quite large, and if this patch is accepted, the common 
names should be kept in one file, and then platform-specific additions 
separately, and then merged in the Makefile.am.

I wonder if that really is useful to be honest.
the "functions" file is autogenerated and well, intermediate and of not TOO much
concequence, while gtk.def is the official ABI and thus manually maintained.
I can see the point of trying to derive "functions" from gtk.def but given the
'not quite the same-ness'... 

It's also possible to get rid of functions entirely and just pipe the nm output
straight into the perl script, would that be a better solution ?

I'd really like it if on Linux 'make check' (and hence 'make distcheck')
checked that 

 - All the exports were expected (no missing statics)
 - gtk.defs had all the exported symbols on Linux except for stuff
   that was explicitely excluded

I could imagine a big file listing all symbols with #ifdef GTK_WINDOWING_X11
conditionals of some sort that was used for this and also for the purpose
of generating the internal-bypass header file.

I agree that abi checking is useful... but it's somewhat beyond the scope of
this patch (although this patch ran into some issues and sort of kinda would
like an abi checklist)

Here is a possible approach to integrating gtk.def, gtkalias.h and abi checking:

gtk.def: functions
	(echo -e EXPORTS; cat functions | cpp -P -DVARIABLES -DG_OS_WIN32 | sed -e
'/^$$/d' -e 's/^/\t/') > gtk.def

gtkalias.h: functions
	 cat functions -P -DG_OS_UNIX -DGTK_WINDOWING_X11 | ./makegtkalias.pl > gtkalias.h

abicheck: functions
	cat functions | cpp -P -DG_OS_UNIX -DGTK_WINDOWING_X11 | sed -e '/^$$/d' | sort
> expected-abi
	nm -D .libs/libgtk-x11-2.0.so | grep " T " | cut -c12- | grep "^gtk" | sort >
actual-abi
	diff -u expected-abi actual-abi

Ie gtk.def, the list of functions to created aliases for and the list to check
the abi against are all generated from a master list which looks like the
following excerpt:

/* This list defines the GTK+ ABI. It is used to generate the gtk.def
 * file and the gtkalias.h header. 
 * Variables must be put into ifdef VARIABLES ... endif sections. 
 * Symbols which occur only on Windows, or only on Unix, or only with 
 * the X11 backend must be included in similar ifdefs, using the symbols 
 * G_OS_WIN32, G_OS_UNIX and GTK_WINDOWING_X11. 
 */ 
gtk_about_dialog_get_artists
gtk_about_dialog_get_authors
gtk_about_dialog_get_comments
gtk_about_dialog_get_copyright
gtk_about_dialog_get_documenters
gtk_about_dialog_get_license

...

gtk_attach_options_get_type
#ifdef VARIABLES
gtk_binary_age
#endif
gtk_binding_entry_add_signal

...

gtk_file_system_render_icon
#ifdef G_OS_UNIX
gtk_file_system_unix_get_type
gtk_file_system_unix_new
#endif
gtk_file_system_uri_to_path

...


This approach seems to work fine for me so far. It regenerates exactly the
gtk.def file we currently have in cvs, and the api check successfully chokes on
my local additions. 

Open questions:

1) Arjan, why does makegtkalias.pl list almost all headers instead of simply
   including gtk.h ?
2) What is the rationale for the list of excluded symbols in makegtkalias.h ?
3) Is the simple #ifdef __GNUC__ check enough to protect the aliasing 
   business ? Do all versions of gcc support the visibility attribute ?
4) Tor, are you and Hans comfortable with making gtk.def a generated file ?
   Generating it from the same file that governs the abi check would have
   the advantage that gtk.def is always uptodate...
4) I have no idea how to hook the abicheck target into the distchecking
5) Do we want the same thing for gdk ?

1) the alias header requires prototypes for ALL the functions it's going to make
an alias for. Including gtk.h is not sufficient for that. I can see the point of
it being desirable for it to be that, and I'd love for the headers to get fixed
to be like that, but right now they aren't.

2) The rationale is that those few symbols have ABI/header issues and break
right now. Ideally there would be no excludes, but it's beyond the scope of this
patch to fix the ABI issues

3) As far as I know every ELF capable gcc has this, which covers suffcient gcc
versions :)

5) I would suggest so

4) Yes! Keeping gtk.def up-to-date by hand is a pain, although I think Hans has 
used some header-parsing script to automate it at least partially.

5) If possible, yes.

Well, I'm certainly OK with the general approach (see comment 7)

On the open issues:

1) gtk.h is explicitely *not* supposed to include things that aren't
   public. On the other hand, we definetely want to kill PLT redirections
   for non-public functions, so I don't think there is anything to "fix"

2) I think we should make sure we have open bugs in bugzilla for all
   problems like this before we commit a list of "excluded symbols" to CVS.
   We might also want to put them in the "functions" file with a special
   #ifdef rather than in a script.

3) Certainly not every ELF capable GCC - e.g.:

   http://sources.redhat.com/ml/libc-hacker/2002-03/msg00064.html

   It's probabably 2.96 or newer. (See G_GNUC_PURE definition in
   gmacros.h for conditionalization)

4) if OS UNIX 
   TESTS = abicheck.sh 
   endif would be the easiest, make distcheck runs make check

5) I don't see a GTK+/GDK difference here. 

Couple small things:

* I don't particularly like a file named 'functions' that includes
  variables :-). Maybe gtk.symbols

* As always, cat shouldnt' be used as a subsitute for <

* s/VARIABLES/INCLUDE_VARIABLES/ ?

Ok, committed a modified version of that patch. Moving this bug to gdk to do the
same surgery there...

Why not skip the whole gtkalias.h circus and use "protected" visibility instead?

that only works within the same .c file
this works even cross .c files....

Committed corresponding gdk patch as well.

this works fine for me:

foo1.c: -----------------------------------------------------------------------
#include <stdio.h>

void
__attribute__ ((visibility ("protected")))
     f (void)
{
  printf ("Hi from %s\n", __func__);
}

foo2.c: -----------------------------------------------------------------------
extern void __attribute__ ((visibility ("protected"))) f (void);

int
main (int argc, char **argv)
{
  f ();
  return 0;
}


darter:~> gcc -Wall -c foo1.c
darter:~> gcc -Wall -c foo1.c
darter:~> gcc -Wall -c foo2.c
darter:~> gcc -Wall foo1.o foo2.o
darter:~> ./a.out 
Hi from f
darter:~> gcc -v
Reading specs from /usr/lib/gcc-lib/i586-suse-linux/3.3.1/specs
Configured with: ../configure --enable-threads=posix --prefix=/usr
--with-local-prefix=/usr/local --infodir=/usr/share/info --mandir=/usr/share/man
--libdir=/usr/lib --enable-languages=c,c++,f77,objc,java,ada --disable-checking
--enable-libgcj --with-gxx-include-dir=/usr/include/g++ --with-slibdir=/lib
--with-system-zlib --enable-shared --enable-__cxa_atexit i586-suse-linux
Thread model: posix
gcc version 3.3.1 (SuSE Linux)

Note, that any change to any .h file with the gtkalias method will cause a global
recompile.

your proposal implies that all prototypes get the attribute in the prototype,
which basically means touching ALL headers; that got vetoed down so the
alternative of doing one special header instead came into existance

I looked through the bug comments and I don't see that a secret cabal should
have vetoed the changing of the .h files.  Why would a very local change of
.h files be a problem compared to the gigantic #include/#define/alias
mess?  Here's the top to my gtkalias.h, btw:

extern __typeof ( )  __internal_alias __attribute((visibility("hidden"))); 
extern __typeof ( )   __attribute((alias(" __internal_alias"),
visibility("default"))); 
#define    __internal_alias 
extern __typeof ( )  __internal_alias __attribute((visibility("hidden"))); 
extern __typeof ( )   __attribute((alias(" __internal_alias"),
visibility("default"))); 
#define    __internal_alias 

That does not, shall we say, look intended, so the whole thing is fragile as
well.

Summary:

Pro-gtkalias:
* No changes to all .h files

Con-gtkalias:
* Any .h file change causes global recompile.
* Fragile perl script.
* Mucks with lots of tools like etags.





3) Is the simple #ifdef __GNUC__ check enough to protect the aliasing 
   business ? Do all versions of gcc support the visibility attribute ?

That's a "no", btw.  See bug 149757.

Hmm, my copy of gtkalias.h has 

#if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 96)

and it doesn't have any empty entries. Maybe try to regenerate it once more...

I have the preamble too -- that's just a constant in the script.  The meat,
however, is consistently starting with the two empties.  With gcc2.95.2
it doesn't get compiled, though.  Here's why the empties occur:

gtk+/gtk> cpp -P -DG_OS_UNIX -DGTK_WINDOWING_X11 -DINCLUDE_INTERNAL_SYMBOLS
gtk.symbols | od -x | head
0000000 200a 0a0a 0a0a 0a0a 0a20 0a67 746b 5f61
0000020 626f 7574 5f64 6961 6c6f 675f 6765 745f
...

you want to at least...

1. Obey configure.in's determination of pre-processor.
2. Find perl in the path or via configure.
3. next if /^\s*$/;

2. Should be a separate bug report, it is an issue with all perl scripts used in
the gtk+ build

2. is now in bug 149826.

Closing this one, as it has been done for gtk, gdk, gdk-pixbuf now. I'll open a
new bug for glib and gobject.

I'm taking the liberty of reopening, as this currently doesn't look very
Win32-friendly: makegdkalias.pl looks X11-specific (#include "x11/gdkx.h"), and
the Makefile.am line that calls it use "cpp -P gdk.symbols -DGDK_WINDOWING_X11
|" even on Win32.

Maybe the solution could be as simple as

if OS_UNIX
gdkalias.h: gdk.symbols
        cpp -P gdk.symbols -DGDK_WINDOWING_X11 | ./makegdkalias.pl > gdkalias.h
else
gdkalias.h: gdk.symbols
        echo '/* empty on Win32 */' > gdkalias.h
endif

since AFAIK it is not expensive to call exported functions from other functions
in the same Win32 DLL. Similar changes would then be needed in gdk-pixbuf/ and
gtk/ as well.

Robert, can you come up with a patch (as outlined above) to make win32 work ?

Created attachment 30525 [details] [review]
Possible patch for win32 compatibility

Here is a patch that makes it compile on Win32 (using gcc and autotools) by
making the changes to {gdk-pixbuf,gdk,gtk}/Makefile.am that was described
above. I am not completely sure this is the right approach, see question below.


I have only tested it on Win32. You might want to test it on some OS_UNIX
system.

I haven't touched any of the makefile.msc files as I don't use MSVC.

Related question (I'm no automake expert): Are the *alias.h files included in
distribution tarballs? If so, won't that cause problems on non-UNIX or non-X11
systems?

Created attachment 30528 [details] [review]
Possible patch for win32 compatibility (tweaked)

Updated to use the same contents in all *alias.h files, sorry for the
sloppiness.

Hmm, yes, gdkalias.h will be included, and the gdkx.h include in gdkalias.h will 
be a little problem for building from tarballs on platforms where
G_HAVE_GNUC_VISIBILITY && !GTK_WINDOWING_X11.

Comment on attachment 30528 [details] [review]
Possible patch for win32 compatibility (tweaked)

Then you shouldn't apply my patch, it is not a correct solution. I have just
realized that I have been even more sloppy. Compilation will actually work on
Win32 even without it, unless there is a gcc version for win32 that accepts the
visibility attribute (mine doesn't). I did the Makefile.am change when the
G_HAVE_GNUC_VISIBILITY hadn't been added and just saw the error messages caused
by the inclusion of gdkx.h, but hadn't the time to submit a bug report until
today. Now I just tested that it worked after updating without realising that
the G_HAVE_GNUC_VISIBILITY test would have taken care of it at least for me.
Bleh.

Well, maybe it will become a problem for those who want to build with the
linux-fb backend?

Feel free to close this bug again if you want. I'll go stand in the corner.
Then I'll go play somewhere else.

Matthias, don't you think this can be closed again?

Yes.