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

Remove _init and _fini exports from NAOT and update android docs #100755

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Apr 8, 2024

@CasualPokePlayer found an alternative workaround to publish apps with latest NDK (LTS): #92272 (comment)

@josephmoresena, @LittleCodingFox, could you please confirm if this works with your setup (presumably with older NDK)? If not, we can keep both workarounds in the docs for now and later we can look into implementing small introspection to detect either-or cases in BuildIntegration to automate it.

Fixes #92272

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 8, 2024
@josephmoresena
Copy link
Contributor

The last NDK I used was 26 with NativeAOT-AndroidHelloJniLib was 26, I just avoid these LinkerArg elements.
In my repo you can see:
< ItemGroup Condition="$(IsAndroid) == 'true' And $(UseLibCSections) == 'true'">
< LinkerArg Include="-Wl,--defsym,_init=__libc_init" />
< LinkerArg Include="-Wl,--defsym,_fini=__libc_fini" />
< /ItemGroup>
I don't remember until which NDK version the __libc_init and __libc_fini where exported as default.

@CasualPokePlayer
Copy link

CasualPokePlayer commented Apr 9, 2024

Doing some investigation, I don't think the older workaround was appropriate at all. __libc_init has a completely different function prototype compared to _init (and __libc_init is normally just called within the _start ELF entrypoint with its expected arguments, eventually calling main).

_fini similarly has a different function prototype compared to __libc_fini (__libc_fini is just expected to be called due to it being internally registered with __cxa_atexit during the __libc_init call).

(Note however, _start won't actually be present if building as a shared library (entrypoint is just NULL, i.e. no entrypoint, no need to call __libc_init; handling other libc initialization/deinitialization is handled with other methods which aren't too relevant))

In more "standard" glibc Linux (and presumably also musl?), you'd get _init/_fini end up being implemented by parts of the libc (well, provided .o files which normally get linked in with a standard compiler setup when compiling, similar to _start). However, it just seems that this is not provided at all on bionic. Of course, user could in theory provide them and they would act the first/last ever functions called (before/after even __attribute__((constructor))/__attribute__((destructor)) implemented functions), but that's not a detail which .NET would need to really care about (well, if this is done it'd probably be in unmanaged code anyways, I doubt a C# function would work for this case).

The proper fix here is to just not put _init/_fini in the export version script at all for bionic, they don't come with bionic, no need to try to include them. It only ever appeared to work before due to clang silently ignoring them, not because they were present.

@am11
Copy link
Member Author

am11 commented Apr 9, 2024

Thanks for the further research @CasualPokePlayer, very thorough! If the signatures are mismatched, then this workaround makes more sense than the old one. I'll mark the PR ready for review.

As to the fix: @josephmoresena, since you added it the global symbols in export list (long time ago), dotnet/corert@88d7571#diff-c9020f726343209ae19211774789db0cb46bcc828f448c07ef62cb6a552b93a8R49 (current location in this repo: src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ExportsFileWriter.cs), do you see any potential harm in deleting that line? I think it should be fine as the final linkage will take care of platform/libc specific choices.

@am11 am11 marked this pull request as ready for review April 9, 2024 09:30
@jkotas
Copy link
Member

jkotas commented Apr 9, 2024

IIRC, it just would not work without that line (the global constructors/destructors would not run). If the shared libraries stil work with that line deleted, I do not see a problem with deleting it.

@am11
Copy link
Member Author

am11 commented Apr 9, 2024

@jkotas, seems like _init and _fini are not exported in case of .so, only when it is executable. Removing them from version file seems safe (init/fini are obsolete and we should be using __attribute(construction/destructor) which we are using in coreclr-pal). The published crossgen2 seems to be working after the repo build with change applied. Should I push the change in this PR?

@am11 am11 changed the title Update android-bionic.md for latest NDK LTS Remove _init and _fini exports from NAOT and update android docs Apr 9, 2024
@am11
Copy link
Member Author

am11 commented Apr 9, 2024

This can use /azp run runtime-nativeaot-outerloop checks.

@LittleCodingFox
Copy link

Thanks for the help guys, I've been busy so I can't really test this right now, but it looks like you got this!

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit f7f67bb into dotnet:main Apr 10, 2024
109 of 112 checks passed
@am11 am11 deleted the patch-31 branch April 10, 2024 07:17
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member os-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 8 RC1: NativeAOT fails to build for linux-bionic-arm64 when specifying project as library (Linker error)
6 participants