Release bytecode only after backtrace has been recorded #10803

renatoalencar · 2021-12-01T14:59:13Z

When an exception happens bytecode has been released before the
backtrace is recorded and so all the debugging information related,
because of that the backtrace information for the script that has
just been compiled was not being found, which resulted on showing
'unknown location' instead of the actual location.

Enable debug flag for running a file on toplevel.

Resolves #9701

When an exception happens bytecode has been released before the backtrace is recorded and so all the debugging information related, because of that the backtrace information for the script that has just been compiled was not being found, which resulted on showing 'unknown location' instead of the actual location. Enable debug flag for running a file on toplevel

gasche

I believe that this is a correct bugfix to a regression we introduced in 4.12 -- see the discussion in #9701 (comment) .

gasche · 2021-12-01T15:55:45Z

toplevel/byte/topeval.ml

@@ -87,13 +87,16 @@ let load_lambda ppf lam =
  match
    may_trace := true;
    Fun.protect
-      ~finally:(fun () -> may_trace := false;
-                          if can_free then Meta.release_bytecode bytecode)
+      ~finally:(fun () -> may_trace := false)


I would be in favor of completely removing the Fun.protect call here, if only because it shows up in the user-visible backtrace.

Sure, removed.

renatoalencar · 2021-12-01T16:10:42Z

@gasche I would like to have a regression test for that. But couldn't figure that out from what is present on testsuite/tests/tool-toplevel/.

gasche · 2021-12-01T16:21:46Z

Re. testing: there is a flag ocaml_script_as_argument to test ocaml foo.ml, see testsuite/tests/tool-ocaml/directive_failure.ml (./ocamltest/ocamltest -e ..../mytest.ml shows how the test runs, -e logs on stderr instead of a file).

renatoalencar · 2021-12-01T16:49:43Z

I was able to add the test, but not to make sure that's right. I couldn't make it fail when the reference file is different from the actual output of the script.

gasche · 2021-12-01T17:11:47Z

testsuite/tests/tool-toplevel/pr9701.ml

+ocaml_exit_status = "2"
+* setup-ocaml-build-env
+** ocaml
+*)


You need to add a check-ocaml-output step (see for example tests/typing-shadowing-of-pervasives-submodules/redefine_largefile_top.ml).

Thank you very much

renatoalencar · 2021-12-02T10:10:10Z

Looks like that test is failing because of the backslash Windows uses for paths, toplevel/byte/topeval.ml still the same though. I'm gonna try to understand why.

> Running pr9701.ml in bytecode toplevel (expected exit status: 2)
> Commandline: C:\projects\🐫реализация-mingw32\runtime\ocamlrun.exe C:\projects\🐫реализация-mingw32\ocaml.exe -noinit -no-version -noprompt -nostdlib -I C:\projects\🐫реализация-mingw32\stdlib -I C:\projects\🐫реализация-mingw32\toplevel pr9701.ml
>   Redirecting stdout to C:\projects\🐫реализация-mingw32\testsuite\_ocamltest\tests/tool-toplevel\pr9701\ocaml\ocaml.output 
>   Redirecting stderr to C:\projects\🐫реализация-mingw32\testsuite\_ocamltest\tests/tool-toplevel\pr9701\ocaml\ocaml.output 
> ### begin stdout ###
> Exception: Failure "test".
> Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
> Called from <unknown> in file ".\pr9701.ml", line 13, characters 9-16
> Called from Topeval.load_lambda in file "toplevel/byte/topeval.ml", line 89, characters 4-14
> ### end stdout ###
> Action 1/1 (ocaml) passed
> Running test check-ocaml-output with 1 actions
> 
> Running action 1/1 (check-ocaml-output)
> Comparing toplevel output C:\projects\🐫реализация-mingw32\testsuite\_ocamltest\tests/tool-toplevel\pr9701\ocaml\ocaml.output to reference C:\projects\🐫реализация-mingw32\testsuite\tests\tool-toplevel\pr9701.compilers.reference
> Action 1/1 (check-ocaml-output) failed (toplevel output C:\projects\🐫реализация-mingw32\testsuite\_ocamltest\tests/tool-toplevel\pr9701\ocaml\ocaml.output differs from reference C:\projects\🐫реализация-mingw32\testsuite\tests\tool-toplevel\pr9701.compilers.reference: 
> --- "C:\\projects\\\360\237\220\253\321\200\320\265\320\260\320\273\320\270\320\267\320\260\321\206\320\270\321\217-mingw32\\testsuite\\tests\\tool-toplevel\\pr9701.compilers.reference"	2021-12-01 20:22:49.778687600 +0000
> +++ "C:\\projects\\\360\237\220\253\321\200\320\265\320\260\320\273\320\270\320\267\320\260\321\206\320\270\321\217-mingw32\\testsuite\\_ocamltest\\tests/tool-toplevel\\pr9701\\ocaml\\ocaml.output"	2021-12-01 20:50:30.477731600 +0000
> @@ -1,4 +1,4 @@
>  Exception: Failure "test".
>  Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
> -Called from <unknown> in file "./pr9701.ml", line 13, characters 9-16
> +Called from <unknown> in file ".\pr9701.ml", line 13, characters 9-16
>  Called from Topeval.load_lambda in file "toplevel/byte/topeval.ml", line 89, characters 4-14
> 
> )

