Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An unexpected behavior in toplevel: combination of consecutive let bindings and exceptions #6211

Closed
vicuna opened this issue Oct 25, 2013 · 6 comments

Comments

@vicuna
Copy link

vicuna commented Oct 25, 2013

Original bug ID: 6211
Reporter: Hiroaki Yamanouchi
Status: closed (set by @xavierleroy on 2015-12-11T18:28:01Z)
Resolution: fixed
Priority: high
Severity: crash
Version: 3.11.2
Target version: 4.02.0+dev
Fixed in version: 4.02.0+dev
Category: back end (clambda to assembly)
Monitored by: @gasche @hcarty

Bug description

An unexpected behavior in consecutive let bindings at toplevel, say, “let x1 = e1 let x2 = e2;;”. If e2 raises an exception, the result of “x;;” afterwards depends on whether x1 had been already bound before the evaluation of the first phrase.

----- Here is a case when a variable is not bound. -----

(* x is not bound *)

let x = 2 let y = 1 / 0;;

Exception: Division_by_zero.

x;;

Error: Unbound value x

----- Here is a case when a variable is bound. -----

let x = 1;;

val x : int = 1

(* now, x is bound to 1 *)

let x = 3 let y = 1 / 0;;

Exception: Division_by_zero.

x;;

  • : int = 3 (* I suppose that the result should be 1 *)

Steps to reproduce

At toplevel, input the following codes.

  1. "let x = 1;;"
  2. "let x = 3 let y = 1 / 0;;"
  3. "x;;"

File attachments

@vicuna
Copy link
Author

vicuna commented Oct 25, 2013

Comment author: @garrigue

This is a "serious" bug:

let x = "hello";;

val x : string = "hello"

let x = 2 let y = 1/0;;

Exception: Division_by_zero.

x;;

Segmentation fault

@vicuna
Copy link
Author

vicuna commented Oct 25, 2013

Comment author: @garrigue

I see two ways to fix this:

  1. generalize the approach taken for modules of using a unique name for toplevel values

or

  1. remember old values and backtrack in case of failure

Personally I would prefer (1), as it is looks like the "right" thing.
However, IIRC correctly, the goal of the current approach of using just a string is to avoid memory leaks.
We would probably have to do something about that too.
WIth (2), we get this for free.

@vicuna
Copy link
Author

vicuna commented Apr 23, 2014

Comment author: @alainfrisch

Couldn't we typecheck+run each structure item separately (and fail on the first error)? As in:

Index: toplevel/toploop.ml
===================================================================
--- toplevel/toploop.ml	(revision 14658)
+++ toplevel/toploop.ml	(working copy)
@@ -236,8 +236,11 @@
 
 (* Execute a toplevel phrase *)
 
-let execute_phrase print_outcome ppf phr =
+let rec execute_phrase print_outcome ppf phr =
   match phr with
+  | Ptop_def (x :: (_ :: _ as tl)) ->
+      execute_phrase print_outcome ppf (Ptop_def [x]) &&
+      execute_phrase print_outcome ppf (Ptop_def tl)
   | Ptop_def sstr ->
       let oldenv = !toplevel_env in
       Typecore.reset_delayed_checks ();

@vicuna
Copy link
Author

vicuna commented Jun 6, 2014

Comment author: @xavierleroy

A possible fix is attached to this PR. The hashtable that holds values for toplevel-bound names is replaced by a reference to a persistent map, so that it is trivial to snapshot the map before every execution of a phrase, then roll back to the snapshot when an exception escapes. Review & comments welcome.

@vicuna
Copy link
Author

vicuna commented Jun 8, 2014

Comment author: @garrigue

Review & comments welcome.

This looks compact and to the point.
And I prefer it to the incremental option, as toplevel inputs
only stop at ";;".

While this is not mentioned in the PR, this is a soundness bug,
hence the high priority:

let x = "hello";;

val x : string = "hello"

let x = 1 let y = raise Exit;;

Exception: Pervasives.Exit.

x;;

Segmentation fault

@vicuna
Copy link
Author

vicuna commented Jun 9, 2014

Comment author: @xavierleroy

Fixed as proposed in 4.02 (r14976) and on trunk (r14977).

@vicuna vicuna closed this as completed Dec 11, 2015
@vicuna vicuna added this to the 4.02.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant