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

Double check rebase through ocaml/ocaml#10726 #811

Closed
ctk21 opened this issue Dec 22, 2021 · 1 comment
Closed

Double check rebase through ocaml/ocaml#10726 #811

ctk21 opened this issue Dec 22, 2021 · 1 comment

Comments

@ctk21
Copy link
Collaborator

ctk21 commented Dec 22, 2021

During the recent rebase ocaml-multicore bringing us up to date ocaml/ocaml's trunk, we rebased over ocaml/ocaml#10726 (Free the alternate signal stack when the main OCaml code / an OCaml thread stop).

The merge was 96b23c3. In the cold light of day I'm not sure I got this correct. I took the multicore changes; the logic was that 10726 was about stack overflow checks and ppc/s390 which aren't the same in multicore. However I notice a caml_terminate_signals function which may be wanted in the future. Mea culpa!

@sadiqj @xavierleroy: as experts in this area, you might spot something I've not done right here?

@xavierleroy
Copy link
Contributor

Multicore/5.00 does not need to catch signals corresponding to stack overflows (good riddance!), but PPC and s390x still need to catch signals corresponding to array bound checking. (There are nice "compare and trap" instructions in these architectures that implement bound checking very nicely.)

So, what you've done is correct for x86 and ARM, but when PPC and s390x are ported back, small pieces of the original code will need to be reinstated.

Meanwhile, I have other simplifications to runtime/signal* to propose once the Multicore PR is merged.

So I suggest you don't do anything before the merge. Sometimes, it's healthy to remove code and re-add it later in a simpler form...

@ctk21 ctk21 closed this as completed Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants