Skip to content

Memprof: fatal error if thread is stopped from a callback.#9458

Merged
stedolan merged 3 commits intoocaml:trunkfrom
jhjourdan:memprof_fatal_error_thread_exit
May 20, 2020
Merged

Memprof: fatal error if thread is stopped from a callback.#9458
stedolan merged 3 commits intoocaml:trunkfrom
jhjourdan:memprof_fatal_error_thread_exit

Conversation

@jhjourdan
Copy link
Copy Markdown
Contributor

@jhjourdan jhjourdan commented Apr 17, 2020

This is specified as undefined behavior in gc.mli.

These are the two first commits of #9449. They do not deserve a Changes entry.

I don't think we need a test for this either.

I addressed @gadmm's concern:

This can also be set in caml_fatal_uncaught_exception, in that case the error message would be misleading.

The use of suspended in caml_fatal_uncaught_exception is replaced by a proper mask at https://github.com/ocaml/ocaml/pull/8961/files#diff-7daa060567fae8578a3e27866a93d610R143 so the problem fixes itself eventually, but not immediately. Do you have an idea of something that could be done in the meantime?

Two different flags are now used for that purpose.

I would like this to be merged in 4.11. But this can be considered as a bugfix, since it protects against potential segfaults if Thread.exit is called from a callback.

@jhjourdan jhjourdan mentioned this pull request Apr 17, 2020
Closed
gadmm
gadmm reviewed Apr 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@gadmm gadmm left a comment

Choose a reason for hiding this comment

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

Thanks for the new flag. Continuing the discussion about fatal_error below. Also, would it be better to move the doc about Thread.exit from gc.mli to the Thread module, and update it accordingly?

gadmm
gadmm approved these changes Apr 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@gadmm gadmm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The question of whether to raise or do a fatal_error is non-blocking, and can be revisited when the bug at #9267 finds a solution.

@jhjourdan jhjourdan force-pushed the memprof_fatal_error_thread_exit branch from c966b58 to fcc4172 Compare April 23, 2020 17:56
@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented Apr 24, 2020

CI is failing, but there does not seem to be n error message, and I cannot reproduce the issue on my machine.

@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented Apr 24, 2020

Ah, I found the issue: a new global variable was neither static nor prefixed by ocaml. This is now fixed.

@jhjourdan jhjourdan force-pushed the memprof_fatal_error_thread_exit branch from fcc4172 to c2fb368 Compare April 24, 2020 11:44
@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented Apr 28, 2020

Could someone review this PR? It is simple, and I would like it to be cherry-picked to 4.11, since it fixes a potential segfault!

@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented Apr 28, 2020

Of course, the usual suspects are @gasche , @stedolan and @xavierleroy . Thanks to them!

@stedolan
Copy link
Copy Markdown
Contributor

stedolan commented Apr 28, 2020

The code looks good to me, but if it's important enough to fix then it should have a testcase! Is there anything tricky about writing one?

jhjourdan added 3 commits May 11, 2020 15:55
@jhjourdan
Memprof: fatal error if thread is stopped from a callback.
97eb98d 
This is specified as undefined behavior in gc.mli.

We now use dedicated functions for the interraction between Memprof
and systhreads.
@jhjourdan
Changes
aa64e70
@jhjourdan
Add test statmemprof/thread_exit_in_callback.
629fae6
@jhjourdan jhjourdan force-pushed the memprof_fatal_error_thread_exit branch from c2fb368 to 629fae6 Compare May 11, 2020 15:12
@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented May 11, 2020 •
edited
Loading

I finally wrote a test. This was not completely trivial since ocamltest does not support checking for fatal errors.

@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented May 18, 2020

@stedolan : I (finally) wrote a test case. Do you think this is ready to merge?

@stedolan stedolan merged commit ff6ae43 into ocaml:trunk May 20, 2020
@stedolan
Copy link
Copy Markdown
Contributor

stedolan commented May 20, 2020

@stedolan : I (finally) wrote a test case. Do you think this is ready to merge?

Yes! Thanks.

@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented May 20, 2020

Thanks, @stedolan! Could you please cherry-pick on 4.11?

@jhjourdan
Copy link
Copy Markdown
Contributor Author

jhjourdan commented Jun 8, 2020

@stedolan : Up !

@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 8, 2020

I will do the cherry picking.

gasche pushed a commit that referenced this pull request Jun 8, 2020
@stedolan @gasche
Merge pull request #9458 from jhjourdan/memprof_fatal_error_thread_exit
7eb55d6 
Memprof: fatal error if thread is stopped from a callback.
(cherry picked from commit ff6ae43)
@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 8, 2020

Done: 7eb55d6.

@stedolan
Copy link
Copy Markdown
Contributor

stedolan commented Jun 8, 2020

Missed this! Thanks @gasche

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

Reviewers

1 more reviewer

@gadmm gadmm gadmm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jhjourdan @stedolan @gasche @gadmm