Re: [PATCH] libpoke: Make all symbols hidden by default

[Mohammad-Reza Nabipoor]

Hi, Jose.

On Tue, Oct 27, 2020 at 12:28:26PM +0100, Jose E. Marchesi wrote:
> 
> Hi Mohammad!
> 
> Thank you for this patch!  It certainly feels good to get rid of all
> these __attribute__ ((visibility ("hidden"))) :)
> 
> > 2020-10-26  Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
> >
> >     * bootstrap.conf: Add `lib-symbol-visibility` module to `libpoke`.
> >     * libpoke/Makefile.am: Add CPP flags to hide symbols by default.
> >     * libpoke/libpoke.h: Define `LIBPOKE_API` macro. Mark all public
> >     functions with `LIBPOKE_API`.
> >     * libpoke/ios.h: Remove `__attribute__ ((visibility ("hidden")))`.
> >     * libpoke/pkl-asm.h: Likewise.
> >     * libpoke/pkl-ast.h: Likewise.
> >     * libpoke/pkl-diag.h: Likewise.
> >     * libpoke/pkl-env.h: Likewise.
> >     * libpoke/pkl-parser.h: Likewise.
> >     * libpoke/pkl-pass.h: Likewise.
> >     * libpoke/pkl.h: Likewise.
> >     * libpoke/pkl-alloc.h: Likewise.
> >     * libpoke/pvm-program.h: Likewise.
> >     * libpoke/pvm-val.h: Likewise.
> >     * libpoke/pvm.h: Likewise.
> 
> The changes above are OK for master.  Feel free to put them in their own
> commit and push them.
> 


Pushed.


> >     * etc/libpoke-public-functions: New file. Name of all public functions
> >     of `libpoke` per line.
> 
> Let's talk a bit about the best way to validate the interface, before
> committing to maintain something like etc/libpoke-public-functions,
> 
> It seems to me that what constituted the API can the reliably be
> automatically determined from libpoke.h by looking at the places where
> LIBPOKE_API is used.  No need to maintain a separated list by hand.
> 
> At some point I imagine we will be using version files anyway, where we
> will have to list the exported symbols explicitly anyway.  But I say
> let's cross that bridge if/when we reach that river.  We don't need that
> extra complexity now that we aren't even released!
> 


OK. You're right.

I've considered the automatic extraction of API; I've dropped this idea because
I thought that check doesn't worth the effort and will not catch any bugs.
Checking that every public function in `libpoke.h`, also exists inside
the `libpoke.so` is unable to catch the mistakes.

I think we're in a good state now and maybe we don't need any kind of automatic
tests of exported symbols.
Maybe having a list of public functions is kind of over-engineering :)
I think having more tests of `libpoke` public functions is much more useful.


> Using poke to look in the shared object for the global symbols is of
> course much more preferable than using readelf! ;)


:D


> 
> > diff --git a/DEV-NEWS b/DEV-NEWS
> > index 709ccc0e..ed9c5dfd 100644
> > --- a/DEV-NEWS
> > +++ b/DEV-NEWS
> > @@ -7,3 +7,8 @@ poke.
> >    are permitted in any medium without royalty provided the copyright
> >    notice and this notice are preserved.
> >  
> > +2020-10-26
> > +   * libpoke: Make all symbols hidden by default.
> > +   * etc/libpoke-public-functions: Sorted list of all public functions of
> > +   `libpoke`. Update this file if you add/remove an API function (in
> > +   `libpoke/libpoke.h` header file).
> 
> I would keep the format of DEV-NEWS simpler, something readable at a
> glance.
> 
> What about:
> 
> 2020-10-26
> . libpoke is now built with all symbols hidden by default.  Public
>   symbols in libpoke.h are explicitly marked as such.


Thanks for the suggestion :+1:
I've changed the format.


Regards,
Mohammad-Reza