2024-11-21 23:44 AEDT
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0000239mercuryBugpublic2011-11-30 08:442011-12-05 14:18
Reportercolanderman 
Assigned To 
PrioritynormalSeverityminorReproducibilityalways
StatusnewResolutionopen 
Product Version 
Target VersionFixed in Version 
Summary0000239: Aliasing of partially instantiated structures in solutions.* predicates
DescriptionIf a partially instantiated structure is fully instantiated and output from the generator predicate of one of the solutions.* predicates, then each result generated will point at the same physical structure (see attached example).

This is because multi/nondet predicates do not deep copy partially instantiated structures when filling them in, and solutions.builtin_aggregate does not deep copy while generating results (except in the case of accurate garbage collection).

Since changing the former would result in pervasive ABI changes, I believe that changing solutions.builtin_aggregate to always copy generated results is the correct solution (albeit at a performance hit).
TagsNo tags attached.
Attached Files
  • ? file icon agg_bug.m (416 bytes) 2011-11-30 08:44
  • patch file icon solutions_copy.patch (3,685 bytes) 2011-11-30 08:45 - 
    --- solutions.m.orig	2011-11-29 16:07:47.000000000 -0500
+++ solutions.m	2011-11-29 16:21:49.000000000 -0500
@@ -786,7 +786,10 @@
   #define MR_PARTIAL_DEEP_COPY(SolutionsHeapPtr,                        \\
         OldVar, NewVal, TypeInfo_for_T)                                 \\
     do {                                                                \\
-        NewVal = OldVal;                                                \\
+        MR_save_transient_hp();                                         \\
+        NewVal = MR_deep_copy(OldVal, (MR_TypeInfo) TypeInfo_for_T,     \\
+            NULL, NULL);                                                \\
+        MR_restore_transient_hp();                                      \\
     } while (0)
 #else
   /*
@@ -803,7 +806,6 @@
         MR_restore_transient_hp();                                      \\
     } while (0)
 #endif
-
 ").
 
 :- pragma foreign_proc("C",
@@ -829,59 +831,62 @@
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::in, NewVal::out),
     [will_not_call_mercury, thread_safe],
 "
-    //
-    // For the IL back-end, we don't do heap reclamation on failure,
-    // so we don't need to worry about making deep copies here.
-    // Shallow copies will suffice.
-    //
-    NewVal = OldVal;
+    NewVal = builtin.deep_copy(OldVal);
 ").
 :- pragma foreign_proc("C#",
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::mdi, NewVal::muo),
     [will_not_call_mercury, thread_safe],
 "
-    NewVal = OldVal;
+    NewVal = builtin.deep_copy(OldVal);
 ").
 :- pragma foreign_proc("C#",
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::di, NewVal::uo),
     [will_not_call_mercury, thread_safe],
 "
-    NewVal = OldVal;
+    NewVal = builtin.deep_copy(OldVal);
 ").
 
 :- pragma foreign_proc("Java",
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::in, NewVal::out),
     [will_not_call_mercury, thread_safe],
 "
-    /*
-    ** For the Java back-end, as for the .NET implementation,
-    ** we don't do heap reclamation on failure,
-    ** so we don't need to worry about making deep copies here.
-    ** Shallow copies will suffice.
-    */
-    NewVal = OldVal;
+    try {
+        NewVal = builtin.deep_copy(OldVal);
+    } catch (java.lang.InstantiationException E) {
+        throw new RuntimeException(E);
+    } catch (java.lang.IllegalAccessException E) {
+        throw new RuntimeException(E);
+    }
 ").
 :- pragma foreign_proc("Java",
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::mdi, NewVal::muo),
     [will_not_call_mercury, thread_safe],
 "
-    NewVal = OldVal;
+    try {
+        NewVal = builtin.deep_copy(OldVal);
+    } catch (java.lang.InstantiationException E) {
+        throw new RuntimeException(E);
+    } catch (java.lang.IllegalAccessException E) {
+        throw new RuntimeException(E);
+    }
 ").
 :- pragma foreign_proc("Java",
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::di, NewVal::uo),
     [will_not_call_mercury, thread_safe],
 "
-    NewVal = OldVal;
+    try {
+        NewVal = builtin.deep_copy(OldVal);
+    } catch (java.lang.InstantiationException E) {
+        throw new RuntimeException(E);
+    } catch (java.lang.IllegalAccessException E) {
+        throw new RuntimeException(E);
+    }
 ").
 
 :- pragma foreign_proc("Erlang",
     partial_deep_copy(_SolutionsHeapPtr::in, OldVal::in, NewVal::out),
     [will_not_call_mercury, thread_safe],
 "
-    % For the Erlang back-end, as for the .NET implementation,
-    % we don't do heap reclamation on failure,
-    % so we don't need to worry about making deep copies here.
-    % Shallow copies will suffice.
     NewVal = OldVal
 ").
 :- pragma foreign_proc("Erlang",
    patch file icon solutions_copy.patch (3,685 bytes) 2011-11-30 08:45 +

-Relationships
+Relationships

-Notes

~0000419

colanderman (reporter)

2011-11-30 08:49

 Attached is a proof-of-concept patch against a 11.07 beta (not CVS, sorry) which fixes the bug in asm_fast.gc, hlc.gc, and java. I believe it also fixes the bug in C# grades, but I cannot test this. I am not sure that the bug exists in Erlang grades.

The attached patch is suboptimal in that it will unnecessarily copy the aggregation result as well. I suppose the changes in the patch should instead create a new predicate similar to partial_deep_copy, but perhaps there is a cleaner method.

~0000431

mark (administrator)

2011-12-05 14:18

 The "bug" here is just that the current implementation does not support partially instantiated data structures, so if you want to use them you should expect to need some workarounds. I don't want to pay the cost of unnecessary copying to partly support a feature that won't be fully supported anyway.

In this case a workaround is to perform the necessary copying yourself in the closure that is passed to solutions.*.
+Notes

-Issue History
Date Modified Username Field Change
2011-11-30 08:44 colanderman New Issue
2011-11-30 08:44 colanderman File Added: agg_bug.m
2011-11-30 08:45 colanderman File Added: solutions_copy.patch
2011-11-30 08:49 colanderman Note Added: 0000419
2011-12-05 14:18 mark Note Added: 0000431
+Issue History