Fix bug in selection subexpression handling.

Did not work correctly if
1. a static expression was passed to a SPAR_ATOMVAL parameter,
2. other parameters to the same selection method were dynamic
   (so that the expression would not be completely eliminated during
   compilation), and
3. the evaluation group of the SEL_EXPRESSION element was dynamic
   (so that the SPAR_ATOMVAL expression could be evaluated for a
   different group during evaluation and compilation).
In this case, the static atom-valued parameter got evaluated during
compilation for the maximal evaluation group and replaced by a constant.
During evaluation, if the evaluation group was smaller, the values for
that expression were no longer correct.
See #1216 for what kinds of concrete selections this applies to.

Fixes #1216.

Backported from master with the same Change-Id.
Needed to also pull in part of the changes from
6e877567d9d3d08a3f6fb436f7bbfbaf35b95f8e.

Change-Id: Ic6b7f9b8df661a9c78d7862b981a07e65a7ebdbf

diff --git a/src/gmxlib/selection/compiler.c b/src/gmxlib/selection/compiler.c
index 1b5e664d37441b9e7f217b74a8d103376293ef2d..973d75b21d0209ca7f178279c66c45d6693ae9f5 100644 (file)
--- a/src/gmxlib/selection/compiler.c
+++ b/src/gmxlib/selection/compiler.c
@@ -306,8 +306,15 @@ enum
     SEL_CDATA_MINMAXALLOC = 16,
     /** Whether subexpressions use simple pass evaluation functions. */
     SEL_CDATA_SIMPLESUBEXPR = 32,
-    /** Whether this expressions is a part of a common subexpression. */
-    SEL_CDATA_COMMONSUBEXPR = 64 
+    /*! \brief
+     * Whether a static subexpression needs to support multiple evaluations.
+     *
+     * This flag may only be set on \ref SEL_SUBEXPR elements that also have
+     * SEL_CDATA_SIMPLESUBEXPR.
+     */
+    SEL_CDATA_STATICMULTIEVALSUBEXPR = 64,
+    /** Whether this expression is a part of a common subexpression. */
+    SEL_CDATA_COMMONSUBEXPR = 128
 };
 
 /*! \internal \brief
@@ -389,6 +396,10 @@ _gmx_selelem_print_compiler_info(FILE *fp, t_selelem *sel, int level)
     {
         fprintf(fp, "Ss");
     }
+    if (sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR)
+    {
+        fprintf(fp, "Sm");
+    }
     if (sel->cdata->flags & SEL_CDATA_COMMONSUBEXPR)
     {
         fprintf(fp, "Sc");
@@ -1094,19 +1105,25 @@ init_item_evalfunc(t_selelem *sel)
             break;
 
         case SEL_SUBEXPR:
-            sel->evaluate = (sel->refcount == 2
-                             ? &_gmx_sel_evaluate_subexpr_simple
-                             : &_gmx_sel_evaluate_subexpr);
+            if (sel->refcount == 2
+                && !(sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
+            {
+                sel->evaluate = &_gmx_sel_evaluate_subexpr_simple;
+            }
+            else
+            {
+                sel->evaluate = &_gmx_sel_evaluate_subexpr;
+            }
             break;
 
         case SEL_SUBEXPRREF:
             sel->name     = sel->child->name;
-            sel->evaluate = (sel->child->refcount == 2
+            sel->evaluate = ((sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
                              ? &_gmx_sel_evaluate_subexprref_simple
                              : &_gmx_sel_evaluate_subexprref);
             break;
     }
-
+    sel->cdata->evaluate = sel->evaluate;
     return TRUE;
 }
 
@@ -1168,7 +1185,8 @@ init_item_evaloutput(t_selelem *sel)
         }
     }
 
-    if (sel->type == SEL_SUBEXPR && sel->refcount == 2)
+    if (sel->type == SEL_SUBEXPR && sel->refcount == 2
+        && !(sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
     {
         sel->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA);
         if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE)
@@ -1188,7 +1206,8 @@ init_item_evaloutput(t_selelem *sel)
             _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr);
         }
     }
-    else if (sel->type == SEL_SUBEXPRREF && sel->child->refcount == 2)
+    else if (sel->type == SEL_SUBEXPRREF
+             && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR))
     {
         if (sel->v.u.ptr)
         {
@@ -1350,6 +1369,15 @@ init_item_staticeval(t_selelem *sel)
                     init_item_staticeval(child);
                 }
             }
+            /* If an expression is evaluated for a dynamic group, then also
+             * atom-valued parameters need to be evaluated every time. */
+            if ((sel->flags & SEL_DYNAMIC)
+                && (sel->type == SEL_EXPRESSION || sel->type == SEL_MODIFIER)
+                && (child->flags & SEL_ATOMVAL))
+            {
+                child->flags        |= SEL_DYNAMIC;
+                child->cdata->flags &= ~SEL_CDATA_STATIC;
+            }
             child = child->next;
         }
     }
