Skip to content

Skip pthread_setcancelstate on android#52

Merged
tov merged 1 commit intojanestreet:masterfrom
purarue:pthread-cancel-android
Sep 19, 2023
Merged

Skip pthread_setcancelstate on android#52
tov merged 1 commit intojanestreet:masterfrom
purarue:pthread-cancel-android

Conversation

@purarue
Copy link
Copy Markdown
Contributor

@purarue purarue commented Sep 19, 2023

This is a follow up from: ocaml/dune#8676

Android doesn't support cancelling threads, so theres no reason to set the cancel state

For more info: https://android.googlesource.com/platform/bionic.git/+/HEAD/docs/status.md

Thread cancellation (pthread_cancel). Unlikely to ever be implemented because of the difficulty and cost of implementing it, and the difficulty of using it correctly. See This is why we can't have safe cancellation points for more about thread cancellation.

@purarue purarue force-pushed the pthread-cancel-android branch from 4a5b3b1 to e196d16 Compare September 19, 2023 00:33
@purarue purarue marked this pull request as ready for review September 19, 2023 00:33
@purarue purarue force-pushed the pthread-cancel-android branch from e196d16 to 653e70d Compare September 19, 2023 00:34
@purarue
Skip pthread_setcancelstate on android
049f40d 
Android doesn't support cancelling threads, so theres
no reason to set the cancel state

Signed-off-by: Sean Breckenridge <seanbrecke@gmail.com>
@purarue purarue force-pushed the pthread-cancel-android branch from 653e70d to 049f40d Compare September 19, 2023 00:37
@purarue
Copy link
Copy Markdown
Contributor Author

purarue commented Sep 19, 2023 •
edited
Loading

Not sure exactly how to build this to test on termux (as it requires ppx_expect, which requires dune, which I cant install on android due to an unrelated issue), but while trying to build dune (which vendors this library) on termux, I get the following:

./.duneboot.exe
Done: 145/1347 (jobs: 8)cd _boot && /data/data/com.termux/files/home/.opam/5.0.0/bin/ocamlopt.opt -c -g -I +unix -I +threads spawn_stubs.c
vendor/spawn/src/spawn_stubs.c:706:3: error: call to undeclared function 'pthread_setcancelstate'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state);
  ^
vendor/spawn/src/spawn_stubs.c:706:26: error: use of undeclared identifier 'PTHREAD_CANCEL_DISABLE'
  pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cancel_state);
                         ^

Which was then fixed by the diff here, allowing me to successfully compile the vendored version of this lib there

Tried compiling the file manually by doing something like:

$ ocamlc -verbose -I ~/.opam/5.1.0/lib/ocaml/caml/ src/spawn_stubs.c 
+ gcc -O2 -fno-strict-aliasing -fwrapv -pthread -fPIC   -D_FILE_OFFSET_BITS=64    -c   '-I/data/data/com.termux/files/home/.opam/5.1.0/lib/ocaml/caml/' -I'/data/data/com.termux/files/home/.opam/5.1.0/lib/ocaml' 'src/spawn_stubs.c'
src/spawn_stubs.c:492:20: error: call to undeclared function 'caml_convert_signal_number'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      int signal = caml_convert_signal_number(Long_val(Field(v_signals_list, 0)));
                   ^
1 error generated.

But can't seem to find the caml_convert_signal_number function. I do see the declaration in the header file in ~/.opam/5.1.0/lib/ocaml/caml/signals.h but don't know what to do past that, do not have enough experience debugging linking errors 😅

@rgrinberg
Copy link
Copy Markdown
Collaborator

rgrinberg commented Sep 19, 2023

@seanbreckenridge the last problem is fixed by: #48

@tov tov merged commit cfa1f30 into janestreet:master Sep 19, 2023
@purarue purarue deleted the pthread-cancel-android branch September 19, 2023 09:15
@purarue purarue mentioned this pull request Sep 21, 2023
Merged
@emillon emillon mentioned this pull request Sep 26, 2023
Merged
emillon added a commit to emillon/dune that referenced this pull request Sep 26, 2023
@emillon
chore: update spawn
b1597e2 
- pull some upstream changes including janestreet/spawn#51 (Haiku) and
  janestreet/spawn#52 (Android)
- point to `ocaml-dune/spawn`
- point to a commit instead of a branch name

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to ocaml/dune that referenced this pull request Sep 26, 2023
@emillon
chore: update spawn (#8768)
ebbfffb 
- pull some upstream changes including janestreet/spawn#51 (Haiku) and
  janestreet/spawn#52 (Android)
- point to `ocaml-dune/spawn`
- point to a commit instead of a branch name

Signed-off-by: Etienne Millon <me@emillon.org>
dkalinichenko-js pushed a commit to dkalinichenko-js/opam-repository that referenced this pull request Nov 12, 2024
@d-kalinichenko
[new release] spawn (0.17.0)
f4a69e4 
CHANGES:

- Support older GCC like 4.8.5 (janestreet/spawn#59)

- Fix spawning processes on Windows when environment contains non-ascii
  characters (janestreet/spawn#58)

- Skip calls to pthread_cancelstate on android, as its not available (janestreet/spawn#52)

- Fix compatibility with systems that do not define `PIPE_BUF`. Use
  `_POSIX_PIPE_BUF` as a fallback. (janestreet/spawn#49)

- [haiku] Fix compilation on Haiku OS. The header sys/syscalls.h isn't
  available, neither is pipe2()

- Allow setting the sigprocmask for spawned processes (janestreet/spawn#32)
@dkalinichenko-js dkalinichenko-js mentioned this pull request Nov 12, 2024
Closed
dkalinichenko-js pushed a commit to dkalinichenko-js/opam-repository that referenced this pull request Nov 12, 2024
@d-kalinichenko
[new release] spawn (v0.17.0)
6eb7062 
CHANGES:

- Support older GCC like 4.8.5 (janestreet/spawn#59)

- Fix spawning processes on Windows when environment contains non-ascii
  characters (janestreet/spawn#58)

- Skip calls to pthread_cancelstate on android, as its not available (janestreet/spawn#52)

- Fix compatibility with systems that do not define `PIPE_BUF`. Use
  `_POSIX_PIPE_BUF` as a fallback. (janestreet/spawn#49)

- [haiku] Fix compilation on Haiku OS. The header sys/syscalls.h isn't
  available, neither is pipe2()

- Allow setting the sigprocmask for spawned processes (janestreet/spawn#32)
@dkalinichenko-js dkalinichenko-js mentioned this pull request Nov 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@purarue @rgrinberg @tov