Skip to content

Liveness changes / exceptions coming out of allocation points#852

Merged
mshinwell merged 2 commits intoocaml:trunkfrom
mshinwell:liveness_exns
Mar 7, 2017
Merged

Liveness changes / exceptions coming out of allocation points#852
mshinwell merged 2 commits intoocaml:trunkfrom
mshinwell:liveness_exns

Conversation

@mshinwell
Copy link
Contributor

@mshinwell mshinwell commented Oct 12, 2016

This patch teaches the liveness analysis pass that allocation points may raise exceptions. This is necessary so that an assertion check against the output of the forwards dataflow pass used on the debugging branch can be implemented.

I think at the moment three things may cause an exception to come out of an allocation point:

  1. An exception from a finaliser
  2. An exception from a signal handler
  3. The "out of memory" exception from the GC.

These issues were discussed on caml-devel two years ago. One conclusion was that the "out of memory" exception should be turned into a fatal error. I will submit another patch for that.

I think I am in favour of having exceptions from finalisers terminate the program. At the moment they manifest as asynchronous exceptions, which can cause unusual behaviour. Quoting an example from @alainfrisch:

  try (Hashtbl.find tbl key, true)
  with Not_found ->
     let r = f key in
     Hashtbl.add tbl key r;
     r, false

In this piece of code, there is no guarantee that if you are in the exception handler, then the key is in the table. The allocation of the pair might have triggered a GC, which triggered a finaliser (or signal handler), which threw Not_found (even though Hashtbl.find had returned a datum).

If it can be agreed that exceptions from finalisers should terminate the program, I can prepare a patch.

The final case of signal handlers, as pointed out by @xavierleroy , is more problematic---for example they are very useful for catching Ctrl+C in interactive applications. It is for this reason that I think we still need this patch so that the liveness pass doesn't produce erroneous information and then wrong code.

As an example of the trouble this causes at the moment, the program below will cause a segfault (at least without Flambda enabled):

let create () =
  let x = ref () in
  Gc.finalise (fun _ -> failwith "exn") x;
  x

let some_of_t t =
  try Some t   (* the exception from the finalizer will come out of here *)
  with Failure _ -> Some t

let () =
  let minor_size = (Gc.get ()).Gc.minor_heap_size in
  for i = 1 to 1000 do
    Gc.minor ();
    ignore (create () : unit ref);
    for i = 1 to minor_size / 2 - 1 do
      ignore (ref ())
    done;
    match some_of_t "some" with
    | Some s -> print_string s
    | None -> assert false
  done

The linearized form of that is:

camlFoo__some_of_t_1008:
  t/29[%rbx] := R/0[%rax]
  setup trap L101
  A/32[%rbx] := "caml_exn_Failure"
  A/33[%rdi] := [A/31[%rax]]
  if A/33[%rdi] !=s A/32[%rbx] goto L102
  {spilled-t/35[s0]*}
  A/34[%rax] := alloc 16
  [A/34[%rax] + -8] := 1024 (init)
  t/36[%rbx] := spilled-t/35[s0] (reload)   <-------
  [A/34[%rax]] := t/36[%rbx] (init)
  reload retaddr
  return R/0[%rax]
  L102:
  reraise R/0[%rax]
  L101:
  push trap
  ...

The line with the arrow shows a register that is claimed to be live, spilled-t/35 -- but in fact, this register is not available; it was never spilled.

@mshinwell mshinwell added this to the 4.05-or-later milestone Oct 12, 2016
@damiendoligez
Copy link
Member

damiendoligez commented Oct 17, 2016

Exceptions from finalisers should probably do the same thing as exceptions escaping from a thread.

Should they be ignored or terminate the program, that is the question.

@dbuenzli
Copy link
Contributor

dbuenzli commented Oct 17, 2016

Should they be ignored or terminate the program, that is the question.

I think there should be some kind of global exception trap to which these exception are given with a context indicating where they occured. Something like Printexc.set_uncaught_exception_handler.

@alainfrisch
Copy link
Contributor

alainfrisch commented Oct 26, 2016

I'm slightly confused about the debugging-prerequiste tag and the first paragraph of the description. This is actually about fixing an actual bug (an unsoundness in the liveness analysis which can cause segfaults), right?

Before merging, it would be interesting to assess the performance impact of the change (it it is too large and the reason to keep such "asynchronous exception" is to support Ctrl+C, one might try to find other ways to support Ctrl+C).

@alainfrisch
Copy link
Contributor

alainfrisch commented Oct 26, 2016

I think there should be some kind of global exception trap to which these exception are given with a context indicating where they occured.

Seems to be a good idea. There remains the more anecdotal question of what to do with exceptions raised by this handler -- should one just call it again in that case, at the risk of going into an infinite loop?

@mshinwell
Copy link
Contributor Author

mshinwell commented Oct 27, 2016

@alainfrisch It's needed for the debugging branch so we can have an assertion that the new dataflow pass isn't going wrong. It is an existing bug, though. I agree that trying to measure the performance impact is probably sensible.

@damiendoligez damiendoligez modified the milestone: 4.05-or-later Feb 15, 2017
@mshinwell
Copy link
Contributor Author

mshinwell commented Mar 7, 2017

At the developer's meeting I believe it was agreed that this (just the liveness change) could be merged subject to checking the performance. I ran some tests on 4.03 and it looks much of a muchness, so I'm going to merge this. We can consider the issues about finalizers etc separately if we so desire.

@mshinwell mshinwell merged commit 8801274 into ocaml:trunk Mar 7, 2017
@alainfrisch alainfrisch mentioned this pull request Jul 25, 2018
Closed
@vicuna vicuna mentioned this pull request Mar 14, 2019
Closed
stedolan added a commit to stedolan/ocaml that referenced this pull request Oct 25, 2022
@stedolan
Add a primitive to enable/disable the tick thread (ocaml#852)
6bedea6
sadiqj pushed a commit to sadiqj/ocaml that referenced this pull request Feb 21, 2023
@stedolan
flambda-backend: Add a primitive to enable/disable the tick thread (o…
6a96b61 
…caml#852)
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 21, 2023
@mshinwell
Squashed 'ocaml/' changes from 2118848..25188da
db559ae 
25188da flambda-backend: Missed comment from PR802 (ocaml#887)
9469765 flambda-backend: Improve the semantics of asynchronous exceptions (new simpler version) (ocaml#802)
d9e4dd0 flambda-backend: Fix `make runtest` on NixOS (ocaml#874)
4bbde7a flambda-backend: Simpler symbols (ocaml#753)
ef37262 flambda-backend: Add opaqueness to Obj.magic under Flambda 2 (ocaml#862)
a9616e9 flambda-backend: Add build system hooks for ocaml-jst (ocaml#869)
045ef67 flambda-backend: Allow the compiler to build with stock Dune (ocaml#868)
3cac5be flambda-backend: Simplify Makefile logic for natdynlinkops (ocaml#866)
c5b12bf flambda-backend: Remove unnecessary install lines (ocaml#860)
ff12bbe flambda-backend: Fix unused variable warning in st_stubs.c (ocaml#861)
c84976c flambda-backend: Static check for noalloc: attributes (ocaml#825)
ca56052 flambda-backend: Build system refactoring for ocaml-jst (ocaml#857)
39eb7f9 flambda-backend: Remove integer comparison involving nonconstant polymorphic variants (ocaml#854)
c102688 flambda-backend: Fix soundness bug by using currying information from typing (ocaml#850)
6a96b61 flambda-backend: Add a primitive to enable/disable the tick thread (ocaml#852)
f64370b flambda-backend: Make Obj.dup use a new primitive, %obj_dup (ocaml#843)
9b78eb2 flambda-backend: Add test for ocaml#820 (include functor soundness bug) (ocaml#841)
8f24346 flambda-backend: Add `-dtimings-precision` flag (ocaml#833)
65c2f22 flambda-backend: Add test for ocaml#829 (ocaml#831)
7b27a49 flambda-backend: Follow-up PR#829 (comballoc fixes for locals) (ocaml#830)
ad7ec10 flambda-backend: Use a custom condition variable implementation (ocaml#787)
3ee650c flambda-backend: Fix soundness bug in include functor (ocaml#820)
2f57378 flambda-backend: Static check noalloc (ocaml#778)
aaad625 flambda-backend: Emit begin/end region only when stack allocation is enabled (ocaml#812)
17c7173 flambda-backend: Fix .cmt for included signatures (ocaml#803)
e119669 flambda-backend: Increase delays in tests/lib-threads/beat.ml (ocaml#800)
ccc356d flambda-backend: Prevent dynamic loading of the same .cmxs twice in private mode, etc. (ocaml#784)
14eb572 flambda-backend: Make local extension point equivalent to local_ expression (ocaml#790)
487d11b flambda-backend: Fix tast_iterator and tast_mapper for include functor. (ocaml#795)
a50a818 flambda-backend: Reduce closure allocation in List (ocaml#792)
96c9c60 flambda-backend: Merge ocaml-jst
a775b88 flambda-backend: Fix ocaml/otherlibs/unix 32-bit build (ocaml#767)
f7c2679 flambda-backend: Create object files internally to avoid invoking GAS (ocaml#757)
c7a46bb flambda-backend: Bugfix for Cmmgen.expr_size with locals (ocaml#756)
b337cb6 flambda-backend: Fix build_upstream for PR749 (ocaml#750)
8e7e81c flambda-backend: Differentiate is_int primitive between generic and variant-only versions (ocaml#749)

git-subtree-dir: ocaml
git-subtree-split: 25188da
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
@cuihtlauac
Ood: Inline Proficiency Module (ocaml#852)
468e124 
* Only used in Problem
* Delete host file, now empty
* Formatting

Co-authored-by: Cuihtlauac ALVARADO <[email protected]>
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

debugging-prerequisite no-change-entry-needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mshinwell @damiendoligez @dbuenzli @alainfrisch