[libdvdcss-devel] libdvdcss.so does not distinguish internal from exposed API

[Fabian Greffrath]

Hi again,

it seems that currently libdvdcss.so contains many internal symbols
that are not marked as such and thus confuse utilities like
dpkg-gensymbols [1].

It would be nice if libdvdcss could either use a version-script like
the following

LIBDVDCSS_2 {
global: dvdcss*;
local: *;
};

or otherwise declare the _dvdcss_*, _print_* and ioctl_* symbols as
internal/local.

AFAICT, the version-script should get saved as src/libavcodec.ver and
then passed to the linker via
"-Wl,--version-script,src/libavcodec.ver". I have however, not yet
tested this approach myself.

Best Regards,
Fabian Greffrath


[1] http://manpages.debian.net/cgi-bin/man.cgi?query=dpkg-gensymbols

[Fabian Greffrath]

Post by Fabian Greffrath
AFAICT, the version-script should get saved as src/libavcodec.ver and
then passed to the linker via
"-Wl,--version-script,src/libavcodec.ver". I have however, not yet
tested this approach myself.

Oops, please 's/libavcodec/libdvdcss/g'. Now you know where I have
stolen this. ;)

[Fabian Greffrath]

Post by Fabian Greffrath
or otherwise declare the _dvdcss_*, _print_* and ioctl_* symbols as
internal/local.

Actually, there is already code prepared for this in
src/dvdcss/dvdcss.h, lines 59-65. However, it does not seem to have
any effect on Linux systems. If I replace the instructions in these
lines with

#define LIBDVDCSS_EXPORT __attribute__ ((visibility ("default"))) extern

and compile with "CFLAGS += -fvisibility=hidden" then everything comes
out as expected and the library only exposes its actual public API.

- Fabian

[Fabian Greffrath]

Alright, my last post for today.

The attached patch sets the appropriate visibility attributes for the
exported symbols when compiled with GCC (>= 4). It requires the code
to get compiled with -fvisibility=hidden to take effect, though.

I think the src/Makefile.am part of the patch may require some
conditionals to check if the used compiler is GCC 4.x. It may as well
get excluded from the patch and left for the user/packager to add to
the actual build/packaging process.

- Fabian

[Jean-Baptiste Kempf]

Post by Fabian Greffrath
I think the src/Makefile.am part of the patch may require some
conditionals to check if the used compiler is GCC 4.x. It may as
well get excluded from the patch and left for the user/packager to
add to the actual build/packaging process.

You need a modification in configure.ac to check if the compiler
supports it.

http://svn.xiph.org/trunk/speex/configure.ac
http://git.videolan.org/?p=vlc.git;a=blob;f=configure.ac;hb=HEAD

Are examples on how to do it.

Best regards,

[Fabian Greffrath]

Post by Jean-Baptiste Kempf
You need a modification in configure.ac to check if the compiler
supports it.
http://svn.xiph.org/trunk/speex/configure.ac
http://git.videolan.org/?p=vlc.git;a=blob;f=configure.ac;hb=HEAD
Are examples on how to do it.

I am back late but would like to suggest a new patch to support symbol
visibility. It is taken more or less from the xiph.org implementation
and works well on my system.

I am kindly asking for a review.

- Fabian

[Reimar Döffinger]

Post by Fabian Greffrath

Post by Jean-Baptiste Kempf
You need a modification in configure.ac to check if the compiler
supports it.
http://svn.xiph.org/trunk/speex/configure.ac
http://git.videolan.org/?p=vlc.git;a=blob;f=configure.ac;hb=HEAD
Are examples on how to do it.

I am back late but would like to suggest a new patch to support
symbol visibility. It is taken more or less from the xiph.org
implementation and works well on my system.
I am kindly asking for a review.

Personally I find the linker script approach that FFmpeg uses
preferable.
Though I admit it does not make so much sense since we already
have that LIBDVDCSS_EXPORT code.
Do you know if the configure check does the right thing on MinGW or
cygwin? Those compilers might support both visibility and dllexport
and they might have different semantics.

[Fabian Greffrath]

Post by Reimar Döffinger
Do you know if the configure check does the right thing on MinGW or
cygwin? Those compilers might support both visibility and dllexport
and they might have different semantics.

Well, according to [1] (which is an excellent read, btw, highly
recommended!) things are a bit compilcated, but solvable:

It seems that both _EXPORTS or _IMPORTS are only #defined by MSVC, so
if neither of them is #defined we are most probably using GCC and thus
either building for _WIN32 (i.e. MinGW), __CYGWIN__ or Unix. Since
both _WIN32 and __CYGWIN__ seem to understand the __declspec()
semantics, it should be sufficient if none of them is #defined to test
for (__GNUC__ >= 4) to be sure we are building on Unix with a
sufficiently recent compiler.

At least this is what I read from the step-by-step guide.

So the attached implementation first checks if _WIN32 or __CYGWIN__
are #defined and then processes the whole Windows-centric block that
was already there. If otherwise __GNUC__ is #defined and higher than
4, then the symbol visibility attribute is applied. If neither _WIN32
nor __CYGWIN__ nor __GNUC__ is #defined, then we fall back to the
"extern" key word.

Both MinGW and Cygwin, which use GCC and thus have __GNUC__ #defined,
enter the first block, because the symbol visibility feature is
restricted to ELF and thus not suitable for Windows.

PS: This should allow for a somewhat simpler check in configure (at
least without an additional #define), but I have not yet got so far.

- Fabian

[1] http://gcc.gnu.org/wiki/Visibility

[Fabian Greffrath]

Hello Reimar,

I have changed my mind. Sorry, but I think I was a bit hastly after
reading the GNU document when I proposed the patch yesterday. All this
second-guessing of specific architectures and compiler versions only
makes things more error-prone and less intuitive.

All we need to know is if the currently used compiler supports ELF
visibility - not its version nor architecture, not even if it compiles
to ELF. We check this in the configure script and then somehow need to
get this information into the header. I could not think up of a
different solution than introducing a new #define, which is then
checked for by the preprocessor. This means that this information is
only available at build time of the library itself, but IMHO this is -
in contrast to the __declspec() declarations used by WIN32 and CYGWIN
compilers - perfectly suitable (because symbols are either hidden or
exported at library build time).

Regarding the __declspec() declaration, both MinGW and CYGWIN (not to
even speak of MSVC) compilers are GCC but do not produce ELF object
code and thus - since visibility is an exclusive ELF feature - cannot
support ELF visibility (confirmed myself with mingw at least). So they
have to use the __declspec() declaration, depending on whether
LIBDVDCSS_EXPORTS or LIBDVDCSS_IMPORTS is #defined. If none is them is
defined, something is seriously borked anyway and the preprocessor
falls back to "extern", but it was already like this before.

So please accept the attached patch as my hopefully final approach for
this issue. :)

I have tested building libdvdcss on a current Debian unstable system
with and without my patch and it works, i.e. with the patch 23 symbols
from the _dvdcss_*, ioctl_* and _print_error name space are hidden.

Best regards,
Fabian

[Fabian Greffrath]

Post by Fabian Greffrath
So please accept the attached patch as my hopefully final approach for
this issue. :)

I am serious about this patch, could I please get a review? ;)

[Fabian Greffrath]

Post by Fabian Greffrath
I am serious about this patch, could I please get a review? ;)

Hello-o?!

The patch in question is attached to my message of July 11th. Could
someone please review it?

Thanks,
Fabian