Skip to content

Simplifications and fixes to multicore systhreads implementation (0.5/3)#11390

Merged
gasche merged 3 commits intoocaml:trunkfrom
gadmm:systhreads_simpl_and_fixes_1b
Jul 4, 2022
Merged

Simplifications and fixes to multicore systhreads implementation (0.5/3)#11390
gasche merged 3 commits intoocaml:trunkfrom
gadmm:systhreads_simpl_and_fixes_1b

Conversation

@gadmm
Copy link
Copy Markdown
Contributor

@gadmm gadmm commented Jul 2, 2022

This contains commits from #11271 except the one that fails the test suite on Alpine.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 3, 2022

Precheck running at https://ci.inria.fr/ocaml/job/precheck/729/

@xavierleroy
Copy link
Copy Markdown
Contributor

Precheck is OK. One unrelated failure with Omnios (will check separately) but Alpine, PPC32 and PPC64 are back.


CAMLextern void caml_reset_domain_lock(void);
CAMLextern int caml_bt_is_in_blocking_section(void);
CAMLexport int caml_bt_is_self(void);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes some of the Windows failures, but not all:

Suggested change
CAMLexport int caml_bt_is_self(void);
CAMLextern int caml_bt_is_self(void);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, to expand - the Windows workers in Jenkins are running binutils 2.34, so don't surface the bug which this fixes. The other failure I'm seeing locally is in the tests/output-complete-obj/main.ml, but that doesn't appear to be this PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(indeed - the failure with tests/output-complete-obj/main.ml seems to be caused by binutils 2.38, which I updated to two days ago )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. This is fixed now.

gadmm added 3 commits July 3, 2022 12:13
This behaviour was likely unintended given that caml_thread_yield runs
signal handlers, which can execute arbitrary OCaml code.
@gadmm
Copy link
Copy Markdown
Contributor Author

gadmm commented Jul 3, 2022

I have arranged that no further changes are needed with this PR, so I propose to go ahead.

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve based on:

  • KC's and Engil's review of the previous PR #11271
  • the fact thaI the INRIA CI is now happy

This will be trunk-only for now: let's wait to see where we are -- and to understand the cause of the bug -- before pushing things in 5.0.

@gasche gasche merged commit 1ef52ed into ocaml:trunk Jul 4, 2022
gasche added a commit that referenced this pull request Jul 19, 2022
Fix some leaks in systhreads

(cherry picked from commit b8dbb53)

The cherry-pick required solving a conflict coming from
  159f3f3
(PR #11390) which is not (yet?) merged in 5.0.
gasche added a commit that referenced this pull request Jul 21, 2022
Simplifications and fixes to multicore systhreads implementation (0.5/3)

(cherry picked from commit 1ef52ed)
@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 21, 2022

Backported to 5.0 as 42bf679.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants