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

mach/iOS12 fix #232

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dkimitsa
Contributor

dkimitsa commented Aug 28, 2018

this PR fixes #231

  • thread mach port is not deallocated when thread is saved in GC_mach_threads as it cause thread not being resumed as thread port object can be changed
  • threads are now being suspended without check of suspend_count (as it can be suspended by app)
  • changed logic of resume, now is primary cycle list are threads in GC_mach_threads

investigation and tech details are in series of posts(to long read as for comment) https://dkimitsa.github.io/2018/08/19/investigating-ios12-beta-crash-vol3/

dkimitsa added some commits Aug 28, 2018

iOS12 fix, changed:
* thread mach port is not deallocated when thread is saved in GC_mach_threads as it cause thread not being resumed as thread port object can be changed
* threads are now being suspended without check of suspend_count (as it can be suspended by app)
* changed logic of resume, now is primary cycle list are threads in GC_mach_threads
if (kern_result != KERN_SUCCESS)
ABORT("thread_resume failed");
GC_log_printf("thread_resume failed %p\n", (void *)thread);

This comment has been minimized.

@ivmai

ivmai Aug 29, 2018

Owner

what are the cases when we should ignore thread_resume failure? can it happen when GC_thread_resume is called having GC_query_task_threads is false?

This comment has been minimized.

@dkimitsa

dkimitsa Aug 29, 2018

Contributor

during having stress test on this I faced the issue when it failed to resume previously successfully suspended thread. it is rare case I met it like several times a day. Had no clear understanding why it was happening under iOS12. Error code usually was "mach port invalid"
considering that current logic allows failure to suspend thread as normal case -- it might be ok to allow application survive when there is thread resume failure as well.

This comment has been minimized.

@ivmai

ivmai Aug 30, 2018

Owner

Error code usually was "mach port invalid"

KERN_INVALID_ARGUMENT error code? Any other error code?

This comment has been minimized.

@ivmai

ivmai Aug 30, 2018

Owner
  1. Have you tested the path in 2 variants: CFLAGS_EXTRA=-DGC_NO_THREADS_DISCOVERY and CFLAGS_EXTRA=-DGC_DISCOVER_TASK_THREADS.

  2. What is the value of GC_query_task_threads when you meat resume failure? Is it always false, or always true, or any?

This comment has been minimized.

@dkimitsa

dkimitsa Aug 30, 2018

Contributor

hi @ivmai
I played a little with stress test today to get exact error number but was not able to reproduce on recent ios12 beta. will check later today and come back with result.

Have you tested the path in 2 variants: CFLAGS_EXTRA=-DGC_NO_THREADS_DISCOVERY and CFLAGS_EXTRA=-DGC_DISCOVER_TASK_THREADS.

very first "fix" was to use --disable-threads-discovery at my end. It is not big surprise it worked as each managed thread in this case keeps own thread's match port (e.g. no dealloc happening)

What is the value of GC_query_task_threads when you meat resume failure? Is it always false, or always true, or any?

Sorry I have no this information as I'm stress testing only isolated stop/start world code snippets in very empty Xcode project without bdwgc itself.

@ivmai

This comment has been minimized.

Owner

ivmai commented Aug 30, 2018

Thank you. Committed with some code refactoring as 59c82a5

@ivmai ivmai closed this Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment