Fix mantis PR 7168 by creating a safety margin in the bytecode stack.#510
Fix mantis PR 7168 by creating a safety margin in the bytecode stack.#510jhjourdan wants to merge 1 commit intoocaml:4.03from
Conversation
|
Minor nitpick: I'm not sure the constant should be defined in |
|
Indeed, it will not change with the underlying system. But nor does stack_threshold, and it may change if someone implements an instructions necessitating more space on the stack. |
|
@xavierleroy Could you review this patch? |
|
|
||
| let _ = | ||
| try f 1 | ||
| with Stack_overflow -> Printf.printf "OK\n" |
There was a problem hiding this comment.
Can we please ensure that this test runs only in bytecode? On some platforms, strange things can happen when ocamlopt-compiled code overflows the stack.
|
Looks good to me, provided the test is run in bytecode mode only. |
|
Fixed according to @xavierleroy 's remarks and merged into 4.03 (commit eda1b39) and trunk (commit 1915e20). |
|
Thanks, @damiendoligez ! |
…t-principality-and-gadts Update a testcase in principality-and-gadts.ml to reflect changes in ocaml#9584
|
@jhjourdan: this PR is attracting new attention because of #11062. I have one question: why 60 for the safety margin ? My analysis (below) suggests 6. APPLY[123] +3 |
|
@xavierleroy : I don't have clear memories, but it really seems like it was based on nothing concrete. It just felt good to use a margin 10 times larger than what was empirically needed, since it was not clear to me which part of the interpreter could allocate out-of-the-limit stack slots. |
I use a safety margin of 60 words, which is enough for the event handler and the apply instructions.