commonlib/list: Support circular list

In some use cases, we want to add items to the linked list and then
iterate over them with the insertion order. With the current API, the
call site needs to either use the inefficient list_append() function to
append items to the end of the list, or manually maintain a "tail"
node pointer.

To support that use case and make the change backward compatible, add a
helper list_init() function to initialize the list as a circular linked
list. list_init() is automatically called within list_insert_after() and
list_append(). In list_insert_before(), an assertion is added to avoid
an insertion before the head node (which should be invalid). The
implementation ensures that the list is initialized as a circular one
whenever the first element is added. That also allows all call sites to
be auto-upgraded to the "circular list" implementation without any
modification.

Modify list_for_each() to support circular lists, and improve
list_append() efficiency by inserting the new node before the
placeholder head node. Also add a few assertions in the implementation.

Add a new test case to test iterating over an empty list.

Note that '(uintptr_t)ptr + (uintptr_t)offsetof(typeof(*(ptr)), member)'
was used instead of the simpler '&((ptr)->member)' because GCC9+ assumes
that the address can never be NULL. See commit 88991caf00
("include/list.h: Add support for GCC9+) for details. Now, with the
new list_for_each() implementation, that pointer value can never be
NULL.

Change-Id: I8451f711d4e522e239c241b3943e00070896dec9
Signed-off-by: Yu-Ping Wu <yupingso@chromium.org>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/90799
Reviewed-by: Julius Werner <jwerner@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>

src/commonlib/include/commonlib/list.h

@@ -17,14 +17,25 @@ void list_remove(struct list_node *node);
 // Insert list_node node after list_node after in a doubly linked list.
 void list_insert_after(struct list_node *node, struct list_node *after);
 // Insert list_node node before list_node before in a doubly linked list.
+// `before` must not be the placeholder head node.
 void list_insert_before(struct list_node *node, struct list_node *before);
-// Appends the node to the end of the list.
+// Append the node to the end of the list.
 void list_append(struct list_node *node, struct list_node *head);
 
-#define list_for_each(ptr, head, member)                                      \
-	for ((ptr) = container_of((head).next, typeof(*(ptr)), member);       \
-		(uintptr_t)ptr + (uintptr_t)offsetof(typeof(*(ptr)), member); \
-		(ptr) = container_of((ptr)->member.next,                      \
+/*
+ * Explanation of `ptr` initialization:
+ * 1. head.next != NULL: This means the list isn't empty. As the implementation ensures that
+ *    list_init() is called when the very first element is added, we can safely assume that
+ *    the list is circular, and hence set `ptr` to the 1st element.
+ * 2. head.next == NULL: This means the list is empty, and list_init() hasn't been called.
+ *    As the `head` arg might be const, we cannot simply call list_init() here. Instead, we set
+ *    `ptr` to a special value such that `&(ptr->member) == &head`, causing the loop to
+ *    terminate immediately.
+ */
+#define list_for_each(ptr, head, member) \
+	for ((ptr) = container_of((head).next ?: &(head), typeof(*(ptr)), member); \
+		(&((ptr)->member) != &(head)); \
+		(ptr) = container_of((ptr)->member.next, \
 			typeof(*(ptr)), member))
 
 #endif /* __COMMONLIB_LIST_H__ */

src/commonlib/list.c

@@ -1,10 +1,22 @@
 /* Taken from depthcharge: src/base/list.c */
 /* SPDX-License-Identifier: GPL-2.0-or-later */
 
+#include <assert.h>
 #include <commonlib/list.h>
 
+// Initialize a circular list, with `head` being a placeholder head node.
+static void list_init(struct list_node *head)
+{
+	if (!head->next) {
+		assert(!head->prev);
+		head->next = head->prev = head;
+	}
+}
+
 void list_remove(struct list_node *node)
 {
+	/* Cannot remove the head node. */
+	assert(node->prev && node->next);
 	if (node->prev)
 		node->prev->next = node->next;
 	if (node->next)
@@ -13,6 +25,9 @@ void list_remove(struct list_node *node)
 
 void list_insert_after(struct list_node *node, struct list_node *after)
 {
+	/* Check uninitialized head node. */
+	if (after->prev == NULL)
+		list_init(after);
 	node->next = after->next;
 	node->prev = after;
 	after->next = node;
@@ -22,6 +37,8 @@ void list_insert_after(struct list_node *node, struct list_node *after)
 
 void list_insert_before(struct list_node *node, struct list_node *before)
 {
+	/* `before` cannot be an uninitialized head node. */
+	assert(before->prev)
 	node->prev = before->prev;
 	node->next = before;
 	before->prev = node;
@@ -31,8 +48,7 @@ void list_insert_before(struct list_node *node, struct list_node *before)
 
 void list_append(struct list_node *node, struct list_node *head)
 {
-	while (head->next)
-		head = head->next;
-
-	list_insert_after(node, head);
+	list_init(head);
+	/* With a circular list, we just need to insert before the head. */
+	list_insert_before(node, head);
 }

tests/commonlib/list-test.c

@@ -1,9 +1,10 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
-#include <tests/test.h>
+#include <commonlib/list.h>
 #include <stdlib.h>
 #include <string.h>
-#include <commonlib/list.h>
+#include <tests/test.h>
+#include <types.h>
 
 struct test_container {
 	int value;
@@ -11,10 +12,20 @@ struct test_container {
 	struct list_node list_node;
 };
 
-void test_list_insert_after(void **state)
+static void test_list_empty(void **state)
+{
+	struct list_node head = {};
+	struct test_container *node;
+
+	list_for_each(node, head, list_node) {
+		assert_true(false);
+	}
+}
+
+static void test_list_insert_after(void **state)
 {
 	int i = 0;
-	struct list_node root = { .prev = NULL, .next = NULL };
+	struct list_node head = {};
 	struct test_container *c1 = (struct test_container *)malloc(sizeof(*c1));
 	struct test_container *c2 = (struct test_container *)malloc(sizeof(*c2));
 	struct test_container *c3 = (struct test_container *)malloc(sizeof(*c2));
@@ -29,11 +40,11 @@ void test_list_insert_after(void **state)
 	c2->value = values[1];
 	c3->value = values[2];
 
-	list_insert_after(&c1->list_node, &root);
+	list_insert_after(&c1->list_node, &head);
 	list_insert_after(&c2->list_node, &c1->list_node);
 	list_insert_after(&c3->list_node, &c2->list_node);
 
-	list_for_each(ptr, root, list_node) {
+	list_for_each(ptr, head, list_node) {
 		assert_int_equal(values[i], ptr->value);
 		i++;
 	}
@@ -45,10 +56,10 @@ void test_list_insert_after(void **state)
 	free(c1);
 }
 
-void test_list_insert_before(void **state)
+static void test_list_insert_before(void **state)
 {
 	int i = 0;
-	struct list_node root = { .prev = NULL, .next = NULL };
+	struct list_node head = {};
 	struct test_container *c1 = (struct test_container *)malloc(sizeof(*c1));
 	struct test_container *c2 = (struct test_container *)malloc(sizeof(*c2));
 	struct test_container *c3 = (struct test_container *)malloc(sizeof(*c2));
@@ -63,12 +74,11 @@ void test_list_insert_before(void **state)
 	c2->value = values[1];
 	c3->value = values[2];
 
-	list_insert_after(&c3->list_node, &root);
+	list_insert_after(&c3->list_node, &head);
 	list_insert_before(&c2->list_node, &c3->list_node);
 	list_insert_before(&c1->list_node, &c2->list_node);
 
-
-	list_for_each(ptr, root, list_node) {
+	list_for_each(ptr, head, list_node) {
 		assert_int_equal(values[i], ptr->value);
 		i++;
 	}
@@ -80,19 +90,27 @@ void test_list_insert_before(void **state)
 	free(c1);
 }
 
-void test_list_remove(void **state)
+static void test_list_insert_before_head(void **state)
 {
-	struct list_node root = { .prev = NULL, .next = NULL };
+	struct list_node head = {};
+	struct test_container c = {};
+
+	expect_assert_failure(list_insert_before(&c.list_node, &head));
+}
+
+static void test_list_remove(void **state)
+{
+	struct list_node head = {};
 	struct test_container *c1 = (struct test_container *)malloc(sizeof(*c1));
 	struct test_container *c2 = (struct test_container *)malloc(sizeof(*c2));
 	struct test_container *ptr;
 	int len;
 
-	list_insert_after(&c1->list_node, &root);
+	list_insert_after(&c1->list_node, &head);
 	list_insert_after(&c2->list_node, &c1->list_node);
 
 	len = 0;
-	list_for_each(ptr, root, list_node) {
+	list_for_each(ptr, head, list_node) {
 		len++;
 	}
 	assert_int_equal(2, len);
@@ -100,14 +118,14 @@ void test_list_remove(void **state)
 	list_remove(&c1->list_node);
 
 	len = 0;
-	list_for_each(ptr, root, list_node) {
+	list_for_each(ptr, head, list_node) {
 		len++;
 	}
 	assert_int_equal(1, len);
 
 	list_remove(&c2->list_node);
 	len = 0;
-	list_for_each(ptr, root, list_node) {
+	list_for_each(ptr, head, list_node) {
 		len++;
 	}
 	assert_int_equal(0, len);
@@ -116,20 +134,26 @@ void test_list_remove(void **state)
 	free(c1);
 }
 
-void test_list_append(void **state)
+static void test_list_remove_head(void **state)
+{
+	struct list_node head = {};
+	expect_assert_failure(list_remove(&head));
+}
+
+static void test_list_append(void **state)
 {
 	size_t idx;
 	struct test_container *node;
-	struct list_node root = {};
+	struct list_node head = {};
 	struct test_container nodes[] = {
 		{1}, {2}, {3}
 	};
 
 	for (idx = 0; idx < ARRAY_SIZE(nodes); ++idx)
-		list_append(&nodes[idx].list_node, &root);
+		list_append(&nodes[idx].list_node, &head);
 
 	idx = 0;
-	list_for_each(node, root, list_node) {
+	list_for_each(node, head, list_node) {
 		assert_ptr_equal(node, &nodes[idx]);
 		idx++;
 	}
@@ -138,12 +162,14 @@ void test_list_append(void **state)
 int main(void)
 {
 	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_list_empty),
 		cmocka_unit_test(test_list_insert_after),
 		cmocka_unit_test(test_list_insert_before),
+		cmocka_unit_test(test_list_insert_before_head),
 		cmocka_unit_test(test_list_remove),
+		cmocka_unit_test(test_list_remove_head),
 		cmocka_unit_test(test_list_append),
 	};
 
-
 	return cb_run_group_tests(tests, NULL, NULL);
 }