Re: [Xfce4-commits] <xfce4-dev-tools:master> Don't set the default visibility in XDT_FEATURE_VISIBILITY.

[Brian J. Tarricone-3]

Nick, revert this please.  If you don't want to use visibility properly
in your module, then don't use the macro.  Don't make it useless for
everyone else.

On 10/22/2009 02:22 AM, Nick Schermer wrote:

> Updating branch refs/heads/master
>          to 77d9703dccee853d007c3b7133bc14accff66203 (commit)
>        from 399b5f31fba3b76767041bdd0d7ad57c53ee1866 (commit)
>
> commit 77d9703dccee853d007c3b7133bc14accff66203
> Author: Nick Schermer <nick@...>
> Date:   Thu Oct 22 11:18:06 2009 +0200
>
>     Don't set the default visibility in XDT_FEATURE_VISIBILITY.
>    
>     Only use XDT_FEATURE_VISIBILITY to detect if visibility is
>     supported by the compiler. Setting the default visibility
>     to hidden will break libraries and modules that don't
>     set the visibility of their public function to "default".
>
>  m4macros/xdt-features.m4 |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/m4macros/xdt-features.m4 b/m4macros/xdt-features.m4
> index ea0c685..be355b6 100644
> --- a/m4macros/xdt-features.m4
> +++ b/m4macros/xdt-features.m4
> @@ -164,8 +164,6 @@ AC_DEFUN([XDT_FEATURE_VISIBILITY],
>  
>    if test "x$have_gnuc_visibility" = "xyes"; then
>      CPPFLAGS="$CPPFLAGS -DHAVE_GNUC_VISIBILITY"
> -    XDT_SUPPORTED_FLAGS([xdt_vis_hidden_cflags], [-fvisibility=hidden])
> -    CFLAGS="$CFLAGS $xdt_vis_hidden_cflags"
>    fi
>  
>    AM_CONDITIONAL([HAVE_GNUC_VISIBILITY], [test "x$have_gnuc_visibility" = "xyes"])
> _______________________________________________
> Xfce4-commits mailing list
> Xfce4-commits@...
> http://foo-projects.org/mailman/listinfo/xfce4-commits

_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/22 Brian J. Tarricone <brian@...>:
> Nick, revert this please.  If you don't want to use visibility properly
> in your module, then don't use the macro.  Don't make it useless for
> everyone else.

Define useless. You think this changes anything in, for example, xfconf?

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/22 Nick Schermer <nickschermer@...>:
> 2009/10/22 Brian J. Tarricone <brian@...>:
>> Nick, revert this please.  If you don't want to use visibility properly
>> in your module, then don't use the macro.  Don't make it useless for
>> everyone else.
>
> Define useless. You think this changes anything in, for example, xfconf?

Let's put in another way, it is better to set the default visibility
level for the library you compile if you want to use this. The global
visibility makes this unusable for thunar (break modules), exo (gio
module and private hal library), 4ui (private kbd lib) and the panel
(all plugins). So that leaves use with xfconf and 4util.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jannis Pohlmann-3]

On Thu, 22 Oct 2009 13:50:12 +0200
Nick Schermer <nickschermer@...> wrote:

> 2009/10/22 Nick Schermer <nickschermer@...>:
> > 2009/10/22 Brian J. Tarricone <brian@...>:
> >> Nick, revert this please.  If you don't want to use visibility
> >> properly in your module, then don't use the macro.  Don't make it
> >> useless for everyone else.
> >
> > Define useless. You think this changes anything in, for example,
> > xfconf?
>
> Let's put in another way, it is better to set the default visibility
> level for the library you compile if you want to use this. The global
> visibility makes this unusable for thunar (break modules), exo (gio
> module and private hal library), 4ui (private kbd lib) and the panel
> (all plugins).

You mean break as in: the symbols of these libraries and modules are
hidden with -fvisibility=hidden because we don't properly export
symbols yet? How about we force people/ourselves to export their public
symbols properly rather than removing something that's actually good
for everyone?

Please correct me if I'm wrong.

  - Jannis


_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/22 Jannis Pohlmann <jannis@...>:

> On Thu, 22 Oct 2009 13:50:12 +0200
> Nick Schermer <nickschermer@...> wrote:
>
>> 2009/10/22 Nick Schermer <nickschermer@...>:
>> > 2009/10/22 Brian J. Tarricone <brian@...>:
>> >> Nick, revert this please.  If you don't want to use visibility
>> >> properly in your module, then don't use the macro.  Don't make it
>> >> useless for everyone else.
>> >
>> > Define useless. You think this changes anything in, for example,
>> > xfconf?
>>
>> Let's put in another way, it is better to set the default visibility
>> level for the library you compile if you want to use this. The global
>> visibility makes this unusable for thunar (break modules), exo (gio
>> module and private hal library), 4ui (private kbd lib) and the panel
>> (all plugins).
>
> You mean break as in: the symbols of these libraries and modules are
> hidden with -fvisibility=hidden because we don't properly export
> symbols yet? How about we force people/ourselves to export their public
> symbols properly rather than removing something that's actually good
> for everyone?

Why force if it can only break things? We need to add macros for
__attribute((visibility("default"))) and prefix all the functions we
want to export. That sounds like a lot of unneeded work to me.

So maybe put it in a XDT_VISIBILITY_CFLAGS variable, so it can me used
where needed, without breaking modules.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On Thu, Oct 22, 2009 at 04:50, Nick Schermer <nickschermer@...> wrote:

> 2009/10/22 Nick Schermer <nickschermer@...>:
>> 2009/10/22 Brian J. Tarricone <brian@...>:
>>> Nick, revert this please.  If you don't want to use visibility properly
>>> in your module, then don't use the macro.  Don't make it useless for
>>> everyone else.
>>
>> Define useless. You think this changes anything in, for example, xfconf?
>
> Let's put in another way, it is better to set the default visibility
> level for the library you compile if you want to use this. The global
> visibility makes this unusable for thunar (break modules), exo (gio
> module and private hal library), 4ui (private kbd lib) and the panel
> (all plugins). So that leaves use with xfconf and 4util.

By using that autoconf macro you are saying "I want to use the
compiler's visibility features to only export public API from the
library."  Obviously, if you are going to do this, you must define
what's public.  If you are not going to take the time to define what's
public, then you should not use this macro.

Furthermore, with your change, there is no straightforward way of
setting the default visibility to hidden if that's what you want.
Well, ok, you can check the value of $have_gnuc_visibility in
configure.ac, but that's retarded.

I would consider (one of) a couple changes from my original macro:

1.  Allowing the macro to take an argument specifying the default
visibility (defaults to "hidden" if not specified).
2.  Allowing the macro to take an argument specifying a variable to
stuff the visibility argument in, so you can add it (or not add it) to
different targets in the module (assuming there's more than one).  If
not specified, it defaults to putting the vis parameter in CFLAGS.

Regardless: revert your change.  And in the future, at least do me the
common courtesy of discussing changes to my code in a module I
maintain before checking things in.  Jannis convinced me we can all
behave properly without having tighter commit permissions on git;
please don't prove him wrong.

     -brian
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On Thu, Oct 22, 2009 at 06:50, Nick Schermer <nickschermer@...> wrote:

> Why force if it can only break things? We need to add macros for
> __attribute((visibility("default"))) and prefix all the functions we
> want to export. That sounds like a lot of unneeded work to me.

Would this still work with alias generation?

I'm not sure that I like this due to the macro bloat in the header
files.  I'd rather not put what amounts to implementation details in
the public header.

     -brian
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Jannis Pohlmann-3]

On Thu, 22 Oct 2009 14:16:19 -0700
"Brian J. Tarricone" <brian@...> wrote:

> On Thu, Oct 22, 2009 at 04:50, Nick Schermer <nickschermer@...>
> wrote:
> > 2009/10/22 Nick Schermer <nickschermer@...>:
> >> 2009/10/22 Brian J. Tarricone <brian@...>:
> >>> Nick, revert this please.  If you don't want to use visibility
> >>> properly in your module, then don't use the macro.  Don't make it
> >>> useless for everyone else.
> >>
> >> Define useless. You think this changes anything in, for example,
> >> xfconf?
> >
> > Let's put in another way, it is better to set the default visibility
> > level for the library you compile if you want to use this. The
> > global visibility makes this unusable for thunar (break modules),
> > exo (gio module and private hal library), 4ui (private kbd lib) and
> > the panel (all plugins). So that leaves use with xfconf and 4util.
>
> By using that autoconf macro you are saying "I want to use the
> compiler's visibility features to only export public API from the
> library."  Obviously, if you are going to do this, you must define
> what's public.  If you are not going to take the time to define what's
> public, then you should not use this macro.
>
> Furthermore, with your change, there is no straightforward way of
> setting the default visibility to hidden if that's what you want.
> Well, ok, you can check the value of $have_gnuc_visibility in
> configure.ac, but that's retarded.
>
> I would consider (one of) a couple changes from my original macro:
>
> 1.  Allowing the macro to take an argument specifying the default
> visibility (defaults to "hidden" if not specified).
> 2.  Allowing the macro to take an argument specifying a variable to
> stuff the visibility argument in, so you can add it (or not add it) to
> different targets in the module (assuming there's more than one).  If
> not specified, it defaults to putting the vis parameter in CFLAGS.

I'm thinking back and forth here... Nick's concern about
-fvisibility=hidden is that exo needs HAVE_GNUC_VISIBILITY for the
alias generation but exo's .symbols file has a few exceptions (e.g.
exo_atomic_{inc,dec}) which are not declared visible when
-fvisiblity=hidden is passed to the compiler. In consequence they don't
show up in the .so file even though they would (without -fvisib...)
because they'd not be hidden and would pass -export-regex-symbols as
well. So there are expections where the alias stuff is used but
-fvisibility=hidden is not desired.

Soo, for the entire afternoon I was thinking, yeah, let's drop
-fvisibility=hidden from XDT_FEATURE_VISIBILITY(). However, without it
XDT_FEATURE_VISIBILITY() doesn't do much anymore. I think it's all
about how it is interpreted. If the entire point of
XDT_FEATURE_VISIBILITY() is seen in hiding all symbols if possible,
then yes, -fvisibility=hidden has to go back in.

I can also see how we could make it useful and more flexible by adding
a parameter to it (e.g. a compiler flag variable), like you suggested
on bugzilla.xfce.org. For aliasing, libraries need HAVE_GNUC_VISIBILITY
but they may or may not want -fvisibility=hidden.

> Regardless: revert your change.  And in the future, at least do me the
> common courtesy of discussing changes to my code in a module I
> maintain before checking things in.  Jannis convinced me we can all
> behave properly without having tighter commit permissions on git;
> please don't prove him wrong.

Yes, I think I agree. Revert for now, especially since we're not using
XDT_FEATURE_VISIBILITY() in exo anymore after the removal of the alias
stuff has been reverted as well.

  - Jannis


_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On Thu, Oct 22, 2009 at 15:10, Jannis Pohlmann <jannis@...> wrote:

>
> I'm thinking back and forth here... Nick's concern about
> -fvisibility=hidden is that exo needs HAVE_GNUC_VISIBILITY for the
> alias generation but exo's .symbols file has a few exceptions (e.g.
> exo_atomic_{inc,dec}) which are not declared visible when
> -fvisiblity=hidden is passed to the compiler. In consequence they don't
> show up in the .so file even though they would (without -fvisib...)
> because they'd not be hidden and would pass -export-regex-symbols as
> well. So there are expections where the alias stuff is used but
> -fvisibility=hidden is not desired.

I don't think I quite understand.  Why are the atomic ops not declared
visible with -fvisibility=hidden?

> Soo, for the entire afternoon I was thinking, yeah, let's drop
> -fvisibility=hidden from XDT_FEATURE_VISIBILITY(). However, without it
> XDT_FEATURE_VISIBILITY() doesn't do much anymore. I think it's all
> about how it is interpreted. If the entire point of
> XDT_FEATURE_VISIBILITY() is seen in hiding all symbols if possible,
> then yes, -fvisibility=hidden has to go back in.

Well, yes.  Personally I'm not too concerned with all the hand-wavy
PLT performance stuff, so, to me, the primary purpose of this exercise
is to only export symbols that are intended to be in the public API.
There are a few nice linking related performance gains involved in
doing only that, but those are incidental as far as I'm concerned.

Now, on top of that, we do the aliasing stuff, which requires
visibility support to work (but does not require the default
-fvisibility=hidden).  The idea is that we declare an alias to each
public symbol (prefix the name with "IA__"), and declare the
visibility of the alias as hidden.  Then (only for internal use in the
library's .c files) we #define the real symbol name to the prefixed
alias name.  This way, library-internal use of the public API doesn't
have to go through a couple redirections to "find" the functions.
Sure, that's nice to do, but it's debatable how much it actually buys
you, since it really depends on the library and whether or not it
needs to often (or rather, in potentially critical paths) call its own
API.

> I can also see how we could make it useful and more flexible by adding
> a parameter to it (e.g. a compiler flag variable), like you suggested
> on bugzilla.xfce.org. For aliasing, libraries need HAVE_GNUC_VISIBILITY
> but they may or may not want -fvisibility=hidden.

Right, that's a reasonable change that keeps the current default but
makes it flexible enough for other use.

     -brian
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Nick Schermer]

2009/10/23 Brian J. Tarricone <brian@...>:

> On Thu, Oct 22, 2009 at 15:10, Jannis Pohlmann <jannis@...> wrote:
>>
>> I'm thinking back and forth here... Nick's concern about
>> -fvisibility=hidden is that exo needs HAVE_GNUC_VISIBILITY for the
>> alias generation but exo's .symbols file has a few exceptions (e.g.
>> exo_atomic_{inc,dec}) which are not declared visible when
>> -fvisiblity=hidden is passed to the compiler. In consequence they don't
>> show up in the .so file even though they would (without -fvisib...)
>> because they'd not be hidden and would pass -export-regex-symbols as
>> well. So there are expections where the alias stuff is used but
>> -fvisibility=hidden is not desired.
>
> I don't think I quite understand.  Why are the atomic ops not declared
> visible with -fvisibility=hidden?

They are not real functions and same for internal variables
(exo_version_*), there the alias code is not suitable too. So, the
visibility macro is nice for xfconf because it only has a simple
library with public functions only, everything else in Xfce needs to
adapt with crapy visibility code.

>> I can also see how we could make it useful and more flexible by adding
>> a parameter to it (e.g. a compiler flag variable), like you suggested
>> on bugzilla.xfce.org. For aliasing, libraries need HAVE_GNUC_VISIBILITY
>> but they may or may not want -fvisibility=hidden.
>
> Right, that's a reasonable change that keeps the current default but
> makes it flexible enough for other use.

Well I think that has not real purpose too. So commit reverted and
thanks for the unusable macro. I'll add the normal visibility code to
all the other libraries.

Nick
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev

[Brian J. Tarricone-3]

On Fri, Oct 23, 2009 at 00:45, Nick Schermer <nickschermer@...> wrote:

> 2009/10/23 Brian J. Tarricone <brian@...>:
>
>> I don't think I quite understand.  Why are the atomic ops not declared
>> visible with -fvisibility=hidden?
>
> They are not real functions and same for internal variables
> (exo_version_*), there the alias code is not suitable too. So, the
> visibility macro is nice for xfconf because it only has a simple
> library with public functions only, everything else in Xfce needs to
> adapt with crapy visibility code.

That doesn't really make sense.  The exo_atomic_* functions are inline
where supported, so those will always work.  Where not supported,
they're just preprocessor defines for the g_atomic_* equivalents.  The
alias stuff shouldn't affect them at all.  If it does, exo is broken.

(Why do the atomic ops in exo exist anyway?  What's wrong with the
ones in glib?)

For the exo_version_* variables, IIRC the visibility attributes can
apply to them, too.  You might need to change the aliasdef generation
script, though, since I'm pretty sure it assumes all symbols in the
.symbols file are functions.

>>> I can also see how we could make it useful and more flexible by adding
>>> a parameter to it (e.g. a compiler flag variable), like you suggested
>>> on bugzilla.xfce.org. For aliasing, libraries need HAVE_GNUC_VISIBILITY
>>> but they may or may not want -fvisibility=hidden.
>>
>> Right, that's a reasonable change that keeps the current default but
>> makes it flexible enough for other use.
>
> Well I think that has not real purpose too. So commit reverted and
> thanks for the unusable macro. I'll add the normal visibility code to
> all the other libraries.

Fine, if you want to be obstinate and not want to try to work it out,
that's your choice.  I made some suggestions how we can make it more
useful and flexible, but I guess you aren't interested in actual
solutions.

     -brian
_______________________________________________
Xfce4-dev mailing list
Xfce4-dev@...
http://foo-projects.org/mailman/listinfo/xfce4-dev