Long frames in frametable #797

gretay-js · 2022-08-30T19:40:23Z

Current frametable format restricts stack frame size to 16 bit for compactness. In some corner cases (e.g., owl library's autogenerated file) when compiling with -linscan, this limit can be exceeded (e.g., see #791). Usually the right way to handle this error is to restructure the code, but sometimes it is hard to do quickly and a temporary workaround is needed.
This PR proposes a way to emit long frametable enteries as an extension of the current format.
Add a compiler flag -allow-long-frames to turn on the new functionality.

xclerc

Looks good, and was indirectly tested on
a substantial part of our codebase through
#786. I am however not competent to review
the runtime part.

driver/flambda_backend_args.ml

driver/flambda_backend_flags.ml

ocaml/runtime/backtrace_nat.c

ocaml/runtime/signals_nat.c

backend/emitaux.ml

mshinwell · 2022-09-07T09:46:12Z

I think @stedolan should probably review the runtime part of this one.

stedolan

This looks good to me!

I think it needs a test, though - as far as I'm aware, there are no long frames in the compiler build, so nothing in the CI currently tests the new logic (i.e. performing a GC and taking a backtrace while there are live values in long frames on the stack).

ocaml/runtime/roots_nat.c

backend/emitaux.ml

gretay-js

I tried to add a test but it was very big. So, as @mshinwell suggested, I added a new flag -debug-long-frames-threshold n, which sets the frame size at which we switch from short to long frames. Then, I added a test, it revealed a bug [1], and I fixed it.

ocaml/runtime/roots_nat.c

backend/emitaux.ml

ocaml/runtime/caml/stack.h

stedolan

This looks good to me!

One minor suggestion: if you change the dune test to run with (diff ...) instead of (run ...), then it'll check the output and provide promotion via dune promote if the output changes.

Move checks from emit_frame to `record_frame_descr` to give a more meaningful error message with a function name and record whether the frame is long.

Check size when frame is recorded not when it is emitted. Move checks from emit_frame to `record_frame_descr` to give a more meaningful error message with a function name and record whether the frame is long. When compiled with -allow-long-frames, emit the long frames frame using 32 bit (whereas we use 16 bit constants for short frames), intead of raising user error.

instead of relying on the C compiler to insert it.

gretay-js · 2022-10-20T16:29:19Z

One minor suggestion: if you change the dune test to run with (diff ...) instead of (run ...), then it'll check the output and provide promotion via dune promote if the output changes.

done.

driver/flambda_backend_flags.ml

driver/flambda_backend_args.ml

This reverts commit 5b0dba7.

@poll

06c189a flambda-backend: Make stack allocation the default (ocaml-flambda#1013) 98debd5 flambda-backend: Initial support for value slots not of value kind (ocaml-flambda#946) deb1714 flambda-backend: Add is_last flag to closinfo words (ocaml-flambda#938) d07fce1 flambda-backend: Disable poll insertion in Configure (ocaml-flambda#967) 0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (ocaml-flambda#1012) 27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (ocaml-flambda#997) 724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (ocaml-flambda#871) 396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (ocaml-flambda#983) b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (ocaml-flambda#980) 7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (ocaml-flambda#970) 8fb75bd flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (ocaml-flambda#965) fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (ocaml-flambda#948) 9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (ocaml-flambda#943) 5f51f21 flambda-backend: Merge pull request ocaml-flambda#932 from mshinwell/4.14-upgrade 841687d flambda-backend: Run make alldepend in ocaml/ (ocaml-flambda#936) 72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff) 6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstream c86bf6e flambda-backend: Remove duplicate tests for polling 971dbeb flambda-backend: Testsuite fixes 32f8356 flambda-backend: Topeval fix for symbols patch befea01 flambda-backend: Compilation fixes / rectify merge faults a84543f flambda-backend: Merge ocaml-jst 8e65056 flambda-backend: Merge ocaml-jst 4d70045 flambda-backend: Remove filename from system frametable (amd64) (ocaml-flambda#920) 5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (ocaml-flambda#918) 6423d5e flambda-backend: Merge pull request ocaml-flambda#914 from mshinwell/merge-ocaml-jst-2022-10-24 ead605c flambda-backend: Add a missing Extract_exception (ocaml-flambda#916) c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributes cf4d0d3 flambda-backend: Merge fixes (#21) c2f742f flambda-backend: Re-enable some tests for Flambda2 (ocaml-flambda#881) 3d38d13 flambda-backend: Long frames in frametable (ocaml-flambda#797) 85aec7b flambda-backend: Add loop attribute to Builtin_attributes c0f16e3 flambda-backend: Compilation fixes 90dea23 flambda-backend: Merge flambda-backend/main 5acc6ea flambda-backend: Fixes after merge e501946 flambda-backend: Merge ocaml-jst 115083b flambda-backend: Merge ocaml-jst 9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (ocaml-flambda#893)"" (ocaml-flambda#909) ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (ocaml-flambda#902) e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (ocaml-flambda#893)" 853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (ocaml-flambda#893) 5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (ocaml-flambda#898) 7fa7f9d flambda-backend: Add missing dependencies to Dune files (ocaml-flambda#889) 3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (ocaml-flambda#896) 7565915 flambda-backend: [@poll error] attribute (ocaml-flambda#745) 9eb9448 flambda-backend: Backport the main safepoints PRs (ocaml-flambda#740) 689bdda flambda-backend: Add strict mode for ocamldep (ocaml-flambda#892) git-subtree-dir: ocaml git-subtree-split: 06c189a

@poll

06c189a flambda-backend: Make stack allocation the default (ocaml-flambda#1013) 98debd5 flambda-backend: Initial support for value slots not of value kind (ocaml-flambda#946) deb1714 flambda-backend: Add is_last flag to closinfo words (ocaml-flambda#938) d07fce1 flambda-backend: Disable poll insertion in Configure (ocaml-flambda#967) 0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (ocaml-flambda#1012) 27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (ocaml-flambda#997) 724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (ocaml-flambda#871) 396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (ocaml-flambda#983) b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (ocaml-flambda#980) 7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (ocaml-flambda#970) 8fb75bd flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (ocaml-flambda#965) fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (ocaml-flambda#948) 9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (ocaml-flambda#943) 5f51f21 flambda-backend: Merge pull request ocaml-flambda#932 from mshinwell/4.14-upgrade 841687d flambda-backend: Run make alldepend in ocaml/ (ocaml-flambda#936) 72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff) 6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstream c86bf6e flambda-backend: Remove duplicate tests for polling 971dbeb flambda-backend: Testsuite fixes 32f8356 flambda-backend: Topeval fix for symbols patch befea01 flambda-backend: Compilation fixes / rectify merge faults a84543f flambda-backend: Merge ocaml-jst 8e65056 flambda-backend: Merge ocaml-jst 4d70045 flambda-backend: Remove filename from system frametable (amd64) (ocaml-flambda#920) 5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (ocaml-flambda#918) 6423d5e flambda-backend: Merge pull request ocaml-flambda#914 from mshinwell/merge-ocaml-jst-2022-10-24 ead605c flambda-backend: Add a missing Extract_exception (ocaml-flambda#916) c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributes cf4d0d3 flambda-backend: Merge fixes (#21) c2f742f flambda-backend: Re-enable some tests for Flambda2 (ocaml-flambda#881) 3d38d13 flambda-backend: Long frames in frametable (ocaml-flambda#797) 85aec7b flambda-backend: Add loop attribute to Builtin_attributes c0f16e3 flambda-backend: Compilation fixes 90dea23 flambda-backend: Merge flambda-backend/main 5acc6ea flambda-backend: Fixes after merge e501946 flambda-backend: Merge ocaml-jst 115083b flambda-backend: Merge ocaml-jst 9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (ocaml-flambda#893)"" (ocaml-flambda#909) ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (ocaml-flambda#902) e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (ocaml-flambda#893)" 853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (ocaml-flambda#893) 5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (ocaml-flambda#898) 7fa7f9d flambda-backend: Add missing dependencies to Dune files (ocaml-flambda#889) 3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (ocaml-flambda#896) 7565915 flambda-backend: [@poll error] attribute (ocaml-flambda#745) 9eb9448 flambda-backend: Backport the main safepoints PRs (ocaml-flambda#740) 689bdda flambda-backend: Add strict mode for ocamldep (ocaml-flambda#892) git-subtree-dir: ocaml git-subtree-split: 06c189a

@poll

b11eea1 flambda-backend: Introduce Import_info (ocaml-flambda#1036) bc5b135 flambda-backend: Fix `ocamlobjinfo` on flambda2 .cmx files (ocaml-flambda#1029) c8babbd flambda-backend: Compilation_unit optimisations (ocaml-flambda#1035) e8d3e22 flambda-backend: Use 4.14.0 opam switch for building (includes upgrading ocamlformat to 0.24.1) (ocaml-flambda#1030) eb14a86 flambda-backend: Port PR81 from ocaml-jst (ocaml-flambda#1024) 131bc12 flambda-backend: Merge ocaml-jst 2022-12-13 (ocaml-flambda#1022) 06c189a flambda-backend: Make stack allocation the default (ocaml-flambda#1013) 98debd5 flambda-backend: Initial support for value slots not of value kind (ocaml-flambda#946) deb1714 flambda-backend: Add is_last flag to closinfo words (ocaml-flambda#938) d07fce1 flambda-backend: Disable poll insertion in Configure (ocaml-flambda#967) 0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (ocaml-flambda#1012) 27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (ocaml-flambda#997) 724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (ocaml-flambda#871) 396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (ocaml-flambda#983) b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (ocaml-flambda#980) 7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (ocaml-flambda#970) 8fb75bd flambda-backend: Port ocaml/ocaml#11727 and ocaml/ocaml#11732 (ocaml-flambda#965) fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (ocaml-flambda#948) 9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (ocaml-flambda#943) 5f51f21 flambda-backend: Merge pull request ocaml-flambda#932 from mshinwell/4.14-upgrade 841687d flambda-backend: Run make alldepend in ocaml/ (ocaml-flambda#936) 72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff) 6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstream c86bf6e flambda-backend: Remove duplicate tests for polling 971dbeb flambda-backend: Testsuite fixes 32f8356 flambda-backend: Topeval fix for symbols patch befea01 flambda-backend: Compilation fixes / rectify merge faults a84543f flambda-backend: Merge ocaml-jst 8e65056 flambda-backend: Merge ocaml-jst 4d70045 flambda-backend: Remove filename from system frametable (amd64) (ocaml-flambda#920) 5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (ocaml-flambda#918) 6423d5e flambda-backend: Merge pull request ocaml-flambda#914 from mshinwell/merge-ocaml-jst-2022-10-24 ead605c flambda-backend: Add a missing Extract_exception (ocaml-flambda#916) c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributes cf4d0d3 flambda-backend: Merge fixes (#21) c2f742f flambda-backend: Re-enable some tests for Flambda2 (ocaml-flambda#881) 3d38d13 flambda-backend: Long frames in frametable (ocaml-flambda#797) 85aec7b flambda-backend: Add loop attribute to Builtin_attributes c0f16e3 flambda-backend: Compilation fixes 90dea23 flambda-backend: Merge flambda-backend/main 5acc6ea flambda-backend: Fixes after merge e501946 flambda-backend: Merge ocaml-jst 115083b flambda-backend: Merge ocaml-jst 9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (ocaml-flambda#893)"" (ocaml-flambda#909) ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (ocaml-flambda#902) e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (ocaml-flambda#893)" 853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (ocaml-flambda#893) 5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (ocaml-flambda#898) 7fa7f9d flambda-backend: Add missing dependencies to Dune files (ocaml-flambda#889) 3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (ocaml-flambda#896) 7565915 flambda-backend: [@poll error] attribute (ocaml-flambda#745) 9eb9448 flambda-backend: Backport the main safepoints PRs (ocaml-flambda#740) 689bdda flambda-backend: Add strict mode for ocamldep (ocaml-flambda#892) git-subtree-dir: ocaml git-subtree-split: b11eea1

* Dummy C stubs * Run max_arity_locals test only when stack allocation is enabled * Fix and promote typing-local/ tests for stack allocation being disabled * Predicate some tests on having stack allocation * Forward port all SIMD runtime changes from d3c1413 * Fix globrootsprim.c test * Fix asmgen tests * Add runtime4 predicate to ocamltest and use it in runtime-naked-pointers/ tests * Add runtime_dir ocamltest variable + use it to fix output-complete-obj/ tests * git mv debugger debugger4 * Import debugger/ from upstream 5 * Debugger build system * Apply JS-specific modifications from debugger4/ -> debugger/ * Fix pr9971 test * Fix tests/regression/pr9326 * Fix tests/gc-roots * Fix tests/asmcomp/polling.c * Fixing of backtrace tests * Skip instrumented-runtime tests on 5.x (these tests are not present in upstream 5) * Fix Ctype.unify call in debugger/loadprinter.ml * Fixes for systhreads/ build and importing of build attributes from systhreads4/ * Add Condition, Mutex and Semaphore to the stdlib * Fix dynlink (including for move of Mutex to the stdlib) * Fix get_header.ml test * Add runtime5 predicate in ocamltest and use it to fix backtrace_c_exn.ml * Fix wrong position of modules line in ocaml/testsuite/tests/typing-local/regions.ml test header * Fix bug in caml_gc_counters in the 5 runtime * Remove ocaml/otherlibs/systhreads4/st_pthreads.h, merge error * Promote tests * Move caml_stat_free call in ocaml/runtime4/dynlink_nat.c * port #797 (long frames in frametable) (#2048) * Fix upstream bug with dynlink check/registration ordering, and revert lib-dynlink-private testcase * Fix caml_natdynlink_run_toplevel to actually call the registration function * Add GC test for ocamlnat * Address code review comments * Regenerate stdlib/.depend * Fix upstream build * Fix three runtime4/ occurrences * Fix for upstream systhreads4 build * Don't skip statmemprof tests on runtime4/ * Fix upstream build for debugger4/ * Use runtime_dir not RUNTIME_DIR in the testsuite scriptsd * Re-enabling (for runtime4) and fixing statmemprof tests * Fix _runtest debugger installation paths * Port bc9a60d to runtime/ (fix for caml_natdynlink_run_toplevel) * Address comments --------- Co-authored-by: Zesen Qian <[email protected]> Co-authored-by: Stephen Dolan <[email protected]>

gretay-js added backend runtime labels Aug 30, 2022

gretay-js requested review from stedolan, mshinwell and xclerc as code owners August 30, 2022 19:40

xclerc approved these changes Sep 7, 2022

View reviewed changes

driver/flambda_backend_args.ml Show resolved Hide resolved

driver/flambda_backend_flags.ml Show resolved Hide resolved

ocaml/runtime/backtrace_nat.c Show resolved Hide resolved

ocaml/runtime/signals_nat.c Show resolved Hide resolved

backend/emitaux.ml Show resolved Hide resolved

gretay-js force-pushed the allow-long-frames branch from 4eef7d7 to b55da00 Compare September 13, 2022 08:21

stedolan reviewed Sep 28, 2022

View reviewed changes

ocaml/runtime/roots_nat.c Outdated Show resolved Hide resolved

backend/emitaux.ml Outdated Show resolved Hide resolved

xclerc mentioned this pull request Oct 4, 2022

Add value<->native_pointer intrinsics #855

Merged

gretay-js force-pushed the allow-long-frames branch from b55da00 to 658810c Compare October 13, 2022 09:00

gretay-js commented Oct 13, 2022

View reviewed changes

ocaml/runtime/roots_nat.c Outdated Show resolved Hide resolved

backend/emitaux.ml Outdated Show resolved Hide resolved

gretay-js force-pushed the allow-long-frames branch from 658810c to 426098b Compare October 13, 2022 13:05

stedolan reviewed Oct 19, 2022

View reviewed changes

ocaml/runtime/caml/stack.h Outdated Show resolved Hide resolved

stedolan approved these changes Oct 19, 2022

View reviewed changes

gretay-js added 11 commits October 20, 2022 17:20

Add compiler flag -allow-long-frames

e6dc30a

Check size when frame is created not when it is emitted

968abb9

Move checks from emit_frame to `record_frame_descr` to give a more meaningful error message with a function name and record whether the frame is long.

Support large frames (runtime)

dbf5d7c

Change default [allow_long_frames] to true and add -no-long-frames

e7d5a5a

Remove CRs

c343996

Add flag -debug-long-frame-threshold and cleanup threshold vs marker

547ae02

Size field of long frames must be 4-byte aligned.

c4472cf

Add a test for long frames

07e8afb

Update test: diff output

ec43e50

Add _pad field to frame_descr_long to ensure frame_size is aligned

f607750

instead of relying on the C compiler to insert it.

gretay-js force-pushed the allow-long-frames branch from 426098b to f607750 Compare October 20, 2022 16:28

mshinwell requested changes Oct 24, 2022

View reviewed changes

driver/flambda_backend_flags.ml Outdated Show resolved Hide resolved

driver/flambda_backend_args.ml Outdated Show resolved Hide resolved

Address review comments

51aaded

mshinwell approved these changes Oct 24, 2022

View reviewed changes

gretay-js merged commit 5b0dba7 into ocaml-flambda:main Oct 24, 2022

gretay-js added a commit to gretay-js/flambda-backend that referenced this pull request Nov 23, 2022

Revert "Long frames in frametable (ocaml-flambda#797)"

0f75cec

This reverts commit 5b0dba7.

gretay-js mentioned this pull request Jun 9, 2023

Regalloc: optimize stack slots #1464

Merged

riaqn added a commit to riaqn/flambda-backend that referenced this pull request Nov 20, 2023

port ocaml-flambda#797

44711f1

riaqn mentioned this pull request Nov 20, 2023

port #797 (long frames in frametable) #2048

Merged

riaqn added a commit that referenced this pull request Nov 20, 2023

port #797 (long frames in frametable) (#2048)

8b2867b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Long frames in frametable #797

Long frames in frametable #797

gretay-js commented Aug 30, 2022

xclerc left a comment

mshinwell commented Sep 7, 2022

stedolan left a comment

gretay-js left a comment •

edited

Loading

stedolan left a comment

gretay-js commented Oct 20, 2022

Long frames in frametable #797

Long frames in frametable #797

Conversation

gretay-js commented Aug 30, 2022

xclerc left a comment

Choose a reason for hiding this comment

mshinwell commented Sep 7, 2022

stedolan left a comment

Choose a reason for hiding this comment

gretay-js left a comment • edited Loading

Choose a reason for hiding this comment

stedolan left a comment

Choose a reason for hiding this comment

gretay-js commented Oct 20, 2022

gretay-js left a comment •

edited

Loading