Created attachment 66244 [details] [review]
Patch with proposed changes


Note that this is the same macro approach that X.org code uses:

if defined(__GNUC__) && ((__GNUC__ * 100 + __GNUC_MINOR__) >= 303)
# define _X_EXPORT      __attribute__((visibility("default")))
# define _X_HIDDEN      __attribute__((visibility("hidden")))
# define _X_INTERNAL    __attribute__((visibility("internal")))
#elif defined(__SUNPRO_C) && (__SUNPRO_C >= 0x550)
# define _X_EXPORT      __global
# define _X_HIDDEN      __hidden
# define _X_INTERNAL    __hidden
#else /* not gcc >= 3.3 and not Sun Studio >= 8 */
# define _X_EXPORT
# define _X_HIDDEN
# define _X_INTERNAL
#endif 

And using this approach, they always use the macro at the beginning of the function definition so it works on both platforms.

Maybe it would make more sense to simply not use this macro at all and
just depend on a first underscore?  Many functions that use this macro
have an underscore as the first character and are therefore stripped out
anyway.  Perhaps better than depending on a compiler specific macro?

I'm happy to update the patch to reflect discussion...

If __hidden after the function prototype doesn't compile with Sun compiler, then G_HAVE_GNUC_VISIBILITY cannot be changed, as it's public interface and used by users of glib, so such a change will break API compatibility.

he patch does not include any changes to the G_HAVE_GNUC_VISIBILITY.  Actually I think the patch should be acceptable to commit to glib as it is.  It simply defines G_GNUC_INTERNAL when building with Sun Studio compiler and then changes the places where this macro is used so that it appears before the prototype rather than after.  GNU doesn't care about the positioning, so this change should simply work on all platforms.  Obviously Sun will need to get other modules that use this macro to move the usage (or patch the code ourselves), but I think that fixing this bug is important.  We really don't want to be exporting symbols that are meant to be hidden on any platform.  And this patch isn't very intrusive, really.  It does not break ABI when using GCC as the compiler, and Sun is prepared to work with the module maintainers to make sure any usage of this macro works with Sun Studio also.

My question about G_HAVE_GNUC_VISIBILITY was more to do with the way G_HAVE_GNUC_VISIBILITY is used in the files glib/gobjectaliasdef.c and glib/gobjectalias.h.  In these files it seems to be used because attributes are used in these files.  Why not use macros like G_GNUC_INTERNAL and get rid of the need for the G_HAVE_GNUC_VISIBILITY #ifdef here?  Just curious.

I was mostly just asking what the usage of the visibility attribute in these files do?  Does not compiling this code cause symbols to get exported that should not
when building with non-GCC compilers.

Refer to bug #350606

Also, note cairo manages this separately.  Might be good to figure out a more clever way to manage this, rather than have each module define its own set of non-GCC specific macros.

  98 #if (__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 3)) && 
      defined(__ELF__)
  99 #define cairo_private           __attribute__((__visibility__("hidden")))
 100 #elif defined(__SUNPRO_C) && (__SUNPRO_C >= 0x550)
 101 #define cairo_private           __hidden
 102 #else /* not gcc >= 3.3 and not Sun Studio >= 8 */
 103 #define cairo_private
 104 #endif

*** Bug 374730 has been marked as a duplicate of this bug. ***

In response to comment 4, I believe what Behdad was trying to say is that
G_GNUC_INTERNAL is public api, and thus using __hidden for it would break
existing uses of G_GNUC_INTERNAL which rely on it working after the 
function prototype. Admittedly, the documentation for G_GNUC_INTERNAL does not specify where to put it, and the breakage would be restricted to users of suns compiler, and --disable-visibility would be an easy way to work around such problems until the problematic software is fixed.

Regarding the protection by G_HAVE_GNUC_VISIBILITY in galias.h, we actually
need some more gcc extensions here, like __typeof and weak aliases, so it is 
unlikely that this will work with suns compiler. Or will it ?

It is true that the attached patch isn't really the ideal fix.  The best fix would be to create a series of macros that are designed to work cross-compiler.  Perhaps GLIB_VISIBILITY would be better and it could be set to work like the 
attached patch.  This macro could be designed to only be placed at the beginning
of the prototype just to ensure that its usage is more standard than the current
macro.  Then the G_GNUC_INTERNAL macro could also be set for the GCC compiler for backwards compatibility.  Then we could migrate modules to use the new better macros.

The problem with the approach discussed already is that when users try to build code, the code will fail to build with SunStudio if the macro is being used anywhere but at the beginning of the prototype.  I should mention that on Solaris, we already patch our code that defines the G_GNUC_INTERNAL macro to __hidden, so Solaris/OpenSolaris users already will experience this issue.  It hasn't seemed to cause any loud (or even quiet) complaints yet.

While this may cause some headaches for some end-users trying to build some code, it is better since Sun's ARC requires that Solaris not expose interfaces that are designed to be hidden (and hidden on other platforms).  So, for us, patching the code so that G_GNUC_INTERNAL gets set for SunStudio and making sure that we patch all the code to move the macro to the beginning, this ensures that we expose no extra functions.  In this case, when it causes build failures.  For us at Sun this build breakage is actually a good thing because we'd rather patch the code to ensure the symbols that need to be hidden are hidden than ship code that exposes symbols that should not be exposed.

Since GCC doesn't care where the macro goes, we've been trying to get our patches upstream so we can avoid patching the code.  Since this doesn't break anything, many maintainers have been agreeable to doing this, which makes our life a bit easier maintaining the patches.  It also makes the usage of the macro a bit more consistent across GNOME.

For the time being, until better macros are available, we'll likely have to keep doing this, unless people have a better suggestion that allows us to ensure we meet our requirements.

So, yes, it does break public api on Solaris, but it will be broken anyway until there is a more elegant solution.

Yes, I think the G_HAVE_* macros like G_HAVE_GNUC_VISIBILITY should be reworked so that other gcc extensions like __typeof can work in a more cross-compiler fashion.  It would be nice if these macros could be setup in a way that also work with SunStudio extensions.  Though I'd say "hidden" is probably the most important one to work in a cross-compiler fashion since not using it causes ABI and stability concerns.

Ok, I am willing to consider this change, and clarify the documentation of
G_GNUC_INTERNAL to say that use of the G_GNUC_INTERNAL macro after the function prototype is not guaranteed to be portable to compilers other than gcc.

Since this is a little bit of an API change, I'd like to see this brought up
on gtk-devel-list before we commit it.

Yes, this makes sense.  I just sent an email to gtk-devel-list explaining the change we are proposing.  Hopefully people will be agreeable on the list, and we can commit this change.

Created attachment 79020 [details] [review]
proposed documentation changes

Some comments:

"to not create inefficient PLT entries."

This is a double negative.  Probably should say "to create efficient PLT entries."  Might be nice to explain what PLT stands for.

+Expands to the GNU C <literal>visibility(hidden)</literal> attribute if 
+the compiler supports it (currently only <command>gcc</command>), or to 
+equivalent constructs supported by other compilers (e.g. 
+<literal>__hidden</literal> for Sun Studio).
+ 
+Note that for portability, the attribute should be placed before the
+function declaration, even though <command>gcc</command> accepts function
+attributes after the declaration, too.

Where the above says "other compilers" seems to suggest that this macro might be supported on other compilers besides GCC and Sun Studio, which isn't really true.  I think it might be more clear if it were worded more like this to specify how the macro works:

---

When using a compiler that supports the GNU C hidden visibility attribute, this macro expands to " __attribute__((visibility("hidden")))</literal>.  When using the Sun Studio compiler, this macro expands to <literal>__hidden</literal>.  

Note that for portability, the attribute should be placed before the function declaration for best Sun Studio portability.  While GCC allows the macro after the declaration, Sun Studio does not.

Committed to trunk

2007-01-26  Matthias Clasen <mclasen@redhat.com>

        * configure.in: Define G_GNUC_INTERNAL for Sun Studio
        as __hidden.  (#342981, Brian Cameron)

        * glib/gconvert.c:
        * glib/gutf8.c: Move G_GNUC_INTERNAL uses to the right
        spot.