diff mbox series

[RFC,05/13] list.h: Fix parentheses around macro pointer parameter use

Message ID 20230504200527.1935944-6-mathieu.desnoyers@efficios.com
State New
Headers
Series Fix parentheses around macro parameter use in headers |

Commit Message

Mathieu Desnoyers May 4, 2023, 8:05 p.m. UTC 
  Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member
- "x = y" is changed for "x = (y)", because "y" can be an expression
  containing a comma if it is the result of the expansion of a macro such
  as #define eval(...) __VA_ARGS__, which would cause unexpected operator
  precedence. This use-case is far-fetched, but we have to choose one
  way or the other (with or without parentheses) for consistency,
- x && y is changed for (x) && (y).

Remove useless parentheses around use of macro parameter (head) in the
following pattern:

- list_is_head(pos, (head))

Because comma is the lowest priority operator already, so the extra pair
of parentheses is redundant.

This corrects the following usage pattern where operator precedence is
unexpected:

  LIST_HEAD(testlist);

  struct test {
          struct list_head node;
          int a;
  };

  // pos->member issue
  void f(void)
  {
          struct test *t1;
          struct test **t2 = &t1;

          list_for_each_entry((*t2), &testlist, node) {   /* works */
                  //...
          }
          list_for_each_entry(*t2, &testlist, node) {     /* broken */
                  //...
          }
  }

  // typeof(*pos) issue
  void f2(void)
  {
          struct test *t1 = NULL, *t2;

          t2 = list_prepare_entry((0 + t1), &testlist, node);     /* works */
          t2 = list_prepare_entry(0 + t1, &testlist, node);       /* broken */
  }

Note that the macros in which "pos" is also used as an lvalue probably
don't suffer from the lack of parentheses around "pos" in typeof(*pos),
but add those nevertheless to keep everything consistent.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Ricardo Martinez <ricardo.martinez@linux.intel.com>
---
 include/linux/list.h | 54 ++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

Comments

Andy Shevchenko May 8, 2023, 12:16 p.m. UTC | #1 
On Thu, May 04, 2023 at 04:05:19PM -0400, Mathieu Desnoyers wrote:
> Add missing parentheses around use of macro argument "pos" in those
> patterns to ensure operator precedence behaves as expected:
> 
> - typeof(*pos)
> - pos->member
> - "x = y" is changed for "x = (y)", because "y" can be an expression
>   containing a comma if it is the result of the expansion of a macro such
>   as #define eval(...) __VA_ARGS__, which would cause unexpected operator
>   precedence. This use-case is far-fetched, but we have to choose one
>   way or the other (with or without parentheses) for consistency,
> - x && y is changed for (x) && (y).
> 
> Remove useless parentheses around use of macro parameter (head) in the
> following pattern:
> 
> - list_is_head(pos, (head))
> 
> Because comma is the lowest priority operator already, so the extra pair
> of parentheses is redundant.

But strictly speaking it might be something like

	list_...(..., (a, b))

where (a, b) is the head. No?

> This corrects the following usage pattern where operator precedence is
> unexpected:
> 
>   LIST_HEAD(testlist);
> 
>   struct test {
>           struct list_head node;
>           int a;
>   };
> 
>   // pos->member issue
>   void f(void)
>   {
>           struct test *t1;
>           struct test **t2 = &t1;
> 
>           list_for_each_entry((*t2), &testlist, node) {   /* works */
>                   //...
>           }
>           list_for_each_entry(*t2, &testlist, node) {     /* broken */
>                   //...

Me still in doubt. But it's up to maintainers.

>           }
>   }
> 
>   // typeof(*pos) issue
>   void f2(void)
>   {
>           struct test *t1 = NULL, *t2;
> 
>           t2 = list_prepare_entry((0 + t1), &testlist, node);     /* works */
>           t2 = list_prepare_entry(0 + t1, &testlist, node);       /* broken */
>   }
> 
> Note that the macros in which "pos" is also used as an lvalue probably
> don't suffer from the lack of parentheses around "pos" in typeof(*pos),
> but add those nevertheless to keep everything consistent.
Mathieu Desnoyers May 8, 2023, 1:46 p.m. UTC | #2 
On 2023-05-08 08:16, Andy Shevchenko wrote:
> On Thu, May 04, 2023 at 04:05:19PM -0400, Mathieu Desnoyers wrote:
>> Add missing parentheses around use of macro argument "pos" in those
>> patterns to ensure operator precedence behaves as expected:
>>
>> - typeof(*pos)
>> - pos->member
>> - "x = y" is changed for "x = (y)", because "y" can be an expression
>>    containing a comma if it is the result of the expansion of a macro such
>>    as #define eval(...) __VA_ARGS__, which would cause unexpected operator
>>    precedence. This use-case is far-fetched, but we have to choose one
>>    way or the other (with or without parentheses) for consistency,
>> - x && y is changed for (x) && (y).
>>
>> Remove useless parentheses around use of macro parameter (head) in the
>> following pattern:
>>
>> - list_is_head(pos, (head))
>>
>> Because comma is the lowest priority operator already, so the extra pair
>> of parentheses is redundant.
> 
> But strictly speaking it might be something like
> 
> 	list_...(..., (a, b))
> 
> where (a, b) is the head. No?

The following case still works after removing the extra parentheses 
around "head" because the parentheses are present where the macro is used:

LIST_HEAD(testlist);

int f2(void)
{
         return 1;
}

void f(void)
{
    struct list_head *pos;

    list_for_each(pos, (f2(), &testlist)) {
            //...
    }
}

The only use I found that would break is as follows:

LIST_HEAD(testlist);

int f2(void)
{
         return 1;
}

#define eval(...)       __VA_ARGS__

void f(void)
{
    struct list_head *pos;

    list_for_each(pos, eval(f2(), &testlist)) {
            //...
    }
}

Because "eval()" will evaluate "f(), &testlist" with comma and all, 
without enclosing parentheses.

So the question is: do we want to support this kind-of-odd macro 
evaluation, considering that it requires adding parentheses around 
pretty much all macro parameters when used as expressions between commas?

Thanks,

Mathieu
Andy Shevchenko May 12, 2023, 11:02 a.m. UTC | #3 
On Mon, May 08, 2023 at 09:46:40AM -0400, Mathieu Desnoyers wrote:
> On 2023-05-08 08:16, Andy Shevchenko wrote:

...

> The only use I found that would break is as follows:
> 
> LIST_HEAD(testlist);
> 
> int f2(void)
> {
>         return 1;
> }
> 
> #define eval(...)       __VA_ARGS__
> 
> void f(void)
> {
>    struct list_head *pos;
> 
>    list_for_each(pos, eval(f2(), &testlist)) {
>            //...
>    }
> }
> 
> Because "eval()" will evaluate "f(), &testlist" with comma and all, without
> enclosing parentheses.
> 
> So the question is: do we want to support this kind-of-odd macro evaluation,
> considering that it requires adding parentheses around pretty much all macro
> parameters when used as expressions between commas?

Similar question can be asked for your initial motivation to support indirect
pointers. I found the double pointer as weird as this macro case. But it can be
only me. Hence I left this to the more experienced developers to express their
opinions.
Mathieu Desnoyers May 12, 2023, 2:32 p.m. UTC | #4 
On 2023-05-12 07:02, Andy Shevchenko wrote:
> On Mon, May 08, 2023 at 09:46:40AM -0400, Mathieu Desnoyers wrote:
>> On 2023-05-08 08:16, Andy Shevchenko wrote:
> 
> ...
> 
>> The only use I found that would break is as follows:
>>
>> LIST_HEAD(testlist);
>>
>> int f2(void)
>> {
>>          return 1;
>> }
>>
>> #define eval(...)       __VA_ARGS__
>>
>> void f(void)
>> {
>>     struct list_head *pos;
>>
>>     list_for_each(pos, eval(f2(), &testlist)) {
>>             //...
>>     }
>> }
>>
>> Because "eval()" will evaluate "f(), &testlist" with comma and all, without
>> enclosing parentheses.
>>
>> So the question is: do we want to support this kind-of-odd macro evaluation,
>> considering that it requires adding parentheses around pretty much all macro
>> parameters when used as expressions between commas?
> 
> Similar question can be asked for your initial motivation to support indirect
> pointers. I found the double pointer as weird as this macro case. But it can be
> only me. Hence I left this to the more experienced developers to express their
> opinions.
> 

The main motivation behind my changes is to make macro code consistent, 
and to eliminate classes of issues that can arise from unexpected 
operator precedence around use of macro arguments that lack parentheses.

The examples I provide in the commit messages are just instances showing 
how the lack of parentheses can lead to unexpected effects due to 
operator precedence.

My end goal is not to "support" specific use-cases. My goal is to 
eliminate inconsistency, increase robustness of the kernel macros, and 
lessen the cognitive burden that comes with using and maintaining those 
macros.

I hope that we can spend less time figuring out operator precedence 
corner-cases and more brain power thinking about and documenting things 
that really matter like memory barriers and synchronization.

Thanks,

Mathieu
diff mbox series

Patch

diff --git a/include/linux/list.h b/include/linux/list.h
index f10344dbad4d..d197b1a6f411 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -603,7 +603,7 @@  static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each(pos, head) \
-	for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
+	for (pos = (head)->next; !list_is_head(pos, head); pos = (pos)->next)
 
 /**
  * list_for_each_rcu - Iterate over a list in an RCU-safe fashion
@@ -612,8 +612,8 @@  static inline void list_splice_tail_init(struct list_head *list,
  */
 #define list_for_each_rcu(pos, head)		  \
 	for (pos = rcu_dereference((head)->next); \
-	     !list_is_head(pos, (head)); \
-	     pos = rcu_dereference(pos->next))
+	     !list_is_head(pos, head); \
+	     pos = rcu_dereference((pos)->next))
 
 /**
  * list_for_each_continue - continue iteration over a list
@@ -623,7 +623,7 @@  static inline void list_splice_tail_init(struct list_head *list,
  * Continue to iterate over a list, continuing after the current position.
  */
 #define list_for_each_continue(pos, head) \
-	for (pos = pos->next; !list_is_head(pos, (head)); pos = pos->next)
+	for (pos = (pos)->next; !list_is_head(pos, head); pos = (pos)->next)
 
 /**
  * list_for_each_prev	-	iterate over a list backwards
@@ -631,7 +631,7 @@  static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_prev(pos, head) \
-	for (pos = (head)->prev; !list_is_head(pos, (head)); pos = pos->prev)
+	for (pos = (head)->prev; !list_is_head(pos, head); pos = (pos)->prev)
 
 /**
  * list_for_each_safe - iterate over a list safe against removal of list entry
@@ -640,9 +640,9 @@  static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_safe(pos, n, head) \
-	for (pos = (head)->next, n = pos->next; \
-	     !list_is_head(pos, (head)); \
-	     pos = n, n = pos->next)
+	for (pos = (head)->next, n = (pos)->next; \
+	     !list_is_head(pos, head); \
+	     pos = (n), n = (pos)->next)
 
 /**
  * list_for_each_prev_safe - iterate over a list backwards safe against removal of list entry
@@ -651,9 +651,9 @@  static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  */
 #define list_for_each_prev_safe(pos, n, head) \
-	for (pos = (head)->prev, n = pos->prev; \
-	     !list_is_head(pos, (head)); \
-	     pos = n, n = pos->prev)
+	for (pos = (head)->prev, n = (pos)->prev; \
+	     !list_is_head(pos, head); \
+	     pos = (n), n = (pos)->prev)
 
 /**
  * list_count_nodes - count nodes in the list
@@ -677,7 +677,7 @@  static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_entry_is_head(pos, head, member)				\
-	(&pos->member == (head))
+	(&(pos)->member == (head))
 
 /**
  * list_for_each_entry	-	iterate over list of given type
@@ -686,7 +686,7 @@  static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry(pos, head, member)				\
-	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	for (pos = list_first_entry(head, typeof(*(pos)), member);	\
 	     !list_entry_is_head(pos, head, member);			\
 	     pos = list_next_entry(pos, member))
 
@@ -697,7 +697,7 @@  static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry_reverse(pos, head, member)			\
-	for (pos = list_last_entry(head, typeof(*pos), member);		\
+	for (pos = list_last_entry(head, typeof(*(pos)), member);	\
 	     !list_entry_is_head(pos, head, member); 			\
 	     pos = list_prev_entry(pos, member))
 
@@ -710,7 +710,7 @@  static inline size_t list_count_nodes(struct list_head *head)
  * Prepares a pos entry for use as a start point in list_for_each_entry_continue().
  */
 #define list_prepare_entry(pos, head, member) \
-	((pos) ? : list_entry(head, typeof(*pos), member))
+	((pos) ? : list_entry(head, typeof(*(pos)), member))
 
 /**
  * list_for_each_entry_continue - continue iteration over list of given type
@@ -773,10 +773,10 @@  static inline size_t list_count_nodes(struct list_head *head)
  * @member:	the name of the list_head within the struct.
  */
 #define list_for_each_entry_safe(pos, n, head, member)			\
-	for (pos = list_first_entry(head, typeof(*pos), member),	\
+	for (pos = list_first_entry(head, typeof(*(pos)), member),	\
 		n = list_next_entry(pos, member);			\
 	     !list_entry_is_head(pos, head, member); 			\
-	     pos = n, n = list_next_entry(n, member))
+	     pos = (n), n = list_next_entry(n, member))
 
 /**
  * list_for_each_entry_safe_continue - continue list iteration safe against removal
@@ -792,7 +792,7 @@  static inline size_t list_count_nodes(struct list_head *head)
 	for (pos = list_next_entry(pos, member), 				\
 		n = list_next_entry(pos, member);				\
 	     !list_entry_is_head(pos, head, member);				\
-	     pos = n, n = list_next_entry(n, member))
+	     pos = (n), n = list_next_entry(n, member))
 
 /**
  * list_for_each_entry_safe_from - iterate over list from current point safe against removal
@@ -807,7 +807,7 @@  static inline size_t list_count_nodes(struct list_head *head)
 #define list_for_each_entry_safe_from(pos, n, head, member) 			\
 	for (n = list_next_entry(pos, member);					\
 	     !list_entry_is_head(pos, head, member);				\
-	     pos = n, n = list_next_entry(n, member))
+	     pos = (n), n = list_next_entry(n, member))
 
 /**
  * list_for_each_entry_safe_reverse - iterate backwards over list safe against removal
@@ -820,10 +820,10 @@  static inline size_t list_count_nodes(struct list_head *head)
  * of list entry.
  */
 #define list_for_each_entry_safe_reverse(pos, n, head, member)		\
-	for (pos = list_last_entry(head, typeof(*pos), member),		\
+	for (pos = list_last_entry(head, typeof(*(pos)), member),	\
 		n = list_prev_entry(pos, member);			\
 	     !list_entry_is_head(pos, head, member); 			\
-	     pos = n, n = list_prev_entry(n, member))
+	     pos = (n), n = list_prev_entry(n, member))
 
 /**
  * list_safe_reset_next - reset a stale list_for_each_entry_safe loop
@@ -1033,11 +1033,11 @@  static inline void hlist_move_list(struct hlist_head *old,
 #define hlist_entry(ptr, type, member) container_of(ptr,type,member)
 
 #define hlist_for_each(pos, head) \
-	for (pos = (head)->first; pos ; pos = pos->next)
+	for (pos = (head)->first; pos ; pos = (pos)->next)
 
 #define hlist_for_each_safe(pos, n, head) \
-	for (pos = (head)->first; pos && ({ n = pos->next; 1; }); \
-	     pos = n)
+	for (pos = (head)->first; (pos) && ({ n = (pos)->next; 1; }); \
+	     pos = (n))
 
 #define hlist_entry_safe(ptr, type, member) \
 	({ typeof(ptr) ____ptr = (ptr); \
@@ -1082,8 +1082,8 @@  static inline void hlist_move_list(struct hlist_head *old,
  * @member:	the name of the hlist_node within the struct.
  */
 #define hlist_for_each_entry_safe(pos, n, head, member) 		\
-	for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
-	     pos && ({ n = pos->member.next; 1; });			\
-	     pos = hlist_entry_safe(n, typeof(*pos), member))
+	for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
+	     (pos) && ({ n = (pos)->member.next; 1; });			\
+	     pos = hlist_entry_safe(n, typeof(*(pos)), member))
 
 #endif