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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: David Howells <[email protected]>
Cc: Ricardo Martinez <[email protected]>
---
include/linux/list.h | 54 ++++++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 27 deletions(-)
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
--
2.25.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.
--
With Best Regards,
Andy Shevchenko
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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
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.
--
With Best Regards,
Andy Shevchenko
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
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com