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

`Weak.get_copy` doesn't copy custom block (fix MPR#7279) #710

Merged
merged 3 commits into from Feb 23, 2017

Conversation

Projects
None yet
6 participants
@bobot
Contributor

bobot commented Jul 22, 2016

Weak.get_copy duplicates values which could break abstraction. It is really a problem in the case of custom block with C-finalizer. This PR proposes to not copy the value in this case. fix MPR#7279

a := create float64 c_layout 10;
Gc.compact ();
let c = create float64 c_layout 10 in

This comment has been minimized.

@braibant

braibant Jul 23, 2016

Contributor

Is c needed for this test?

@braibant

braibant Jul 23, 2016

Contributor

Is c needed for this test?

This comment has been minimized.

@bobot

bobot Jul 25, 2016

Contributor

It fills the old location of the C-array of a with 0.. The bug is not visible if it is removed. But I can use it so that it is not removed.

@bobot

bobot Jul 25, 2016

Contributor

It fills the old location of the C-array of a with 0.. The bug is not visible if it is removed. But I can use it so that it is not removed.

This comment has been minimized.

@braibant

braibant Jul 25, 2016

Contributor

I just re-tested this with a fresh install of 4.03, and I get the same behavior with or without the let c = ... line (modulo the warning). That is:

$ ocamlbuild -pkg bigarray t.native --
Finished, 4 targets (0 cached) in 00:00:00.
a.(0) = 42.000000
b.(0) = 42.000000
b.(0) = 0.000000

I am a bit confused about where this would come from. In any case. if you set c.(0) to something (like 21), it would make the issue more obvious, isn't it?

@braibant

braibant Jul 25, 2016

Contributor

I just re-tested this with a fresh install of 4.03, and I get the same behavior with or without the let c = ... line (modulo the warning). That is:

$ ocamlbuild -pkg bigarray t.native --
Finished, 4 targets (0 cached) in 00:00:00.
a.(0) = 42.000000
b.(0) = 42.000000
b.(0) = 0.000000

I am a bit confused about where this would come from. In any case. if you set c.(0) to something (like 21), it would make the issue more obvious, isn't it?

This comment has been minimized.

@bobot

bobot Jul 26, 2016

Contributor

It was like that in @alainfrisch POC, but I lost it when I make it work in both bytecode and native. Now c is readded and set.

@bobot

bobot Jul 26, 2016

Contributor

It was like that in @alainfrisch POC, but I lost it when I make it work in both bytecode and native. Now c is readded and set.

@@ -285,7 +285,8 @@ CAMLprim value caml_ephe_get_key_copy (value ar, value n)
if (is_ephe_key_none(ar, offset)) CAMLreturn (None_val);
v = Field (ar, offset);
if (Is_block (v) && Is_in_heap_or_young(v)) {
/** Don't copy custom_block #7279 */
if (Is_block (v) && Is_in_heap_or_young(v) && Tag_val(v) != Custom_tag ) {
elt = caml_alloc (Wosize_val (v), Tag_val (v));

This comment has been minimized.

@braibant

braibant Jul 23, 2016

Contributor

What happens here if Tag_val(v) = Forward_tag? I am thinking that this call to caml_alloc might short-circuit v and make it a custom block (of a different size than what's allocated for elt).

@braibant

braibant Jul 23, 2016

Contributor

What happens here if Tag_val(v) = Forward_tag? I am thinking that this call to caml_alloc might short-circuit v and make it a custom block (of a different size than what's allocated for elt).

This comment has been minimized.

@bobot

bobot Jul 25, 2016

Contributor

In #676 @jhjourdan added a while loop in order to handle similar case cb07812. I don't know which MR while be merged first. But I will fix the conflict in the later one. So at the end your case should be handled.

@bobot

bobot Jul 25, 2016

Contributor

In #676 @jhjourdan added a while loop in order to handle similar case cb07812. I don't know which MR while be merged first. But I will fix the conflict in the later one. So at the end your case should be handled.

This comment has been minimized.

@braibant

braibant Jul 25, 2016

Contributor

Ack. I reviewed the change you referenced, and it seems good, thanks!.

@braibant

braibant Jul 25, 2016

Contributor

Ack. I reviewed the change you referenced, and it seems good, thanks!.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Jul 25, 2016

Member

Aren't you breaking the GC invariant that every reachable block must be marked at the end of marking? If the value happens to be reachable only through the weak pointer, you're returning a pointer to a block that will never be marked, which will become a dangling pointer during the sweep phase.

Member

damiendoligez commented Jul 25, 2016

Aren't you breaking the GC invariant that every reachable block must be marked at the end of marking? If the value happens to be reachable only through the weak pointer, you're returning a pointer to a block that will never be marked, which will become a dangling pointer during the sweep phase.

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Jul 26, 2016

Contributor

Aren't you breaking the GC invariant that every reachable block must be marked at the end of marking?

😳 thank you. That was the drawback of this solution, I forgot to code it....

It is now fixed.

Contributor

bobot commented Jul 26, 2016

Aren't you breaking the GC invariant that every reachable block must be marked at the end of marking?

😳 thank you. That was the drawback of this solution, I forgot to code it....

It is now fixed.

Show outdated Hide outdated Changes
@@ -74,6 +74,9 @@ OCaml 4.04.0:
(Alain Frisch, request by Edgar Aroutiounian, review by Daniel Bunzli
and Damien Doligez)
-* PR#7279 GPR#710: `Weak.get_copy` `Ephemeron.*_copy` doesn't copy anymore custom block
(François Bobot, Alain Frisch, bug reported by sawfish (name?),

This comment has been minimized.

@martin-neuhaeusser

martin-neuhaeusser Aug 9, 2016

sawfish = Martin R. Neuhäußer

@martin-neuhaeusser

martin-neuhaeusser Aug 9, 2016

sawfish = Martin R. Neuhäußer

This comment has been minimized.

@bobot

bobot Aug 24, 2016

Contributor

Thanks fixed.

@bobot

bobot Aug 24, 2016

Contributor

Thanks fixed.

bobot added some commits Jul 22, 2016

Add a test for a break of the lifetime of a bigarray
   a proof of concept from Alain Frisch
`Weak.get_copy` don't copy custom block
  otherwise finaliser could be run twice and invariant broken
@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 28, 2016

Contributor

@bobot What is the status of this patch?

Contributor

mshinwell commented Dec 28, 2016

@bobot What is the status of this patch?

@martin-neuhaeusser

This comment has been minimized.

Show comment
Hide comment
@martin-neuhaeusser

martin-neuhaeusser Jan 31, 2017

I would very much like to see that patch integrated; can I do anything to help?

martin-neuhaeusser commented Jan 31, 2017

I would very much like to see that patch integrated; can I do anything to help?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 18, 2017

Contributor

Very little time remains for this code to find its way into release 4.05. Quick status report, please!

Contributor

xavierleroy commented Feb 18, 2017

Very little time remains for this code to find its way into release 4.05. Quick status report, please!

@bobot

This comment has been minimized.

Show comment
Hide comment
@bobot

bobot Feb 22, 2017

Contributor

Sorry, the status is that all the comments made have been fixed. So according to me it is good.

Contributor

bobot commented Feb 22, 2017

Sorry, the status is that all the comments made have been fixed. So according to me it is good.

@damiendoligez damiendoligez self-assigned this Feb 23, 2017

@damiendoligez damiendoligez merged commit 2b8836c into ocaml:trunk Feb 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Feb 23, 2017

Member

Merged in trunk and cherry-picked to 4.05.

Member

damiendoligez commented Feb 23, 2017

Merged in trunk and cherry-picked to 4.05.

damiendoligez added a commit that referenced this pull request Feb 23, 2017

PR#7279: `Weak.get_copy` doesn't copy custom block (#710)
`Weak.get_copy` don't copy custom block

  otherwise finaliser could be run twice and invariant broken

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

PR#7279: `Weak.get_copy` doesn't copy custom block (#710)
`Weak.get_copy` don't copy custom block

  otherwise finaliser could be run twice and invariant broken
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment