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

Bug in marshaling of recursive functions #5772

Closed
vicuna opened this Issue Oct 4, 2012 · 7 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Oct 4, 2012

Original bug ID: 5772
Reporter: ccpasteur
Status: closed (set by @xavierleroy on 2015-12-11T18:08:12Z)
Resolution: fixed
Priority: normal
Severity: minor
OS: MAC OS X
OS Version: 10.6.8
Version: 4.00.0
Fixed in version: 4.00.2+dev
Category: runtime system and C interface
Monitored by: meyer

Bug description

After declaring with two recursive functions (and a few other functions), marshaling fails with the error:
Fatal error: exception Invalid_argument("output_value: abstract value (outside heap)"). It does not fail if we change the order of declaration of the two recursive functions.

Steps to reproduce

Compile the file with ocamlc or ocamlopt and run it.

Additional information

The module is declared by:
module P = struct
let rec test_one q x = x > 3
and test_list q = List.for_all (test_one q) q

let g () = ()

let f q =
if test_list q then
g ()
end

Marshalling f fails. The weird thing is that it does not fail if we change the order of declarations of the recursive functions. It also does not fail if we remove the call to g in f. Marshalling test_list works.

The bug happens with bytecode and native code. It does not happen in OCaml 3.12.1 .

File attachments

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 4, 2012

Comment author: @gasche

Not-so-surprisingly, I have bisected back the bug to the following two commits:

r12248
#5318: Reverting last un-marshaler changes, to rework the control flow of mainloop and provide better solution for eliminating recursion

r12247
#5318: Non-recursive version of extern_rec and intern_rec, to allow marshaling and un-marshaling of deeper data structures

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 4, 2012

Comment author: @jhjourdan

I submitted a patch that fixes it !

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 4, 2012

Comment author: @gasche

Indeed I can confirm that Jacques-Henri's patch fixes the issue. I have uploaded ccpasteur's initial example as patch to modify the existing "lib-marshal" testsuite, that makes apparent the issue and its solution, while allowing to check that the fix didn't break any of the lib-marshal tests.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 5, 2012

Comment author: meyer

Gabriel: To be honest and at the same time precise, the commits you pointed out are not a full story;) To my satisfaction.

Jacques-Henri: Thanks for the patch. I'll review the patch once more tomorrow and apply if it's correct.

Personally, I think that your patch should be included in the current patch release. (if proven correct).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 5, 2012

Comment author: @gasche

Indeed, I did not intend to mean that the patch were bad, just that a "git bisect" showed that the error appeared precisely there. I did review the first one at the time (obviously I missed this problem), and I think they're kind of cool -- defunctionalization in the real world!

Regarding the current patch release, Damien will decide (because that would be work for him in the end), but indeed I think that would be a good idea.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 5, 2012

Comment author: @xavierleroy

Yes, Jacques-Henri's patch looks right to me. Will see what our almighty release manager thinks about inclusion in 4.01.

As to blaming, I probably share some of the guilt for missing this issue while reviewing the new marshaler implementation.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Oct 5, 2012

Comment author: @xavierleroy

Applied the patches by jacques-henri.jourdan and gasche, in 4.00 bugfix branch (commit 12991) and in trunk (commit 12992).

@vicuna vicuna closed this Dec 11, 2015

@vicuna vicuna added stdlib bug labels Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.