splay: use access functions, add asserts, use Curl_timediff

- add set/get functions for the custom data in a tree node

- use Curl_timediff for time comparisons instead of the custom macro, as they
  do the exact same things.

- add asserts to catch mistakes better

- updated test 1309 accordingly

Closes #14562

lib/multi.c

@@ -2665,7 +2665,7 @@ statemachine_end:
 CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles)
 {
   CURLMcode returncode = CURLM_OK;
-  struct Curl_tree *t;
+  struct Curl_tree *t = NULL;
   struct curltime now = Curl_now();
   struct Curl_llist_node *e;
   struct Curl_llist_node *n = NULL;
@@ -2716,7 +2716,7 @@ CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles)
     multi->timetree = Curl_splaygetbest(now, multi->timetree, &t);
     if(t) {
       /* the removed may have another timeout in queue */
-      struct Curl_easy *data = t->payload;
+      struct Curl_easy *data = Curl_splayget(t);
       if(data->mstate == MSTATE_PENDING) {
         bool stream_unused;
         CURLcode result_unused;
@@ -2726,7 +2726,7 @@ CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles)
           move_pending_to_connect(multi, data);
         }
       }
-      (void)add_next_timeout(now, multi, t->payload);
+      (void)add_next_timeout(now, multi, Curl_splayget(t));
     }
   } while(t);
 
@@ -3153,7 +3153,7 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
 {
   CURLMcode result = CURLM_OK;
   struct Curl_easy *data = NULL;
-  struct Curl_tree *t;
+  struct Curl_tree *t = NULL;
   struct curltime now = Curl_now();
   bool run_conn_cache = FALSE;
   SIGPIPE_VARIABLE(pipe_st);
@@ -3256,8 +3256,8 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
 
     multi->timetree = Curl_splaygetbest(now, multi->timetree, &t);
     if(t) {
-      data = t->payload; /* assign this for next loop */
+      data = Curl_splayget(t); /* assign this for next loop */
-      (void)add_next_timeout(now, multi, t->payload);
+      (void)add_next_timeout(now, multi, data);
     }
 
   } while(t);
@@ -3408,7 +3408,10 @@ static CURLMcode multi_timeout(struct Curl_multi *multi,
     /* splay the lowest to the bottom */
     multi->timetree = Curl_splay(tv_zero, multi->timetree);
 
-    if(Curl_splaycomparekeys(multi->timetree->key, now) > 0) {
+    /* 'multi->timetree' will be non-NULL here but the compilers sometimes
+       yell at us if we assume so */
+    if(multi->timetree &&
+       Curl_timediff_us(multi->timetree->key, now) > 0) {
       /* some time left before expiration */
       timediff_t diff = Curl_timediff_ceil(multi->timetree->key, now);
       /* this should be safe even on 32-bit archs, as we do not use that
@@ -3454,7 +3457,7 @@ CURLMcode Curl_update_timer(struct Curl_multi *multi)
   }
   if(timeout_ms < 0) {
     static const struct curltime none = {0, 0};
-    if(Curl_splaycomparekeys(none, multi->timer_lastcall)) {
+    if(Curl_timediff_us(none, multi->timer_lastcall)) {
       multi->timer_lastcall = none;
       /* there is no timeout now but there was one previously, tell the app to
          disable it */
@@ -3474,7 +3477,7 @@ CURLMcode Curl_update_timer(struct Curl_multi *multi)
    * timeout we got the (relative) time-out time for. We can thus easily check
    * if this is the same (fixed) time as we got in a previous call and then
    * avoid calling the callback again. */
-  if(Curl_splaycomparekeys(multi->timetree->key, multi->timer_lastcall) == 0)
+  if(Curl_timediff_us(multi->timetree->key, multi->timer_lastcall) == 0)
     return CURLM_OK;
 
   multi->timer_lastcall = multi->timetree->key;
@@ -3616,7 +3619,7 @@ void Curl_expire(struct Curl_easy *data, timediff_t milli, expire_id id)
   /* Indicate that we are in the splay tree and insert the new timer expiry
      value since it is our local minimum. */
   *nowp = set;
-  data->state.timenode.payload = data;
+  Curl_splayset(&data->state.timenode, data);
   multi->timetree = Curl_splayinsert(*nowp, multi->timetree,
                                      &data->state.timenode);
 }

lib/splay.c

@@ -24,6 +24,7 @@
 
 #include "curl_setup.h"
 
+#include "timeval.h"
 #include "splay.h"
 
 /*
@@ -33,7 +34,7 @@
  *  zero          : when i is equal   to   j
  *  positive when : when i is larger  than j
  */
-#define compare(i,j) Curl_splaycomparekeys((i),(j))
+#define compare(i,j) Curl_timediff_us(i,j)
 
 /*
  * Splay using the key i (which may or may not be in the tree.) The starting
@@ -45,12 +46,12 @@ struct Curl_tree *Curl_splay(struct curltime i,
   struct Curl_tree N, *l, *r, *y;
 
   if(!t)
-    return t;
+    return NULL;
   N.smaller = N.larger = NULL;
   l = r = &N;
 
   for(;;) {
-    long comp = compare(i, t->key);
+    timediff_t comp = compare(i, t->key);
     if(comp < 0) {
       if(!t->smaller)
         break;
@@ -106,11 +107,11 @@ struct Curl_tree *Curl_splayinsert(struct curltime i,
     ~0, -1
   }; /* will *NEVER* appear */
 
-  if(!node)
+  DEBUGASSERT(node);
-    return t;
 
   if(t) {
     t = Curl_splay(i, t);
+    DEBUGASSERT(t);
     if(compare(i, t->key) == 0) {
       /* There already exists a node in the tree with the very same key. Build
          a doubly-linked circular list of nodes. We add the new 'node' struct
@@ -166,6 +167,7 @@ struct Curl_tree *Curl_splaygetbest(struct curltime i,
 
   /* find smallest */
   t = Curl_splay(tv_zero, t);
+  DEBUGASSERT(t);
   if(compare(i, t->key) < 0) {
     /* even the smallest is too big */
     *removed = NULL;
@@ -217,9 +219,11 @@ int Curl_splayremove(struct Curl_tree *t,
   }; /* will *NEVER* appear */
   struct Curl_tree *x;
 
-  if(!t || !removenode)
+  if(!t)
     return 1;
 
+  DEBUGASSERT(removenode);
+
   if(compare(KEY_NOTUSED, removenode->key) == 0) {
     /* Key set to NOTUSED means it is a subnode within a 'same' linked list
        and thus we can unlink it easily. */
@@ -238,6 +242,7 @@ int Curl_splayremove(struct Curl_tree *t,
   }
 
   t = Curl_splay(removenode->key, t);
+  DEBUGASSERT(t);
 
   /* First make sure that we got the same root node as the one we want
      to remove, as otherwise we might be trying to remove a node that
@@ -268,6 +273,7 @@ int Curl_splayremove(struct Curl_tree *t,
       x = t->larger;
     else {
       x = Curl_splay(removenode->key, t->smaller);
+      DEBUGASSERT(x);
       x->larger = t->larger;
     }
   }
@@ -276,3 +282,16 @@ int Curl_splayremove(struct Curl_tree *t,
 
   return 0;
 }
+
+/* set and get the custom payload for this tree node */
+void Curl_splayset(struct Curl_tree *node, void *payload)
+{
+  DEBUGASSERT(node);
+  node->ptr = payload;
+}
+
+void *Curl_splayget(struct Curl_tree *node)
+{
+  DEBUGASSERT(node);
+  return node->ptr;
+}

lib/splay.h

@@ -26,13 +26,14 @@
 #include "curl_setup.h"
 #include "timeval.h"
 
+/* only use function calls to access this struct */
 struct Curl_tree {
   struct Curl_tree *smaller; /* smaller node */
   struct Curl_tree *larger;  /* larger node */
   struct Curl_tree *samen;   /* points to the next node with identical key */
   struct Curl_tree *samep;   /* points to the prev node with identical key */
-  struct curltime key;        /* this node's "sort" key */
+  struct curltime key;       /* this node's "sort" key */
-  void *payload;             /* data the splay code does not care about */
+  void *ptr;                 /* data the splay code does not care about */
 };
 
 struct Curl_tree *Curl_splay(struct curltime i,
@@ -50,9 +51,8 @@ int Curl_splayremove(struct Curl_tree *t,
                      struct Curl_tree *removenode,
                      struct Curl_tree **newroot);
 
-#define Curl_splaycomparekeys(i,j) ( ((i.tv_sec)  < (j.tv_sec)) ? -1 : \
+/* set and get the custom payload for this tree node */
-                                   ( ((i.tv_sec)  > (j.tv_sec)) ?  1 : \
+void Curl_splayset(struct Curl_tree *node, void *payload);
-                                   ( ((i.tv_usec) < (j.tv_usec)) ? -1 : \
+void *Curl_splayget(struct Curl_tree *node);
-                                   ( ((i.tv_usec) > (j.tv_usec)) ?  1 : 0))))
 
 #endif /* HEADER_CURL_SPLAY_H */

tests/unit/unit1309.c

@@ -88,7 +88,7 @@ UNITTEST_START
     key.tv_sec = 0;
     key.tv_usec = (541*i)%1023;
     storage[i] = key.tv_usec;
-    nodes[i].payload = &storage[i];
+    Curl_splayset(&nodes[i], &storage[i]);
     root = Curl_splayinsert(key, root, &nodes[i]);
   }
 
@@ -100,7 +100,7 @@ UNITTEST_START
     printf("Tree look:\n");
     splayprint(root, 0, 1);
     printf("remove pointer %d, payload %zu\n", rem,
-           *(size_t *)nodes[rem].payload);
+           *(size_t *)Curl_splayget(&nodes[rem]));
     rc = Curl_splayremove(root, &nodes[rem], &root);
     if(rc) {
       /* failed! */
@@ -121,7 +121,7 @@ UNITTEST_START
     /* add some nodes with the same key */
     for(j = 0; j <= i % 3; j++) {
       storage[i * 3 + j] = key.tv_usec*10 + j;
-      nodes[i * 3 + j].payload = &storage[i * 3 + j];
+      Curl_splayset(&nodes[i * 3 + j], &storage[i * 3 + j]);
       root = Curl_splayinsert(key, root, &nodes[i * 3 + j]);
     }
   }
@@ -133,8 +133,8 @@ UNITTEST_START
     root = Curl_splaygetbest(tv_now, root, &removed);
     while(removed) {
       printf("removed payload %zu[%zu]\n",
-             (*(size_t *)removed->payload) / 10,
+             *(size_t *)Curl_splayget(removed) / 10,
-             (*(size_t *)removed->payload) % 10);
+             *(size_t *)Curl_splayget(removed) % 10);
       root = Curl_splaygetbest(tv_now, root, &removed);
     }
   }