-
-
Notifications
You must be signed in to change notification settings - Fork 422
Conversation
|
Thanks for your pull request, @joakim-noah! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
| @@ -372,6 +372,9 @@ unittest | |||
| // by checking that locking is not possible. This assumes | |||
| // that the underlying implementation is well behaved | |||
| // and makes the object non-lockable upon destruction. | |||
| // For example, Bionic doesn't appear to do so, so this test is | |||
| // not run on Android. | |||
| version (CRuntime_Bionic) {} else | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test asserts on Android 6.0 Marshmallow, as it doesn't appear that pthread_mutex_destroy really destroys the mutex there. I notice that if I comment out the line that sets PTHREAD_MUTEX_RECURSIVE in this druntime module, the test passes again, but I haven't dug into why it only doesn't work for recursive mutexes. It doesn't, so this test can't be run as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the author of this test (and the test below) I think it might have been a mistake to rely on implementation defined behavior here (actually undefined behavior according to the Posix standard). I assumed that pthread_mutex_trylock would return EINVAL on all platforms if called with a previously destroyed mutex (otherwise what would be the point of specifying such return value in the standard?).
I would love to find a better way to verify that the internal implementation has been destroyed successfully, but I can't think of any other alternatives at the moment.
…d doesn't run its signal handler, which doesn't matter for resuming.
| // The Android VM runtime intercepts SIGUSR1 and apparently doesn't allow | ||
| // its signal handler to run, so swap the two signals on Android, since | ||
| // thread_resumeHandler does nothing. | ||
| version( Android ) thread_setGCSignals(SIGUSR2, SIGUSR1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Android VM runtime intercepts SIGUSR1 in its signal-catcher thread and apparently doesn't run its signal-handler. This doesn't matter when D executables are run as standalone processes on the Android command-line, but it causes problems when threads are suspended in an Android app that runs D code in a shared library called from the VM runtime, as that's how native code normally has to be run on Android.
When the unit tests for core.thread are run in an Android app without this change, they hang at pthread_kill when it tries to suspend a thread. My guess is this is because the VM runtime intercepts SIGUSR1 and the thread_suspendHandler is never run. If I swap the two signals, it still resumes fine, presumably because the thread_resumeHandler does nothing, so it doesn't matter if it doesn't run.
I've been using Ilya's #1565 also, which works fine. However, that pull has been stuck in limbo for more than a year, so I'd like to get this in in the meantime. Whenever that pull is approved, this can be removed.
src/rt/sections_android.d
Outdated
| auto mbeg = cast(immutable ModuleInfo**)&__start_minfo; | ||
| auto mend = cast(immutable ModuleInfo**)&__stop_minfo; | ||
| auto mbeg = cast(immutable ModuleInfo**)&__start___minfo; | ||
| auto mend = cast(immutable ModuleInfo**)&__stop___minfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where these magic linker symbols come from, a grep of the Android NDK for them turns up nothing. But these are the symbols ldc uses and the extra underscores are needed for this to actually work with the NDK's linker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may help: https://www.airs.com/blog/archives/56
We set the section name to "__minfo", so that's where the extra underscores come from: https://github.com/ldc-developers/ldc/blob/0b8d09d517ce2155496bf9e8f39ae57eff0b6ad0/gen/modules.cpp#L375
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link. I thought a grep of the linker scripts would turn them up, but that's obviously not going to work for a section name we make up.
…ment on how our emulated TLS scheme for Android works.
|
Since Johan pointed out that the different linker symbols are because ldc names its moduleinfo section differently, took out that change and will just submit it separately to ldc's druntime. |
Tested with @kinke's WIP ldc branch merge-2.074, these are the last remaining changes for a fully working druntime on Android.