GNOME Bugzilla – Bug 342981
Changed needed to support interface hiding with Sun Studio compiler
Last modified: 2007-01-27 03:38:19 UTC
The G_GNUC_INTERNAL macro is used to hide interfaces so that they are not exported. However, this only works with the GCC compiler. The Sun Studio
compiler supports the "__hidden" keyword to do the same thing. However,
Sun Studio requires that this keyword be placed *before* the function prototype.
GCC is more flexible and allows the __attribute((visibility("hidden")))
attribute to be specified anywhere (before or after) the prototype.
This patch fixes configure so that it recognizes the Forte compiler and
sets the G_GNUC_INTERNAL macro to "__hidden" on this platform. I also
modified some header and source files so that this macro is specified at
the beginning of the prototype instead of at the end. This will work just
as well on Linux, but will also mean that on Solaris users will not see
unintended exported symbols.
I do have one question, though. I notice that in the files glib/gobjectaliasdef.c and glib/gobjectalias.h that many things specify the __attribute((visibility("hidden"))) attribute. On Solaris this code is never
looked at since it is all contained in "#ifdef G_HAVE_GNUC_VISIBILITY". It's not clear to me what impact this might have on Solaris interfaces being different.
I tried just removing the #ifdef G_HAVE_GNUC_VISIBILITY and using the G_GNUC_INTERNAL macro instead of the "hidden" attribute, but then the compiler started complaining that variables were being referenced that were out of scope so it seemed like the IN_FILE and IN_HEADER macros weren't working right or something. Not sure what to think about this, or if it is even important.
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
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)) &&
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
*** 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
"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 <email@example.com>
* configure.in: Define G_GNUC_INTERNAL for Sun Studio
as __hidden. (#342981, Brian Cameron)
* glib/gutf8.c: Move G_GNUC_INTERNAL uses to the right