Add STRICT_ABI cmake flag to generate export lists. #117

Merged
merged 1 commit into from Sep 19, 2016

Conversation

Projects
None yet
2 participants
@iphydf
Member

iphydf commented Sep 8, 2016

Enabling this flag will generate and use an LD version script. It
ensures that the dynamic libraries (libtoxcore.so, libtoxav.so) only
export the symbols that are defined in their public API (tox.h and
toxav.h, respectively).


This change is Reviewable

@iphydf iphydf assigned nurupo and unassigned nurupo Sep 8, 2016

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 8, 2016

Member

This depends on making old group chats a supported API. On hold until then.

Member

iphydf commented Sep 8, 2016

This depends on making old group chats a supported API. On hold until then.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 8, 2016

Member

So, why not tell gcc/clang not to export all the symbols by default (like msvc++ does), mark public API with EXPORT macros like WEBCAM_CAPTURE_EXPORT in this project and tell cake to generate the right EXPORT macro expansion during build time (the macro expansion will change between building static version of the library and shared) using cmake's GenerateExportHeader module (see this commit for example, or just read cmake docs).

Member

nurupo commented Sep 8, 2016

So, why not tell gcc/clang not to export all the symbols by default (like msvc++ does), mark public API with EXPORT macros like WEBCAM_CAPTURE_EXPORT in this project and tell cake to generate the right EXPORT macro expansion during build time (the macro expansion will change between building static version of the library and shared) using cmake's GenerateExportHeader module (see this commit for example, or just read cmake docs).

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 8, 2016

Member

Mainly because libvpx is a really weird beast. In particular, its --enable-pic switch doesn't work well with static libraries on Linux/amd64. Secondarily, we split libtoxcore into 7-8 layers, each is its own .so file, so we'd need to put those export things on internal symbols as well.

A little more detail on the libvpx issue: if you want to create a statically linked libtoxav.so that contains all its dependencies (you want that e.g. for Android), you need to compile the dependencies with -fPIC but as static libraries. libvpx uses a version script for its own .so file to solve this issue (linking its non-PIC asm code into the relocatable shared object). In this PR, we copy their method. We also do this in tox4j for the same reason.


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

Member

iphydf commented Sep 8, 2016

Mainly because libvpx is a really weird beast. In particular, its --enable-pic switch doesn't work well with static libraries on Linux/amd64. Secondarily, we split libtoxcore into 7-8 layers, each is its own .so file, so we'd need to put those export things on internal symbols as well.

A little more detail on the libvpx issue: if you want to create a statically linked libtoxav.so that contains all its dependencies (you want that e.g. for Android), you need to compile the dependencies with -fPIC but as static libraries. libvpx uses a version script for its own .so file to solve this issue (linking its non-PIC asm code into the relocatable shared object). In this PR, we copy their method. We also do this in tox4j for the same reason.


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 8, 2016

Member

And yet one more clarification: the issue is that libvpx contains asm code that is not PIC enough for Linux/amd64 linkers. Hiding the symbol allows the linker to make static calls to it. Adding our own EXPORT macros would not help, because our dependency is misbehaving anyway.


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

Member

iphydf commented Sep 8, 2016

And yet one more clarification: the issue is that libvpx contains asm code that is not PIC enough for Linux/amd64 linkers. Hiding the symbol allows the linker to make static calls to it. Adding our own EXPORT macros would not help, because our dependency is misbehaving anyway.


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 9, 2016

Member

Secondarily, we split libtoxcore into 7-8 layers, each is its own .so file, so we'd need to put those export things on internal symbols as well.

I get this.

Mainly because libvpx is a really weird beast. In particular, its --enable-pic switch doesn't work well with static libraries on Linux/amd64.

I don't get this.

What libvpx has to do with this all? Building a static version of a library doesn't necessarily mean that it should have all of its dependencies statically linked into it. If you build static toxav and link it against shared libvpx — that should work. Also if you build static toxav and link it against static libvpx — that should work too, as long as libvpx can be built statically in the first place.

Btw, how .ld files are different from specifying whether to export symbols or not using compiler directives, e.g. __declspec(dllexport), __declspec(dllimport)? I'm pretty much familiar with this symbol stuff from a Windows project that I had to compile into a .dll, and MSVC++ doesn't export any symbols for .dlls, so you have to mark things you want it to export with such directives. By marking them as exported, MSVC++ generates import library file .lib with all the exports for your .dll.

Member

nurupo commented Sep 9, 2016

Secondarily, we split libtoxcore into 7-8 layers, each is its own .so file, so we'd need to put those export things on internal symbols as well.

I get this.

Mainly because libvpx is a really weird beast. In particular, its --enable-pic switch doesn't work well with static libraries on Linux/amd64.

I don't get this.

What libvpx has to do with this all? Building a static version of a library doesn't necessarily mean that it should have all of its dependencies statically linked into it. If you build static toxav and link it against shared libvpx — that should work. Also if you build static toxav and link it against static libvpx — that should work too, as long as libvpx can be built statically in the first place.

Btw, how .ld files are different from specifying whether to export symbols or not using compiler directives, e.g. __declspec(dllexport), __declspec(dllimport)? I'm pretty much familiar with this symbol stuff from a Windows project that I had to compile into a .dll, and MSVC++ doesn't export any symbols for .dlls, so you have to mark things you want it to export with such directives. By marking them as exported, MSVC++ generates import library file .lib with all the exports for your .dll.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 9, 2016

Member

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


CMakeLists.txt, line 407 [r1] (raw file):

function(make_version_script header ns lib)
  execute_process(
    COMMAND sh -c "egrep '^\\w' ${header} | grep '${ns}_[a-z_]*(' | grep -v '^typedef' | grep -o '${ns}_[a-z_]*(' | egrep -o '\\w+' | sort -u"
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation. All rights reserved.

C:\Users\nurupo>sh
"sh" is not recognized as an internal or external command,
operable program or batch file

Comments from Reviewable

Member

nurupo commented Sep 9, 2016

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


CMakeLists.txt, line 407 [r1] (raw file):

function(make_version_script header ns lib)
  execute_process(
    COMMAND sh -c "egrep '^\\w' ${header} | grep '${ns}_[a-z_]*(' | grep -v '^typedef' | grep -o '${ns}_[a-z_]*(' | egrep -o '\\w+' | sort -u"
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation. All rights reserved.

C:\Users\nurupo>sh
"sh" is not recognized as an internal or external command,
operable program or batch file

Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 9, 2016

Member

If you link libvpx statically on a platform that doesn't care as much about PIC as amd64 does, you'll end up with a libtoxav exporting all of libvpx, which you probably don't want. Building and linking libvpx is possible on systems that do care by having this explicit export list.

The difference between .ld files and symbol lists in headers is that the latter only affects those specific symbols, not the symbols defined in libraries we depend on.


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


CMakeLists.txt, line 407 [r1] (raw file):

Previously, nurupo wrote…

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation. All rights reserved.

C:\Users\nurupo>sh
"sh" is not recognized as an internal or external command,
operable program or batch file
So don't enable `-DSTRICT_ABI=ON` on Windows. It's meaningless there anyway, because we can only build static libraries for Windows.

Comments from Reviewable

Member

iphydf commented Sep 9, 2016

If you link libvpx statically on a platform that doesn't care as much about PIC as amd64 does, you'll end up with a libtoxav exporting all of libvpx, which you probably don't want. Building and linking libvpx is possible on systems that do care by having this explicit export list.

The difference between .ld files and symbol lists in headers is that the latter only affects those specific symbols, not the symbols defined in libraries we depend on.


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


CMakeLists.txt, line 407 [r1] (raw file):

Previously, nurupo wrote…

Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation. All rights reserved.

C:\Users\nurupo>sh
"sh" is not recognized as an internal or external command,
operable program or batch file
So don't enable `-DSTRICT_ABI=ON` on Windows. It's meaningless there anyway, because we can only build static libraries for Windows.

Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 9, 2016

Member

The difference between .ld files and symbol lists in headers is that the latter only affects those specific symbols, not the symbols defined in libraries we depend on.

Ah, I see.

Another question: why with the .ld file we don't have to export the symbols of the layers? i.e. why we don't have this issue:

Secondarily, we split libtoxcore into 7-8 layers, each is its own .so file, so we'd need to put those export things on internal symbols as well.

If only tox.h and toxav.h symbols are exported in the .so libraries, I'd expect this to be issue.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

Member

nurupo commented Sep 9, 2016

The difference between .ld files and symbol lists in headers is that the latter only affects those specific symbols, not the symbols defined in libraries we depend on.

Ah, I see.

Another question: why with the .ld file we don't have to export the symbols of the layers? i.e. why we don't have this issue:

Secondarily, we split libtoxcore into 7-8 layers, each is its own .so file, so we'd need to put those export things on internal symbols as well.

If only tox.h and toxav.h symbols are exported in the .so libraries, I'd expect this to be issue.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 9, 2016

Member

If we used the version script flag to link the other libraries, that would be an issue. We don't, however, since version script is only used for toxcore and toxav. If we reach the end goal of #119, then the layers will each also have a version script. For now, they just export everything. This still fixes the issue, because really only toxav needs the version script to work. I added toxcore for consistency and because I do want to have version scripts on all libraries at some point. Once a library has a stable public api, I want a version script to enforce its abi.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

Member

iphydf commented Sep 9, 2016

If we used the version script flag to link the other libraries, that would be an issue. We don't, however, since version script is only used for toxcore and toxav. If we reach the end goal of #119, then the layers will each also have a version script. For now, they just export everything. This still fixes the issue, because really only toxav needs the version script to work. I added toxcore for consistency and because I do want to have version scripts on all libraries at some point. Once a library has a stable public api, I want a version script to enforce its abi.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 10, 2016

Member

Why only toxav really needs a version script? Does libvpx have a potential to change the ABI of toxav.so (e.g. offset the addresses of all the toxav symbols) without toxav changing its own ABI? If toxav symbols are the first one in the toxav.so and the libvpx symbols come after it, libvpx shouldn't be able to change ABI of toxav.so, but if it's the other way around (or random), then i see how that might be an issue.

Member

nurupo commented Sep 10, 2016

Why only toxav really needs a version script? Does libvpx have a potential to change the ABI of toxav.so (e.g. offset the addresses of all the toxav symbols) without toxav changing its own ABI? If toxav symbols are the first one in the toxav.so and the libvpx symbols come after it, libvpx shouldn't be able to change ABI of toxav.so, but if it's the other way around (or random), then i see how that might be an issue.

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 11, 2016

Member

libvpx won't change the ABI, but linking libvpx.a into a libtoxav.so (statically) will expose libvpx symbols through libtoxav.so. This is not so bad (though also not so great), but the main problem is that the linker must expose all those symbols in the symbol table, requiring all of them to be backed by position independent code. Not all libvpx symbols are PIC, making an attempt at such linking fail.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

Member

iphydf commented Sep 11, 2016

libvpx won't change the ABI, but linking libvpx.a into a libtoxav.so (statically) will expose libvpx symbols through libtoxav.so. This is not so bad (though also not so great), but the main problem is that the linker must expose all those symbols in the symbol table, requiring all of them to be backed by position independent code. Not all libvpx symbols are PIC, making an attempt at such linking fail.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 11, 2016

Member

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

Member

nurupo commented Sep 11, 2016

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Sep 18, 2016

Member

@nurupo I've changed it a bit (using LINK_FLAGS now). PTAL.

Member

iphydf commented Sep 18, 2016

@nurupo I've changed it a bit (using LINK_FLAGS now). PTAL.

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Sep 19, 2016

Member

:lgtm_strong:


Reviewed 2 of 2 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Member

nurupo commented Sep 19, 2016

:lgtm_strong:


Reviewed 2 of 2 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Add STRICT_ABI cmake flag to generate export lists.
Enabling this flag will generate and use an LD version script. It
ensures that the dynamic libraries (libtoxcore.so, libtoxav.so) only
export the symbols that are defined in their public API (tox.h and
toxav.h, respectively).

@iphydf iphydf closed this Sep 19, 2016

@iphydf iphydf deleted the iphydf:strict-abi branch Sep 19, 2016

@iphydf iphydf merged commit 67ac913 into TokTok:master Sep 19, 2016

3 checks passed

code-review/reviewable 1/1 LGTMs
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 73.794%
Details

@iphydf iphydf modified the milestone: v0.0.1 Nov 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment