[RFC,4/4] llist.h: Fix parentheses around macro pointer parameter use

  Add missing parentheses around use of macro argument "pos" in those
patterns to ensure operator precedence behaves as expected:

- typeof(*pos)
- pos->member

The typeof(*pos) lack of parentheses around "pos" is not an issue per se
in the specific macros modified here because "pos" is used as an lvalue,
which should prevent use of any operator causing issue. Still add the
extra parentheses for consistency.

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

  llist_entry((node), typeof(*pos), member)

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

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/llist.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Hi, Mathieu,

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> Add missing parentheses around use of macro argument "pos" in those
> patterns to ensure operator precedence behaves as expected:
>
> - typeof(*pos)
> - pos->member
>
> The typeof(*pos) lack of parentheses around "pos" is not an issue per se
> in the specific macros modified here because "pos" is used as an lvalue,
> which should prevent use of any operator causing issue. Still add the
> extra parentheses for consistency.

I don't think it's necessary to add parentheses here.  As you said,
"pos" is used as an lvalue.

> Remove useless parentheses around use of macro parameter (node) in the
> following pattern:
>
>   llist_entry((node), typeof(*pos), member)
>
> Because comma is the lowest priority operator already, so the extra pair
> of parentheses is redundant.

This change looks good for me.

Best Regards,
Huang, Ying

> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  include/linux/llist.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/llist.h b/include/linux/llist.h
> index 85bda2d02d65..45d358c15d0d 100644
> --- a/include/linux/llist.h
> +++ b/include/linux/llist.h
> @@ -173,9 +173,9 @@ static inline void init_llist_head(struct llist_head *list)
>   * reverse the order by yourself before traversing.
>   */
>  #define llist_for_each_entry_safe(pos, n, node, member)			       \
> -	for (pos = llist_entry((node), typeof(*pos), member);		       \
> +	for (pos = llist_entry(node, typeof(*(pos)), member);		       \
>  	     member_address_is_nonnull(pos, member) &&			       \
> -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
> +		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
>  	     pos = n)
>  
>  /**
  

On 2023-05-04 01:54, Huang, Ying wrote:
> Hi, Mathieu,
> 
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:
> 
>> Add missing parentheses around use of macro argument "pos" in those
>> patterns to ensure operator precedence behaves as expected:
>>
>> - typeof(*pos)
>> - pos->member
>>
>> The typeof(*pos) lack of parentheses around "pos" is not an issue per se
>> in the specific macros modified here because "pos" is used as an lvalue,
>> which should prevent use of any operator causing issue. Still add the
>> extra parentheses for consistency.
> 
> I don't think it's necessary to add parentheses here.  As you said,
> "pos" is used as an lvalue.

I agree that it's not strictly necessary to add the parentheses around
"pos" in typeof(*pos) when pos is also used as an lvalue within a macro,
but if we look at what happened in list.h, we can see why having a consistent
pattern is good to eliminate issues as the code evolves.

When code from list_for_each_entry_continue was lifted into
list_prepare_entry(), we had a situation where "pos" was initially used
as an lvalue in the original macro, but not in list_prepare_entry(), for
which the parentheses were relevant.

This example is from the pre-git era, in tglx's history tree:

commit a3500b9e955d47891e57587c30006de83a3591f5
Author: Linus Torvalds <torvalds@home.osdl.org>
Date:   Wed Feb 11 21:00:34 2004 -0800

     Fix "bus_for_each_dev()" and "bus_for_each_drv()", which did not
     correctly handle the "restart from this device/driver" case, and
     caused oopses with ieee1394.
     
     This just uses "list_for_each_entry_continue()" instead.
     
     Add helper macro to make usage of "list_for_each_entry_continue()"
     a bit more readable.

[...]

+/**
+ * list_prepare_entry - prepare a pos entry for use as a start point in
+ *                     list_for_each_entry_continue
+ * @pos:       the type * to use as a start point
+ * @head:      the head of the list
+ * @member:    the name of the list_struct within the struct.
+ */
+#define list_prepare_entry(pos, head, member) \
+       ((pos) ? : list_entry(head, typeof(*pos), member))

So even though the fact that "pos" is used as an lvalue specifically in
llist_for_each_entry_safe() makes it so that the parentheses are not
strictly required around "pos" in typeof(*pos), I argue that we should
still add those for consistency.

> 
>> Remove useless parentheses around use of macro parameter (node) in the
>> following pattern:
>>
>>    llist_entry((node), typeof(*pos), member)
>>
>> Because comma is the lowest priority operator already, so the extra pair
>> of parentheses is redundant.
> 
> This change looks good for me.

Thanks,

Mathieu

> 
> Best Regards,
> Huang, Ying
> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Huang Ying <ying.huang@intel.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> ---
>>   include/linux/llist.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/llist.h b/include/linux/llist.h
>> index 85bda2d02d65..45d358c15d0d 100644
>> --- a/include/linux/llist.h
>> +++ b/include/linux/llist.h
>> @@ -173,9 +173,9 @@ static inline void init_llist_head(struct llist_head *list)
>>    * reverse the order by yourself before traversing.
>>    */
>>   #define llist_for_each_entry_safe(pos, n, node, member)			       \
>> -	for (pos = llist_entry((node), typeof(*pos), member);		       \
>> +	for (pos = llist_entry(node, typeof(*(pos)), member);		       \
>>   	     member_address_is_nonnull(pos, member) &&			       \
>> -	        (n = llist_entry(pos->member.next, typeof(*n), member), true); \
>> +		(n = llist_entry((pos)->member.next, typeof(*(n)), member), true); \
>>   	     pos = n)
>>   
>>   /**
  

On Thu, May 4, 2023 at 7:54 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> +#define list_prepare_entry(pos, head, member) \
> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>
> So even though the fact that "pos" is used as an lvalue specifically in
> llist_for_each_entry_safe() makes it so that the parentheses are not
> strictly required around "pos" in typeof(*pos), I argue that we should
> still add those for consistency.

Ack. It may not matter in reality because of how 'pos' is supposed to
be just a local variable name, but I agree that for consistency our
macros should still follow the usual pattern.

Of course, *because* of how 'pos' is not some random expression, and
is supposed to be that local variable, and because of how it is used,
it must always violate the whole "only use once" usual pattern,.

Exactly the same way the member name is also typically used multiple
times because of how it's not an expression, but really a "part of the
syntax".

So an alternative might be that we should use a different syntax for
those things and make it clear that they are not normal expressions.
Some people use upper-case names for special things like that to make
them stand out as "not normal" (kind of the same way upper-case macros
themselves were a warning that they weren't normal and might evaluate
arguments multiple times).

                   Linus
  

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, May 4, 2023 at 7:54 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> +#define list_prepare_entry(pos, head, member) \
>> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>>
>> So even though the fact that "pos" is used as an lvalue specifically in
>> llist_for_each_entry_safe() makes it so that the parentheses are not
>> strictly required around "pos" in typeof(*pos), I argue that we should
>> still add those for consistency.
>
> Ack. It may not matter in reality because of how 'pos' is supposed to
> be just a local variable name, but I agree that for consistency our
> macros should still follow the usual pattern.
>
> Of course, *because* of how 'pos' is not some random expression, and
> is supposed to be that local variable, and because of how it is used,
> it must always violate the whole "only use once" usual pattern,.
>
> Exactly the same way the member name is also typically used multiple
> times because of how it's not an expression, but really a "part of the
> syntax".
>
> So an alternative might be that we should use a different syntax for
> those things and make it clear that they are not normal expressions.
> Some people use upper-case names for special things like that to make
> them stand out as "not normal" (kind of the same way upper-case macros
> themselves were a warning that they weren't normal and might evaluate
> arguments multiple times).

This sounds a good idea for me.

And with this, I think that we will use typeof(*POS) instead of
typeof(*(pos))?

Best Regards,
Huang, Ying
  

On 2023-05-04 13:16, Linus Torvalds wrote:
> On Thu, May 4, 2023 at 7:54 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> +#define list_prepare_entry(pos, head, member) \
>> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>>
>> So even though the fact that "pos" is used as an lvalue specifically in
>> llist_for_each_entry_safe() makes it so that the parentheses are not
>> strictly required around "pos" in typeof(*pos), I argue that we should
>> still add those for consistency.
> 
> Ack. It may not matter in reality because of how 'pos' is supposed to
> be just a local variable name, but I agree that for consistency our
> macros should still follow the usual pattern.
> 
> Of course, *because* of how 'pos' is not some random expression, and
> is supposed to be that local variable, and because of how it is used,
> it must always violate the whole "only use once" usual pattern,.
> 
> Exactly the same way the member name is also typically used multiple
> times because of how it's not an expression, but really a "part of the
> syntax".
> 
> So an alternative might be that we should use a different syntax for
> those things and make it clear that they are not normal expressions.
> Some people use upper-case names for special things like that to make
> them stand out as "not normal" (kind of the same way upper-case macros
> themselves were a warning that they weren't normal and might evaluate
> arguments multiple times).

Is a list iteration position absolutely required to be a local variable,
or can it be a dereferenced pointer ?

Let's say we take "list_for_each()" as example:

#define list_for_each(pos, head) \
         for (pos = (head)->next; !list_is_head(pos, head); pos = pos->next)

and turn "pos" into "POS" to make it clearer that it is used as an lvalue:

#define list_for_each(POS, head) \
         for (POS = (head)->next; !list_is_head(POS, head); pos = POS->next)

The following usage pattern is still broken:

struct list_head list;

void f(struct list_head **ppos)
{
   list_for_each(*ppos, &list) {
     //...
   }
}

Because ->next has higher operator precedence than "*", which is unexpected.

I would argue that even if we choose to capitalize the macro special arguments used
as lvalues and as member names so they stand out, it does not eliminate the need to
be careful about proper use of parentheses around those parameters when they are also
used as rvalues.

Thanks,

Mathieu
  

On Fri, May 5, 2023 at 7:23 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Is a list iteration position absolutely required to be a local variable,
> or can it be a dereferenced pointer ?

Well, it was certainly the intention, but while the member name
obviously has to be a member name, the iterator *could* be any lvalue.

Many of the "foreach" kind of loops would actually prefer to have
entirely local variables, but C syntax rules didn't allow that (now
with C11 we can do that variable declaration in the for-loop itself,
but we're still limited to just _one_ variable which can be a
problem).

So if we had started with C11, that 'list_for_each()' wouldn't have
had an external 'pos' declaration at all, it would have done it
internally, but that's not the reality we're in today ;/

              Linus
  

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> writes:

> On 2023-05-04 13:16, Linus Torvalds wrote:
>> On Thu, May 4, 2023 at 7:54/AM Mathieu Desnoyers
>> <mathieu.desnoyers@efficios.com> wrote:
>>> +#define list_prepare_entry(pos, head, member) \
>>> +       ((pos) ? : list_entry(head, typeof(*pos), member))
>>>
>>> So even though the fact that "pos" is used as an lvalue specifically in
>>> llist_for_each_entry_safe() makes it so that the parentheses are not
>>> strictly required around "pos" in typeof(*pos), I argue that we should
>>> still add those for consistency.
>> Ack. It may not matter in reality because of how 'pos' is supposed
>> to
>> be just a local variable name, but I agree that for consistency our
>> macros should still follow the usual pattern.
>> Of course, *because* of how 'pos' is not some random expression, and
>> is supposed to be that local variable, and because of how it is used,
>> it must always violate the whole "only use once" usual pattern,.
>> Exactly the same way the member name is also typically used multiple
>> times because of how it's not an expression, but really a "part of the
>> syntax".
>> So an alternative might be that we should use a different syntax for
>> those things and make it clear that they are not normal expressions.
>> Some people use upper-case names for special things like that to make
>> them stand out as "not normal" (kind of the same way upper-case macros
>> themselves were a warning that they weren't normal and might evaluate
>> arguments multiple times).
>
> Is a list iteration position absolutely required to be a local variable,
> or can it be a dereferenced pointer ?
>
> Let's say we take "list_for_each()" as example:
>
> #define list_for_each(pos, head) \
>         for (pos = (head)->next; !list_is_head(pos, head); pos = pos->next)
>
> and turn "pos" into "POS" to make it clearer that it is used as an lvalue:
>
> #define list_for_each(POS, head) \
>         for (POS = (head)->next; !list_is_head(POS, head); pos = POS->next)
>
> The following usage pattern is still broken:
>
> struct list_head list;
>
> void f(struct list_head **ppos)
> {
>   list_for_each(*ppos, &list) {
>     //...
>   }
> }
>
> Because ->next has higher operator precedence than "*", which is unexpected.
>
> I would argue that even if we choose to capitalize the macro special arguments used
> as lvalues and as member names so they stand out, it does not eliminate the need to
> be careful about proper use of parentheses around those parameters when they are also
> used as rvalues.

Yes.  The special parameter isn't necessary a variable name (except the
member name).  So special parameters

- are lvalue or member name
- may be "evaluated" multiple times

This makes them special enough from other macro parameters.  And we
still need to be careful about them (for example, when used as rvalue)
as other macro parameters.

Best Regards,
Huang, Ying