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

Hide non-API functions #2895

Open
wants to merge 2 commits into
base: v1.x
Choose a base branch
from

Conversation

urisimchoni
Copy link

@urisimchoni urisimchoni commented Oct 9, 2023

Usage of -fvisibility=hidden is the recommended default for DSO's, and is harmless in statically-linked objects.

If the spdlog build is for a shared library, then API symbols have SPDLOG_API marking on them, and cause the symbol to be exported, with other symbols now hidden by default.

If the build is for a static library with position-independent code, then spdlog is to be embedded as implementation detail in another shared library or executable. In that case we don't want to export any symbol. Conveniently, SPDLPG_API in that case resolves to nothing, so with the default of hidden all symbols are made hidden.

Usage of -fvisibility=hidden is the recommended default for DSO's.

If the spdlog build is for a shared library, then API symbols have
SPDLOG_API marking on them, and cause the symbol to be exported, with
other symbols now hidden by default.

If the build is for a static library with position-independent code, then
spdlog is to be embedded as implementation detail in another shared
library or executable. In that case we don't want to export any symbol.
Conveniently, SPDLPG_API in that case resolves to nothing, so with the
default of hidden all symbols are made hidden.
@gabime
Copy link
Owner

gabime commented Oct 10, 2023

Thanks. Why only in PIC ? I think it can be used without pic as well.

@urisimchoni
Copy link
Author

Thanks. Why only in PIC ? I think it can be used without pic as well.

I suppose it's not really tied to PIC, although the context in which it matters is dynamically-linked executables, which require PIC. Without the condition, the -fvisibility=hidden flag is given to static builds as well, where it should have no effect, so I suppose I can remove the condition.

@urisimchoni urisimchoni changed the title Hide non-API functions when position independent code is enabled Hide non-API functions Oct 10, 2023
@gabime
Copy link
Owner

gabime commented Oct 10, 2023

We need to be careful since the bundled fmt use default visibilty. Not sure what would happen. i.e. If users of spdlog will be able to use the bundled fmt lib. Could you check?

@urisimchoni
Copy link
Author

If I understand correctly, the concern is that an aplication should be able to use fmt::XXX embedded in the shared libspdlog.so.
I ran the following quick check:

  • Add the following lines to example.cpp:
    auto s = fmt::format("i={}",1);
    printf("%s\n", s.c_str());
  • Build spdlog with cmake -DSPDLOG_BUILD_SHARED=ON -DSPDLOG_BUILD_PIC=ON ../spdlog && make -j
  • Run the example

I can also see that libspdlog.so built that way exports some fmt symbols.

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