Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix symbol visibility on Linux when compiling with -fvisibility=hidden #1535

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

milianw
Copy link
Contributor

@milianw milianw commented Jan 27, 2020

Make FMT_API symbols use the default visibility on non-Windows
platforms. Otherwise, one cannot use the generated fmt library when
compiling globally with -fvisibility=hidden.

Fixes compile errors like:

../3rdParty/fmt/include/fmt/core.h:757: error: undefined reference to 'fmt::v6::internal::assert_fail(char const*, int, char const*)'

Note that the symbol exists, but is local:

$ nm -C libfmtd.so.6.1.3  | grep assert_fail
                 U __assert_fail
0000000000233ffa t fmt::v6::internal::assert_fail(char const*, int, char const*)

With this patch, the compile error is gone and the symbol is properly
exported:

$ nm -a bin/libfmtd.so -C | grep assert_fail
                 U __assert_fail
00000000002366ba T fmt::v6::internal::assert_fail(char const*, int, char const*)

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

@vitaut
Copy link
Contributor

vitaut commented Jan 27, 2020

Thanks for the PR. Looks good but please fix the CI errors.

@milianw
Copy link
Contributor Author

milianw commented Jan 27, 2020

I'm quite puzzled by the CI errors, since I cannot reproduce them locally. Apparently it's testing some different compile configuration there than what I have locally. I'll have to find the time to figure out what's going on here. If you have any hints that would be appreciated.

@milianw
Copy link
Contributor Author

milianw commented Jan 27, 2020

Ah, -Werror explains it - I do see it locally after all...

@milianw milianw force-pushed the fix_compile_with_visibility_hidden branch from dcd956f to c1ac04e Compare January 27, 2020 14:52
@milianw
Copy link
Contributor Author

milianw commented Jan 27, 2020

this should fix the linux build, but I wonder if the FMT_API in the .cc file was required for Windows?

@vitaut
Copy link
Contributor

vitaut commented Jan 27, 2020

I wonder if the FMT_API in the .cc file was required for Windows?

AFAIK yes, it is required by MSVC. I suggest introducing another macro (sigh) FMT_INSTANTIATION_DEF and using it on the instantiation definition instead of FMT_API.

@milianw
Copy link
Contributor Author

milianw commented Jan 28, 2020

With the AppVeyor build passing... do we really need this? Shouldn't that test this API too, just like the Linux CI on Travis did?

@vitaut
Copy link
Contributor

vitaut commented Jan 28, 2020

do we really need this?

We do. CI doesn't test all possible configurations, just several common ones.

@milianw milianw force-pushed the fix_compile_with_visibility_hidden branch from c1ac04e to a8eddc2 Compare January 28, 2020 16:22
Make FMT_API symbols use the default visibility on non-Windows
platforms. Otherwise, one cannot use the generated fmt library when
compiling globally with -fvisibility=hidden.

Fixes compile errors like:

```
../3rdParty/fmt/include/fmt/core.h:757: error: undefined reference to 'fmt::v6::internal::assert_fail(char const*, int, char const*)'
```

Note that the symbol exists, but is local:

```
$ nm -C libfmtd.so.6.1.3  | grep assert_fail
                 U __assert_fail
0000000000233ffa t fmt::v6::internal::assert_fail(char const*, int, char const*)
```

With this patch, the compile error is gone and the symbol is properly
exported:

```
$ nm -a bin/libfmtd.so -C | grep assert_fail
                 U __assert_fail
00000000002366ba T fmt::v6::internal::assert_fail(char const*, int, char const*)
```

Change-Id: I96054e622d9a2ae81907e1b01a1033e629767a91
@milianw milianw force-pushed the fix_compile_with_visibility_hidden branch from a8eddc2 to 7977db4 Compare January 30, 2020 08:29
@vitaut vitaut merged commit 314e150 into fmtlib:master Jan 30, 2020
@vitaut
Copy link
Contributor

vitaut commented Jan 30, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants