list_head debugging?
PATCH 1/1 list_head debugging?
A friend recently was bitten by passing a list_head from list_for_each to a code path that later moved the list_head around. Does the attached patch re-create some debugging wheel that's hiding off in a corner somewhere?
Beyond catching the obvious use of uninitialized things, it also seems to catch double adds, double deletes, and simple deletes of 'pos' in list_for_each. it even seems to bark about the seemingly hard-to-detect movement of 'pos' from the iterating list to another, but probably only in the rare case where you're unconditionally moving all 'pos' in the loop body. (you eventually iterate into the second list's head and try to add it to itself)
I've only played with this in userspace but am glad to pretty it up with CONFIG_s and bring it into 2.5 if people care. its against 2.4.mumble.
Signature · 110 lines
zach
--- ./list.h.debug Thu Sep 19 15:58:47 2002
+++ ./list.h Fri Sep 20 13:43:21 2002
@@ -21,6 +21,25 @@
typedef struct list_head list_t;
+#define LIST_HEAD_DEBUGGING
+#ifdef LIST_HEAD_DEBUGGING
+
+static inline void __list_valid(struct list_head *list)
+{
+ BUG_ON(list == NULL);
+ BUG_ON(list->next == NULL);
+ BUG_ON(list->prev == NULL);
+ BUG_ON(list->next->prev != list);
+ BUG_ON(list->prev->next != list);
+ BUG_ON((list->next == list) && (list->prev != list));
+ BUG_ON((list->prev == list) && (list->next != list));
+}
+#else
+
+#define __list_valid(args...)
+
+#endif
+
#define LIST_HEAD_INIT(name) { &(name), &(name) }
#define LIST_HEAD(name) \
@@ -56,7 +75,9 @@
*/
static __inline__ void list_add(struct list_head *new, struct list_head *head)
{
+ __list_valid(head);
__list_add(new, head, head->next);
+ __list_valid(new);
}
/**
@@ -69,7 +90,9 @@
*/
static __inline__ void list_add_tail(struct list_head *new, struct list_head *head)
{
+ __list_valid(head);
__list_add(new, head->prev, head);
+ __list_valid(new);
}
/*
@@ -93,6 +116,7 @@
*/
static __inline__ void list_del(struct list_head *entry)
{
+ __list_valid(entry);
__list_del(entry->prev, entry->next);
}
@@ -102,6 +126,7 @@
*/
static __inline__ void list_del_init(struct list_head *entry)
{
+ __list_valid(entry);
__list_del(entry->prev, entry->next);
INIT_LIST_HEAD(entry);
}
@@ -112,6 +137,7 @@
*/
static __inline__ int list_empty(struct list_head *head)
{
+ __list_valid(head);
return head->next == head;
}
@@ -124,6 +150,9 @@
{
struct list_head *first = list->next;
+ __list_valid(list);
+ __list_valid(head);
+
if (first != list) {
struct list_head *last = list->prev;
struct list_head *at = head->next;
@@ -150,9 +179,10 @@
* @pos: the &struct list_head to use as a loop counter.
* @head: the head for your list.
*/
-#define list_for_each(pos, head) \
- for (pos = (head)->next, prefetch(pos->next); pos != (head); \
- pos = pos->next, prefetch(pos->next))
+#define list_for_each(pos, head) \
+ for (pos = (head)->next, prefetch(pos->next); \
+ __list_valid(pos), pos != (head); \
+ __list_valid(pos), pos = pos->next, prefetch(pos->next))
/**
* list_for_each_safe - iterate over a list safe against removal of list entry
@@ -170,8 +200,9 @@
* @head: the head for your list.
*/
#define list_for_each_prev(pos, head) \
- for (pos = (head)->prev, prefetch(pos->prev); pos != (head); \
- pos = pos->prev, prefetch(pos->prev))
+ for (pos = (head)->prev, prefetch(pos->prev); \
+ __list_valid(pos), pos != (head); \
+ __list_valid(pos), pos = pos->prev, prefetch(pos->prev))
#endif /* __KERNEL__ || _LVM_H_INCLUDE */Quoted 5 lines · Zach Brown
A friend recently was bitten by passing a
list_headfromlist_for_eachto a code path that later moved thelist_headaround. Does the attached patch re-create some debugging wheel that's hiding off in a corner somewhere?
Quoted 7 lines
Beyond catching the obvious use of uninitialized things, it also seems to catch double adds, double deletes, and simple deletes of 'pos' in
list_for_each. it even seems to bark about the seemingly hard-to-detect movement of 'pos' from the iterating list to another, but probably only in the rare case where you're unconditionally moving all 'pos' in the loop body. (you eventually iterate into the second list's head and try to add it to itself)
Hey, cool!
Yes, I think a lot of people would be all for something like this as a CONFIG_DEBUG_LISTS or such. Very nice.
Robert Love
Hi,
Before Fri, 20 Sep 2002, Zach Brown wrote:
list.h renamed from list.h.debug +14 -0
@@ -21,6 +21,25 @@typedef struct list_head list_t ; +#define LIST_HEAD_DEBUGGING +#ifdef LIST_HEAD_DEBUGGING + +static inline void __list_valid (struct list_head *list) +{ +BUG_ON (list ==NULL ); +BUG_ON (list->next ==NULL ); +BUG_ON (list->prev ==NULL ); +BUG_ON (list->next->prev != list); +BUG_ON (list->prev->next != list); +BUG_ON ((list->next == list) && (list->prev != list)); +BUG_ON ((list->prev == list) && (list->next != list)); +} +#else
It's all cool, but I'm not entirely convinced why it must be a BUG macro. I'd rather have something said via printk here. If whatever we did was bad, it will show up with a BUG() just too soon.
I'd describe a macro.
#define list_assert(cond) \
if (cond) printk(KERN_ERR "%s failed!\n", #cond)
Or the like. BTW, I'd define LIST_HEAD_DEBUGGING as 1.
Thunder
Signature · 1 line
assert(typeof((fool)->next) == typeof(fool)); /* wrong */
Em Fri, Sep 20, 2002 at 04:53:04PM -0400, Zach Brown escreveu:
Quoted 4 lines
A friend recently was bitten by passing a
list_headfromlist_for_eachto a code path that later moved thelist_headaround. Does the attached patch re-create some debugging wheel that's hiding off in a corner somewhere?
Cool! I'll be always using this while developing, thanks!
- Arnaldo