d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Improve typing of POSIX search tree functions.

Browse Source 
Back in 2015 when I reimplemented these functions to use an AVL tree, I
was annoyed by the weakness of the typing of these functions. Both tree
nodes and keys are represented by 'void *', meaning that things like the
documentation for these functions are an absolute train wreck.

To make things worse, users of these functions need to cast the return
value of tfind()/tsearch() from 'void *' to 'type_of_key **' in order to
access the key. Technically speaking such casts violate aliasing rules.
I've observed actual breakages as a result of this by enabling features
like LTO.

I've filed a bug report at the Austin Group. Looking at the way the bug
got resolved, they made a pretty good step in the right direction. A new
type 'posix_tnode' has been added to correspond to tree nodes. It is
still defined as 'void' for source-level compatibility, but in the very
far future it could be replaced by a proper structure type containing a
key pointer.

MFC after:	1 month
Differential Revision:	https://reviews.freebsd.org/D8205
This commit is contained in:
Ed Schouten 2016-10-13 18:25:40 +00:00
parent ec7bbf1f79
commit 4ef9bd22ed
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=307227
7 changed files with 70 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
include/search.h
View File

@ -34,16 +34,18 @@ typedef enum {
} VISIT; } VISIT;
#ifdef _SEARCH_PRIVATE #ifdef _SEARCH_PRIVATE
typedef struct node { typedef struct __posix_tnode {
void *key; void *key;
struct node *llink, *rlink; struct __posix_tnode *llink, *rlink;
signed char balance; signed char balance;
} node_t; } posix_tnode;
struct que_elem { struct que_elem {
struct que_elem *next; struct que_elem *next;
struct que_elem *prev; struct que_elem *prev;
}; };
#else
typedef void posix_tnode;
#endif #endif
#if __BSD_VISIBLE #if __BSD_VISIBLE
@ -62,12 +64,15 @@ void *lfind(const void *, const void *, size_t *, size_t,
void *lsearch(const void *, void *, size_t *, size_t, void *lsearch(const void *, void *, size_t *, size_t,
int (*)(const void *, const void *)); int (*)(const void *, const void *));
void remque(void *); void remque(void *);
void *tdelete(const void * __restrict, void ** __restrict, void *tdelete(const void * __restrict, posix_tnode ** __restrict,
int (*)(const void *, const void *)); int (*)(const void *, const void *));
void *tfind(const void *, void * const *, posix_tnode *
tfind(const void *, posix_tnode * const *,
int (*)(const void *, const void *)); int (*)(const void *, const void *));
void *tsearch(const void *, void **, int (*)(const void *, const void *)); posix_tnode *
void twalk(const void *, void (*)(const void *, VISIT, int)); tsearch(const void *, posix_tnode **,
int (*)(const void *, const void *));
void twalk(const posix_tnode *, void (*)(const posix_tnode *, VISIT, int));
#if __BSD_VISIBLE #if __BSD_VISIBLE
int hcreate_r(size_t, struct hsearch_data *); int hcreate_r(size_t, struct hsearch_data *);

24
lib/libc/stdlib/tdelete.c
View File

@ -46,9 +46,9 @@ __FBSDID("$FreeBSD$");
* that we won't need to perform any rotations above \ * that we won't need to perform any rotations above \
* this point. In this case rotations are always \ * this point. In this case rotations are always \
* capable of keeping the subtree in balance. Make \ * capable of keeping the subtree in balance. Make \
* this the base node and reset the path. \ * this the root node and reset the path. \
*/ \ */ \
base = leaf; \ rootp = leaf; \
path_init(&path); \ path_init(&path); \
} \ } \
path_taking_left(&path); \ path_taking_left(&path); \
@ -59,7 +59,7 @@ __FBSDID("$FreeBSD$");
#define GO_RIGHT() do { \ #define GO_RIGHT() do { \
if ((*leaf)->balance == 0 || \ if ((*leaf)->balance == 0 || \
((*leaf)->balance > 0 && (*leaf)->llink->balance == 0)) { \ ((*leaf)->balance > 0 && (*leaf)->llink->balance == 0)) { \
base = leaf; \ rootp = leaf; \
path_init(&path); \ path_init(&path); \
} \ } \
path_taking_right(&path); \ path_taking_right(&path); \
@ -67,18 +67,16 @@ __FBSDID("$FreeBSD$");
} while (0) } while (0)
void * void *
tdelete(const void *restrict key, void **restrict rootp, tdelete(const void *restrict key, posix_tnode **restrict rootp,
int (*compar)(const void *, const void *)) int (*compar)(const void *, const void *))
{ {
struct path path; struct path path;
node_t *root, **base, **leaf, *old, **n, *x, *y, *z; posix_tnode **leaf, *old, **n, *x, *y, *z, *result;
void *result;
int cmp; int cmp;
/* POSIX requires that tdelete() returns NULL if rootp is NULL. */ /* POSIX requires that tdelete() returns NULL if rootp is NULL. */
if (rootp == NULL) if (rootp == NULL)
return (NULL); return (NULL);
root = *rootp;
/* /*
* Find the leaf that needs to be removed. Return if we cannot * Find the leaf that needs to be removed. Return if we cannot
@ -86,19 +84,18 @@ tdelete(const void *restrict key, void **restrict rootp,
* to get to the node, as we will need it to adjust the * to get to the node, as we will need it to adjust the
* balances. * balances.
*/ */
result = (void *)1; result = (posix_tnode *)1;
path_init(&path); path_init(&path);
base = &root; leaf = rootp;
leaf = &root;
for (;;) { for (;;) {
if (*leaf == NULL) if (*leaf == NULL)
return (NULL); return (NULL);
cmp = compar(key, (*leaf)->key); cmp = compar(key, (*leaf)->key);
if (cmp < 0) { if (cmp < 0) {
result = &(*leaf)->key; result = *leaf;
GO_LEFT(); GO_LEFT();
} else if (cmp > 0) { } else if (cmp > 0) {
result = &(*leaf)->key; result = *leaf;
GO_RIGHT(); GO_RIGHT();
} else { } else {
break; break;
@ -134,7 +131,7 @@ tdelete(const void *restrict key, void **restrict rootp,
* and left-left case that only exists when deleting. Hence the * and left-left case that only exists when deleting. Hence the
* duplication of code. * duplication of code.
*/ */
for (n = base; n != leaf;) { for (n = rootp; n != leaf;) {
if (path_took_left(&path)) { if (path_took_left(&path)) {
x = *n; x = *n;
if (x->balance < 0) { if (x->balance < 0) {
@ -207,6 +204,5 @@ tdelete(const void *restrict key, void **restrict rootp,
} }
/* Return the parent of the old entry. */ /* Return the parent of the old entry. */
*rootp = root;
return (result); return (result);
} }

7
lib/libc/stdlib/tfind.c
View File

@ -4,8 +4,6 @@
* Tree search generalized from Knuth (6.2.2) Algorithm T just like * Tree search generalized from Knuth (6.2.2) Algorithm T just like
* the AT&T man page says. * the AT&T man page says.
* *
* The node_t structure is for internal use only, lint doesn't grok it.
*
* Written by reading the System V Interface Definition, not the code. * Written by reading the System V Interface Definition, not the code.
* *
* Totally public domain. * Totally public domain.
@ -29,11 +27,10 @@ __FBSDID("$FreeBSD$");
* vkey - key to be found * vkey - key to be found
* vrootp - address of the tree root * vrootp - address of the tree root
*/ */
void * posix_tnode *
tfind(const void *vkey, void * const *vrootp, tfind(const void *vkey, posix_tnode * const *rootp,
int (*compar)(const void *, const void *)) int (*compar)(const void *, const void *))
{ {
node_t **rootp = (node_t **)vrootp;
if (rootp == NULL) if (rootp == NULL)
return NULL; return NULL;

29
lib/libc/stdlib/tsearch.3
View File

@ -27,7 +27,7 @@
.\" OpenBSD: tsearch.3,v 1.2 1998/06/21 22:13:49 millert Exp .\" OpenBSD: tsearch.3,v 1.2 1998/06/21 22:13:49 millert Exp
.\" $FreeBSD$ .\" $FreeBSD$
.\" .\"
.Dd December 6, 2015 .Dd October 9, 2016
.Dt TSEARCH 3 .Dt TSEARCH 3
.Os .Os
.Sh NAME .Sh NAME
@ -36,13 +36,13 @@
.Sh SYNOPSIS .Sh SYNOPSIS
.In search.h .In search.h
.Ft void * .Ft void *
.Fn tdelete "const void * restrict key" "void ** restrict rootp" "int (*compar) (const void *, const void *)" .Fn tdelete "const void * restrict key" "posix_tnode ** restrict rootp" "int (*compar) (const void *, const void *)"
.Ft void * .Ft posix_tnode *
.Fn tfind "const void *key" "void * const *rootp" "int (*compar) (const void *, const void *)" .Fn tfind "const void *key" "posix_tnode * const *rootp" "int (*compar) (const void *, const void *)"
.Ft void * .Ft posix_tnode *
.Fn tsearch "const void *key" "void **rootp" "int (*compar) (const void *, const void *)" .Fn tsearch "const void *key" "posix_tnode **rootp" "int (*compar) (const void *, const void *)"
.Ft void .Ft void
.Fn twalk "const void *root" "void (*action) (const void *, VISIT, int)" .Fn twalk "const posix_tnode *root" "void (*action) (const posix_tnode *, VISIT, int)"
.Sh DESCRIPTION .Sh DESCRIPTION
The The
.Fn tdelete , .Fn tdelete ,
@ -134,3 +134,18 @@ function returns no value.
.Xr bsearch 3 , .Xr bsearch 3 ,
.Xr hsearch 3 , .Xr hsearch 3 ,
.Xr lsearch 3 .Xr lsearch 3
.Sh STANDARDS
These functions conform to
.St -p1003.1-2008 .
.Pp
The
.Fa posix_tnode
type is not part of
.St -p1003.1-2008 ,
but it is expected to be standardized by future versions of the standard.
It is defined as
.Fa void
for source-level compatibility.
Using
.Fa posix_tnode
makes it easier to distinguish between nodes and keys.

35
lib/libc/stdlib/tsearch.c
View File

@ -32,18 +32,17 @@ __FBSDID("$FreeBSD$");
#include "tsearch_path.h" #include "tsearch_path.h"
void * posix_tnode *
tsearch(const void *key, void **rootp, tsearch(const void *key, posix_tnode **rootp,
int (*compar)(const void *, const void *)) int (*compar)(const void *, const void *))
{ {
struct path path; struct path path;
node_t *root, **base, **leaf, *result, *n, *x, *y, *z; posix_tnode **leaf, *result, *n, *x, *y, *z;
int cmp; int cmp;
/* POSIX requires that tsearch() returns NULL if rootp is NULL. */ /* POSIX requires that tsearch() returns NULL if rootp is NULL. */
if (rootp == NULL) if (rootp == NULL)
return (NULL); return (NULL);
root = *rootp;
/* /*
* Find the leaf where the new key needs to be inserted. Return * Find the leaf where the new key needs to be inserted. Return
@ -52,8 +51,7 @@ tsearch(const void *key, void **rootp,
* balances. * balances.
*/ */
path_init(&path); path_init(&path);
base = &root; leaf = rootp;
leaf = &root;
while (*leaf != NULL) { while (*leaf != NULL) {
if ((*leaf)->balance != 0) { if ((*leaf)->balance != 0) {
/* /*
@ -62,9 +60,9 @@ tsearch(const void *key, void **rootp,
* need to perform any rotations above this * need to perform any rotations above this
* point. In this case rotations are always * point. In this case rotations are always
* capable of keeping the subtree in balance. * capable of keeping the subtree in balance.
* Make this the base node and reset the path. * Make this the root node and reset the path.
*/ */
base = leaf; rootp = leaf;
path_init(&path); path_init(&path);
} }
cmp = compar(key, (*leaf)->key); cmp = compar(key, (*leaf)->key);
@ -75,7 +73,7 @@ tsearch(const void *key, void **rootp,
path_taking_right(&path); path_taking_right(&path);
leaf = &(*leaf)->rlink; leaf = &(*leaf)->rlink;
} else { } else {
return (&(*leaf)->key); return (*leaf);
} }
} }
@ -94,7 +92,7 @@ tsearch(const void *key, void **rootp,
* have a balance of zero, meaning that these nodes will not get * have a balance of zero, meaning that these nodes will not get
* out of balance. * out of balance.
*/ */
for (n = *base; n != *leaf;) { for (n = *rootp; n != *leaf;) {
if (path_took_left(&path)) { if (path_took_left(&path)) {
n->balance += 1; n->balance += 1;
n = n->llink; n = n->llink;
@ -106,10 +104,10 @@ tsearch(const void *key, void **rootp,
/* /*
* Adjusting the balances may have pushed the balance of the * Adjusting the balances may have pushed the balance of the
* base node out of range. Perform a rotation to bring the * root node out of range. Perform a rotation to bring the
* balance back in range. * balance back in range.
*/ */
x = *base; x = *rootp;
if (x->balance > 1) { if (x->balance > 1) {
y = x->llink; y = x->llink;
if (y->balance < 0) { if (y->balance < 0) {
@ -129,7 +127,7 @@ tsearch(const void *key, void **rootp,
z->llink = y; z->llink = y;
x->llink = z->rlink; x->llink = z->rlink;
z->rlink = x; z->rlink = x;
*base = z; *rootp = z;
x->balance = z->balance > 0 ? -1 : 0; x->balance = z->balance > 0 ? -1 : 0;
y->balance = z->balance < 0 ? 1 : 0; y->balance = z->balance < 0 ? 1 : 0;
@ -146,7 +144,7 @@ tsearch(const void *key, void **rootp,
*/ */
x->llink = y->rlink; x->llink = y->rlink;
y->rlink = x; y->rlink = x;
*base = y; *rootp = y;
x->balance = 0; x->balance = 0;
y->balance = 0; y->balance = 0;
@ -165,12 +163,12 @@ tsearch(const void *key, void **rootp,
* / \ A B C D * / \ A B C D
* B C * B C
*/ */
node_t *z = y->llink; posix_tnode *z = y->llink;
x->rlink = z->llink; x->rlink = z->llink;
z->llink = x; z->llink = x;
y->llink = z->rlink; y->llink = z->rlink;
z->rlink = y; z->rlink = y;
*base = z; *rootp = z;
x->balance = z->balance < 0 ? 1 : 0; x->balance = z->balance < 0 ? 1 : 0;
y->balance = z->balance > 0 ? -1 : 0; y->balance = z->balance > 0 ? -1 : 0;
@ -187,7 +185,7 @@ tsearch(const void *key, void **rootp,
*/ */
x->rlink = y->llink; x->rlink = y->llink;
y->llink = x; y->llink = x;
*base = y; *rootp = y;
x->balance = 0; x->balance = 0;
y->balance = 0; y->balance = 0;
@ -195,6 +193,5 @@ tsearch(const void *key, void **rootp,
} }
/* Return the new entry. */ /* Return the new entry. */
*rootp = root; return (result);
return (&result->key);
} }

9
lib/libc/stdlib/twalk.c
View File

@ -4,8 +4,6 @@
* Tree search generalized from Knuth (6.2.2) Algorithm T just like * Tree search generalized from Knuth (6.2.2) Algorithm T just like
* the AT&T man page says. * the AT&T man page says.
* *
* The node_t structure is for internal use only, lint doesn't grok it.
*
* Written by reading the System V Interface Definition, not the code. * Written by reading the System V Interface Definition, not the code.
* *
* Totally public domain. * Totally public domain.
@ -23,12 +21,11 @@ __FBSDID("$FreeBSD$");
#include <search.h> #include <search.h>
#include <stdlib.h> #include <stdlib.h>
typedef void (*cmp_fn_t)(const void *, VISIT, int); typedef void (*cmp_fn_t)(const posix_tnode *, VISIT, int);
/* Walk the nodes of a tree */ /* Walk the nodes of a tree */
static void static void
trecurse(const node_t *root, /* Root of the tree to be walked */ trecurse(const posix_tnode *root, cmp_fn_t action, int level)
cmp_fn_t action, int level)
{ {
if (root->llink == NULL && root->rlink == NULL) if (root->llink == NULL && root->rlink == NULL)
@ -46,7 +43,7 @@ trecurse(const node_t *root, /* Root of the tree to be walked */
/* Walk the nodes of a tree */ /* Walk the nodes of a tree */
void void
twalk(const void *vroot, cmp_fn_t action) /* Root of the tree to be walked */ twalk(const posix_tnode *vroot, cmp_fn_t action)
{ {
if (vroot != NULL && action != NULL) if (vroot != NULL && action != NULL)
trecurse(vroot, action, 0); trecurse(vroot, action, 0);

4
lib/libc/tests/stdlib/tsearch_test.c
View File

@ -34,7 +34,7 @@ __FBSDID("$FreeBSD$");
/* Validates the integrity of an AVL tree. */ /* Validates the integrity of an AVL tree. */
static inline unsigned int static inline unsigned int
tnode_assert(const node_t *n) tnode_assert(const posix_tnode *n)
{ {
unsigned int height_left, height_right; unsigned int height_left, height_right;
int balance; int balance;
@ -79,7 +79,7 @@ ATF_TC_BODY(tsearch_test, tc)
keys[i] = i; keys[i] = i;
/* Apply random operations on a binary tree and check the results. */ /* Apply random operations on a binary tree and check the results. */
void *root = NULL; posix_tnode *root = NULL;
bool present[NKEYS] = {}; bool present[NKEYS] = {};
for (int i = 0; i < NKEYS * 10; ++i) { for (int i = 0; i < NKEYS * 10; ++i) {
int key = nrand48(random_state) % NKEYS; int key = nrand48(random_state) % NKEYS;