2019-12-14 13:55 AEDT

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0000239mercuryBugpublic2011-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)

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)

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