@@ -1406,7 +1434,17 @@ init_item_subexpr_flags(t_selelem *sel)
     }
     else if (sel->type == SEL_SUBEXPRREF && sel->child->refcount == 2)
     {
-        sel->cdata->flags |= SEL_CDATA_SIMPLESUBEXPR;
+        /* See similar condition in init_item_staticeval(). */
+        if ((sel->flags & SEL_ATOMVAL)
+            && (sel->flags & SEL_DYNAMIC)
+            && !(sel->child->flags & SEL_DYNAMIC))
+        {
+            sel->child->cdata->flags |= SEL_CDATA_STATICMULTIEVALSUBEXPR;
+        }
+        else
+        {
+            sel->cdata->flags |= SEL_CDATA_SIMPLESUBEXPR;
+        }
     }
 
     /* Process children, but only follow subexpression references if the
@@ -2168,7 +2206,9 @@ analyze_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g)
             break;
 
         case SEL_SUBEXPR:
-            if (sel->cdata->flags & (SEL_CDATA_SIMPLESUBEXPR | SEL_CDATA_FULLEVAL))
+            if (((sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR) &&
+                 !(sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
+                || (sel->cdata->flags & SEL_CDATA_FULLEVAL))
             {
                 rc = sel->cdata->evaluate(data, sel, g);
                 _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr);
@@ -2477,6 +2517,17 @@ postprocess_item_subexpressions(t_selelem *sel)
         sel->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA);
         sel->child->v.nalloc = -1;
     }
+
+    /* For static subexpressions with a dynamic evaluation group, there is
+     * no need to evaluate them again, as the SEL_SUBEXPRREF takes care of
+     * everything during evaluation. */
+    if (sel->type == SEL_SUBEXPR
+        && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)
+        && (sel->cdata->flags & SEL_CDATA_STATICMULTIEVALSUBEXPR))
+    {
+        sel->evaluate        = NULL;
+        sel->cdata->evaluate = NULL;
+    }
 }
 
 
@@ -2721,24 +2772,30 @@ gmx_ana_selcollection_compile(gmx_ana_selcollection_t *sc)
             /* FIXME: Clean up the collection */
             return -1;
         }
-        /* Initialize evaluation function. */
-        if (!init_item_evalfunc(item))
-        {
-            /* FIXME: Clean up the collection */
-            return -1;
-        }
-        setup_memory_pooling(item, sc->mempool);
         /* Initialize the compiler data */
         init_item_compilerdata(item);
         init_item_staticeval(item);
         item = item->next;
     }
-    /* Initialize subexpression flags and evaluation output.
+    /* Initialize subexpression flags.
      * Requires compiler flags for the full tree. */
     item = sc->root;
     while (item)
     {
         init_item_subexpr_flags(item);
+        item = item->next;
+    }
+    /* Initialize evaluation.
+     * Requires subexpression flags. */
+    item = sc->root;
+    while (item)
+    {
+        if (!init_item_evalfunc(item))
+        {
+            /* FIXME: Clean up the collection */
+            return -1;
+        }
+        setup_memory_pooling(item, sc->mempool);
         init_item_evaloutput(item);
         item = item->next;
     }

diff --git a/src/gmxlib/selection/evaluate.c b/src/gmxlib/selection/evaluate.c
index 1913adad26fda89ea17d07a61a697e07dcbe1fea..c8bee7156f955ac1ee886cf9e3cb076e54e2a0af 100644 (file)
--- a/src/gmxlib/selection/evaluate.c
+++ b/src/gmxlib/selection/evaluate.c
@@ -182,7 +182,8 @@ gmx_ana_selcollection_evaluate(gmx_ana_selcollection_t *sc,
     while (sel)
     {
         /* Clear the evaluation group of subexpressions */
-        if (sel->child && sel->child->type == SEL_SUBEXPR)
+        if (sel->child && sel->child->type == SEL_SUBEXPR
+            && sel->child->evaluate != NULL)
         {
             sel->child->u.cgrp.isize = 0;
             /* Not strictly necessary, because the value will be overwritten
@@ -626,7 +627,7 @@ _gmx_sel_evaluate_subexprref(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_i
     t_selelem *expr;
     int        i, j;
 
-    if (g)
+    if (g != NULL && sel->child->evaluate != NULL)
     {
         int rc;