Octachron · 2021-12-02T10:13:17Z

A simpler workaround might be to fix the filename by adding a line directive at the start of the script:

#1 "pr9701.ml"
Printexc.record_backtrace true;;

gasche · 2021-12-02T10:14:30Z

It's odd that Windows is using .\foo.ml, but toplevel/byte/topeval.ml. (cc @dra27)

But yes I would go for @Octachron's workaround, or just disable the test under Windows for now.

renatoalencar · 2021-12-02T10:53:12Z

Gonna try @Octachron's workaround first, it seems to work locally.

xavierleroy · 2022-01-30T17:17:33Z

What's the status on this PR? It looks like a bug fix, so it should be given some attention for 4.14.

xavierleroy · 2022-01-30T17:18:54Z

toplevel/toploop.ml

@@ -109,6 +109,7 @@ let load_file = load_file false
 (* Execute a script.  If [name] is "", read the script from stdin. *)

 let run_script ppf name args =
+  Clflags.debug := true;


Is this related to the bug fix or an independent change?

Without this, the toplevel wouldn't collect debug information and show no line numbers after an exception.

I still don't understand. Is this change visible to users? If so it should have been discussed as part of this PR and it should be described as such in the Changes file.

I understand this as a minor fix to an inconsistency in the toplevel code: Toploop.loop, called by interactive usage of the toplevel, sets Clflags.debug := true on entry, but (until this change) ocaml foo.ml would behave differently. With this change, both Toploop entry points from Topmain do the same thing.

That will affect the output of shebang ocaml scripts, which is a known area for surprising bug reports (cf. the still-smouldering, if closed, stuff around #2155). That sounds worth an explicit entry in Changes, therefore?

gasche · 2022-01-30T20:58:26Z

Thanks for the ping! I guess we got distracted by other stuff. I fixed the Changes conflict and will merge in 4.14 if the CI comes back green.

renatoalencar · 2022-01-30T20:59:03Z

What's the status on this PR? It looks like a bug fix, so it should be given some attention for 4.14.

It should be really to go. Although I think location for scripts could be really improved, this specific bug should be fixed.

renatoalencar added 2 commits December 1, 2021 10:10

Fix broken test

b42b9c0

gasche approved these changes Dec 1, 2021

View reviewed changes

Remove Fun.protect while protecting the stack trace

a33b7d4

renatoalencar marked this pull request as ready for review December 1, 2021 16:08

Add regression tests

ad4b640

gasche reviewed Dec 1, 2021

View reviewed changes

Fix regression test reference output

2cd1d56

Fix test

5ae16cb

Add missing entry on Changes

0178c60

xavierleroy added the bug label Jan 30, 2022

xavierleroy reviewed Jan 30, 2022

View reviewed changes

Merge branch 'trunk' into fix-premature-free

7cda6ce

gasche added the merge-me label Jan 30, 2022

gasche merged commit 6a7a7b3 into ocaml:trunk Jan 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release bytecode only after backtrace has been recorded #10803

Release bytecode only after backtrace has been recorded #10803

renatoalencar commented Dec 1, 2021 •

edited

gasche left a comment

gasche Dec 1, 2021

renatoalencar Dec 1, 2021

renatoalencar commented Dec 1, 2021

gasche commented Dec 1, 2021

renatoalencar commented Dec 1, 2021 •

edited

gasche Dec 1, 2021

renatoalencar Dec 1, 2021

renatoalencar Dec 1, 2021

renatoalencar commented Dec 2, 2021

Octachron commented Dec 2, 2021

gasche commented Dec 2, 2021

renatoalencar commented Dec 2, 2021

xavierleroy commented Jan 30, 2022

xavierleroy Jan 30, 2022

renatoalencar Jan 30, 2022

xavierleroy Jan 31, 2022

gasche Jan 31, 2022

dra27 Jan 31, 2022

gasche commented Jan 30, 2022

renatoalencar commented Jan 30, 2022

Release bytecode only after backtrace has been recorded #10803

Release bytecode only after backtrace has been recorded #10803

Conversation

renatoalencar commented Dec 1, 2021 • edited

gasche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatoalencar commented Dec 1, 2021

gasche commented Dec 1, 2021

renatoalencar commented Dec 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatoalencar commented Dec 2, 2021

Octachron commented Dec 2, 2021

gasche commented Dec 2, 2021

renatoalencar commented Dec 2, 2021

xavierleroy commented Jan 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasche commented Jan 30, 2022

renatoalencar commented Jan 30, 2022

renatoalencar commented Dec 1, 2021 •

edited

renatoalencar commented Dec 1, 2021 •

edited