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

[Jose E. Marchesi]

> 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.

Thank you.