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

GDNative: Fix symbols visibility for GCC #44671

Merged
merged 1 commit into from
Mar 12, 2021
Merged

GDNative: Fix symbols visibility for GCC #44671

merged 1 commit into from
Mar 12, 2021

Conversation

o01eg
Copy link
Contributor

@o01eg o01eg commented Dec 25, 2020

I'm building native library with CMake and found out default visibility settings make it unusable with Godot because symbols are hidden.
This PR makes exported functions visible when compiling them with GCC.

Copy of godotengine/godot-headers#81

@Chaosus Chaosus added this to the 4.0 milestone Dec 25, 2020
@o01eg
Copy link
Contributor Author

o01eg commented Feb 22, 2021

Are there any news regarding this PR?

@akien-mga akien-mga requested review from hpvb and a team February 22, 2021 09:04
@touilleMan
Copy link
Member

Would it be useful to add something similar to the GDAPI macro ? I guess it depends on the flags used when compiling Godot but from what I understand it could help with final binary size
Not sure why, but currently only iOS use the custom visibility attribute:

#if defined(_WIN32) || defined(__ANDROID__)
#define GDCALLINGCONV
#define GDAPI GDCALLINGCONV
#elif defined(__APPLE__)
#include "TargetConditionals.h"
#if TARGET_OS_IPHONE
#define GDCALLINGCONV __attribute__((visibility("default")))
#define GDAPI GDCALLINGCONV
#elif TARGET_OS_MAC
#define GDCALLINGCONV __attribute__((sysv_abi))
#define GDAPI GDCALLINGCONV
#endif
#else // !_WIN32 && !__APPLE__
#define GDCALLINGCONV __attribute__((sysv_abi))
#define GDAPI GDCALLINGCONV
#endif

@akien-mga akien-mga changed the title Fix visibility for GCC GDNative: Fix symbols visibility for GCC Feb 22, 2021
@BastiaanOlij
Copy link
Contributor

Would it be useful to add something similar to the GDAPI macro ? I guess it depends on the flags used when compiling Godot but from what I understand it could help with final binary size
Not sure why, but currently only iOS use the custom visibility attribute:

I could be completely wrong but that may be a left over from earlier times, don't we statically link GDNative modules in iOS? Or has that changed since I last used GDNative in iOS

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

I don't use GCC myself so i can't test it but this change seems sound. Functions marked as GDN_EXPORT need to be set to visible so if GCC does not do so by default then this seems like the right way to fix it to me.

@akien-mga
Copy link
Member

I could be completely wrong but that may be a left over from earlier times, don't we statically link GDNative modules in iOS? Or has that changed since I last used GDNative in iOS

There's now proper support for dynamically linking GDNative on iOS, implemented by @naithar: #39996.

@BastiaanOlij
Copy link
Contributor

There's now proper support for dynamically linking GDNative on iOS, implemented by @naithar: #39996.

That is cool! Unrelated to this PR really though :)

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 12, 2021
@akien-mga akien-mga merged commit fec979b into godotengine:master Mar 12, 2021
@akien-mga
Copy link
Member

Thanks!

@o01eg o01eg deleted the fix-gcc-visibility branch March 12, 2021 08:44
@o01eg
Copy link
Contributor Author

o01eg commented Mar 12, 2021

Could it be backported to 3.2?

@akien-mga
Copy link
Member

Yes, that's what the cherrypick:3.2 label means.

@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants