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

Closure marshalling in toplevel is broken since at least 4.00 #6760

Closed
vicuna opened this issue Jan 25, 2015 · 5 comments
Closed

Closure marshalling in toplevel is broken since at least 4.00 #6760

vicuna opened this issue Jan 25, 2015 · 5 comments

Comments

@vicuna
Copy link

@vicuna vicuna commented Jan 25, 2015

Original bug ID: 6760
Reporter: @whitequark
Assigned to: @whitequark
Status: closed (set by @xavierleroy on 2016-12-07T10:47:12Z)
Resolution: fixed
Priority: normal
Severity: major
Fixed in version: 4.03.0+dev / +beta1
Category: runtime system and C interface
Tags: patch
Related to: #6468
Monitored by: @gasche @hcarty

Bug description

To reproduce, do in toplevel:

Marshal.to_string (fun () -> ()) [Marshal.Closures]
Exception: Invalid_argument "output_value: abstract value (outside heap)".

File attachments

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 25, 2015

Comment author: @whitequark

@gasche: I've attached a patch.

Loading

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 25, 2015

Comment author: @gasche

I find it rather suspicious that caml_ext_table_remove would take the responsibility to caml_stat_free the data to remove. Does this really correspond to the ownership disciplines of all users of caml_ext_table_add?

Also, I'm not familiar with this part of the code, but isn't abort()ing in caml_static_release_bytecode a bit overconfident? I would be more comfortable with at least an error message.

Loading

@vicuna
Copy link
Author

@vicuna vicuna commented Jan 25, 2015

Comment author: @whitequark

It does; caml_ext_table_free performs caml_stat_free on the data, which is why I wrote _remove that way.

I don't think so; in the impossible case (mismatched reify/release) that it happens, the backtrace provides more than enough context to debug this.

Loading

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 4, 2015

Comment author: @jhjourdan

gashe : here is what I think. It seems fine to me, except for the abort(), that does not fit the discipline of the rest of the runtime. You should instead use CAMLassert.

Loading

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 8, 2015

Comment author: @gasche

Merged -- in trunk only -- with the change suggested by Jacques-Henri:

if (!cf) {
/* [cf] Not matched with a caml_reify_bytecode call; impossible. */
Assert (0);
} else {
caml_ext_table_remove(&caml_code_fragments_table, cf);
}

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant