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

Preserve BASS FX and BASSmix library symbols from getting stripped in iOS #4890

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Nov 12, 2021

Normally, any native library wrapper class under iOS would have to import from __Internal in order to p/invoke the methods successfully.

But ManagedBass played it differently on the BASS addons (BASS FX, BASSmix, etc.) starting from 3.0.0, by importing from the native library name as-is, but instead using dllmaps to map the native library names to __Internal to be able to p/invoke on iOS (see ManagedBass/ManagedBass#30).

And that appears to be working (minus one worked around issue), but not quite with mtouch's strip symbols feature... (which is enabled on Release configuration)

It appears that mtouch isn't aware of the dllmap configuration and thinks that the BASS FX and BASSmix symbols aren't used anywhere (while they're, but imported from bass_fx/bassmix with dllmap rather than __Internal directly), and therefore strips the symbols out and causing EntryPointNotFoundEexception when attempting to call any of the library methods.


The only workaround to this as far as I've seen from trying out several other ways is to tell mtouch to specifically preserve each symbol of the library, and I've done that via this quick script I've wrote, which takes defined and external symbols from the library, and prepend the --nosymbolstrip= flag to each.

Other ways like using the Preserve attribute against ManagedBass.Fx.BassFx can't work, because BassFx imports the methods from bass_fx, while mtouch would be looking for imports from __Internal to actually preserve the symbols.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 12, 2021
bdach
bdach previously requested changes Nov 12, 2021
And therefore mtouch would consider the libraries symbols as unused and strip them away.

In order to work around it, specify each symbol of each library with the "nosymbolstrip={symbol}" flag to preserve it from getting stripped.
Use the following script to generate the "nosymbolstrip" flags: https://gist.github.com/frenzibyte/a09112e31cb28bc8becfa76d6a2bd23a -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

this script should not be in a random gist, it should be in this repository.

extra points if you can make it update this props file automatically in the correct manner.

Copy link
Member Author

@frenzibyte frenzibyte Nov 12, 2021

Choose a reason for hiding this comment

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

Well there exists a linked gist in the framework already, but sure can turn it to a framework wiki page if that's fine?

As for updating the props file automatically, sounds feasible, will look at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not a wiki page. an executable script in source. that you can run without copying it over from wiki or a gist or wherever.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, will look at that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added script in source with automatic project updating support. But the generated symbols will still have to be copied across to osu! and templates, unfortunately. As they can't access the native libraries to extract the symbols from, unless it's fine to attempt finding the osu-framework folder, which I'm not really sure about doing.

I'm planning on reporting the original issue to Xamarin anyways, as I'm hoping we wouldn't require this long term.

Copy link
Member

Choose a reason for hiding this comment

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

I'm planning on reporting the original issue to Xamarin anyways, as I'm hoping we wouldn't require this long term.

About that (check upstream report). Safe to say this workaround is sticking with us until NET6.

osu.Framework.iOS/Properties/AssemblyInfo.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Nov 14, 2021

Needs conflict resolution.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 14, 2021
@bdach
Copy link
Collaborator

bdach commented Nov 14, 2021

I can't verify the script because linux nm seems to be incompatible with macOS shared objects, so going to dismiss review and leave it to be verified by anyone else that has the hard/software to do so.

(Albeit I will say that a few things look weird - why no #! preprocessor directive? what's with the builtin near cd? but those may well be some macOS quirks.)

@bdach bdach dismissed their stale review November 14, 2021 13:21

see above

@frenzibyte
Copy link
Member Author

frenzibyte commented Nov 14, 2021

I can't verify the script because linux nm seems to be incompatible with macOS shared objects, so going to dismiss review and leave it to be verified by anyone else that has the hard/software to do so.

(Albeit I will say that a few things look weird - why no #! preprocessor directive? what's with the builtin near cd? but those may well be some macOS quirks.)

Just to clarify things out, I'm not really competent with sh/bash scripting, but felt this is enough to be correct (or so have I thought).

Regarding the #! preprocessor, to be frank I've looked at the build.sh script in osu!framework and noticed it didn't have that included, so thought it shouldn't be included in this script as well.

As for the builtin cd, there is actually no point in having that now that I checked what builtin is actually for, will remove that.

@frenzibyte
Copy link
Member Author

Reported upstream at xamarin/xamarin-macios#13359.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Looks to work as expected in terms of generation. I haven't tested this actually runs on a device but it looks like it should.

@peppy peppy enabled auto-merge November 16, 2021 05:11
@peppy peppy merged commit 05920eb into ppy:master Nov 16, 2021
@frenzibyte frenzibyte deleted the preserve-bass-addons-symbols branch November 16, 2021 07:09
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.

3 participants