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

Systhreads may be not initialized with multi-threaded callbacks #7459

Closed
vicuna opened this issue Jan 13, 2017 · 6 comments
Closed

Systhreads may be not initialized with multi-threaded callbacks #7459

vicuna opened this issue Jan 13, 2017 · 6 comments

Comments

@vicuna
Copy link

vicuna commented Jan 13, 2017

Original bug ID: 7459
Reporter: @kayceesrk
Status: resolved (set by @xavierleroy on 2017-02-19T16:51:51Z)
Resolution: fixed
Priority: normal
Severity: minor
OS: Ubuntu
OS Version: 16.04
Version: 4.04.0
Target version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Fixed in version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: otherlibs
Monitored by: @yallop

Bug description

The systhreads scheduler is initialized (by caml_thread_initialize) when linking against the threads library and the OCaml code refers to the thread module. The initialization ensures that acquiring the runtime acquires the runtime master lock. If the OCaml code does not refer to the Thread module, the initialization code is never run, and the master lock is not initialize.

A possible fix is to expose caml_thread_initialize and require any multi-threaded C program calling into OCaml call this function after calling caml_main. Initialization is already protected against duplicate initializations. Other C threads unknown to the runtime still invoke caml_c_thread_register.

Steps to reproduce

The test case has 2 pthreads calling into OCaml code. Since the access is not protected by the runtime lock, errors are seen.

Untar the attachment and run make. Run ./app.native. You should see:

....
[0x7f610fe8a700] expected=10 seen=0
[0x7f610fe8a700] expected=10 seen=0
[0x7f610fe8a700] expected=10 seen=0
[0x7f6111143700] expected=10 seen=1
[0x7f610fe8a700] expected=10 seen=1
[0x7f610fe8a700] expected=10 seen=1
[0x7f610f689700] expected=10 seen=0
[0x7f6111143700] expected=10 seen=1
[0x7f610fe8a700] expected=10 seen=0
[0x7f610fe8a700] expected=10 seen=1
[0x7f6111143700] expected=10 seen=0
[0x7f610fe8a700] expected=10 seen=1
[0x7f610fe8a700] expected=10 seen=1
[0x7f6111143700] expected=10 seen=1
....

which indicates errors. This can be "fixed" by uncommenting the first line in caml_stubs.ml:

let _ = Threads.self

if you recompile and run the program again, none of the errors are seen.

File attachments

@vicuna
Copy link
Author

vicuna commented Jan 13, 2017

Comment author: @xavierleroy

yes, we could expose caml_thread_initialize and let the C part of the application call it at the right time. I wonder how many other libraries (from the core OCaml distribution or third-party) have a similar problem that would have to be fixed similarly by exposing a C initialization function.

A possible alternative would be to mark some OCaml compilation units as initialization code that must always be run even if their definitions are not referenced. Basically, it's the -linkall option applied to individual compilation units.

Speaking of -linkall, in 4.03 the systhreads library is built with -linkall for Win32 but not for Unix, and in the current OCaml trunk, the bytecode version of the systhreads library is built with -linkall, but not the native-code version. This needs cleaning up. But the point remain that the versions of the systhreads library built with -linkall very probably do not exhibit the problem shown here.

@vicuna
Copy link
Author

vicuna commented Jan 13, 2017

Comment author: @kayceesrk

It seems to be the case that native-code version in trunk is also compiled with -linkall: https://github.com/ocaml/ocaml/blob/trunk/otherlibs/systhreads/Makefile#L82-L83

@vicuna
Copy link
Author

vicuna commented Jan 13, 2017

Comment author: @xavierleroy

Indeed I misread the Makefile in the current trunk. So, maybe the problem is gone with the trunk version of systhreads?

@vicuna
Copy link
Author

vicuna commented Jan 13, 2017

Comment author: @kayceesrk

Yes. I can confirm that the problem is gone with trunk.

@vicuna
Copy link
Author

vicuna commented Jan 13, 2017

Comment author: @xavierleroy

Thanks for checking. So, the problem is fixed, even though the fix leaves me slightly unsatisfied.

See also: #1009 for a modest improvement to -linkall.

@vicuna
Copy link
Author

vicuna commented Jan 19, 2017

Comment author: @kayceesrk

This issue should be closed since the problem has been fixed in trunk.

@vicuna vicuna closed this as completed Feb 19, 2017
@vicuna vicuna added this to the 4.05.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant