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 module symbols by default #340

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

MaddTheSane
Copy link
Contributor

@MaddTheSane MaddTheSane commented May 13, 2024

Pull Request Type

  • GitHub Workflow changes
  • Documentation or Wiki changes
  • Build and Dependency changes
  • Runtime changes
    • Render changes
    • Audio changes
    • Input changes
    • Network changes
    • Other changes

Description

This changes the visibility of the symbols of the plug-ins, the net games, and the TCP/IP net module. This can be great for LTO and dead code stripping, as only exported symbols (and symbols that the exported symbols reference) are included, reducing the size of linked objects.

Checklist

  • I have tested my changes locally and verified that they work as intended.
  • I have documented any new or modified functionality.
  • I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
  • I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

Due to how DMFC is statically-linked and built for Linux and macOS, a lot of the DMFC symbols will still be exported. The next part would be changing visibility of the DMFC library for macOS and Linux to not export its symbols if they're not building for Windows.

@Lgt2x Lgt2x added the cleanup Code cleanup label May 13, 2024
@MaddTheSane MaddTheSane changed the title Hide symbols by default Hide module symbols by default May 14, 2024
@MaddTheSane
Copy link
Contributor Author

An alternative to marking specific symbols as exportable in the source code is to have an exported symbols list that is passed to the linker. The problem is, macOS and Windows start each symbol with an underscore, while Linux/other UNIXes do not. This would resolve the DMFC symbols being exported, though.

@JeodC
Copy link
Collaborator

JeodC commented May 20, 2024

I did some research on this subject and came up with some possible downsides:

  • Compatibility-wise we can't test this yet because netgames aren't fully working (last I tried). You ticked the box for that though--did you test with retail netgames, then?
  • Reduced visibility will make debugging more of a challenge in an area we haven't touched extensively. The filesizes of these things aren't overtly huge, so is hidden symbols really warranted at this time?

@MaddTheSane
Copy link
Contributor Author

  • It doesn't just target net games, but also the level scripts.
  • If you feel like this is too soon, you can wait until the project is a bit more mature.

@JeodC
Copy link
Collaborator

JeodC commented May 20, 2024

  • It doesn't just target net games, but also the level scripts.
  • If you feel like this is too soon, you can wait until the project is a bit more mature.

@jcoby @winterheart @Arcnor What do you think?

@winterheart
Copy link
Collaborator

I have little experience on dll using, but changes looks fine for me.

@@ -4,6 +4,7 @@ set(CPPS EntropyBase.cpp EntropyPackets.cpp EntropyRoom.cpp)
set(NETGAME_MODULE entropy)

add_library(${NETGAME_MODULE} MODULE ${CPPS} ${HEADERS})
set_target_properties(${NETGAME_MODULE} PROPERTIES CXX_VISIBILITY_PRESET "hidden")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about writing a custom CMake command for netgames that are all built the same way, maybe for a future contribution

@Lgt2x Lgt2x merged commit e66c359 into DescentDevelopers:main May 22, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants