Message ID | 20230504012914.1797355-4-mathieu.desnoyers@efficios.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1736343vqo; Wed, 3 May 2023 18:44:45 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7C/DgnslIDhkmBwNQjo8Z6MrDZ7dtta/wq4VMv+Bx2o+JKhN1KWBu1TWiM35S7Vx2YaMRT X-Received: by 2002:a17:903:2352:b0:1ab:109e:a553 with SMTP id c18-20020a170903235200b001ab109ea553mr2368592plh.62.1683164685078; Wed, 03 May 2023 18:44:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1683164685; cv=none; d=google.com; s=arc-20160816; b=sc2H3LLqNff+fKo5ZwYjN8zxOXDwuUJTYG2NDl2hIrW6amy2yJM6JA4GwystG0S14K 3ESjhURglDMJlsyM4zO99d75n2RS3DvylcU3yRrH/PuAoMrRr3ZdgMepTsuhZTyd2BAv RCseI9Xei/USaEc1Lg0eC9C689gCsj7Ybo0O1WzC0CPrSGOsLAqUZobD+wLUPtiXChCF ZSSl2cqMQGjFjb1dQLQjKrwAnzz4ZKvfOlw3pJsPguxxvepXzP+cLLYap4EyJ3zHUR8t +Kq6XeqCfob4L/xet12zV/uh2ry7+c/yoBmaqOYpWeH35Okjltl2Wkb1mHioL2nPwKen CqXQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=mnR4vQgg+RwzAFnIUVvJaE+kou6tSnm7Sdnxo2/8WVs=; b=e2f9hBSXBPmGkCnkqjzWd4bkSUkzWKZjtF1CNjc3R04EOeWaFv5yy1M5TtX2/t6X8T OtyWfVEuNbYeVoBSUYiTzTnNpfGCc3zRObZw+GS3NtweEl3e1t/nEiVWMhs4eXBexw0p Z80RSZZbkxpaA6+xGymzz1RWa5Fh28N2UPVfAUg7Qlbqzlxu4o5r5ixjUOqjfpfwX3q6 uVhSHsznFJwoZPXvCHAWIKtBGTao/xZdZXs+NU5JBu06qhlCcVxSAitHK/OBwDI7gb5h 07CxvmlQjcsclso0yigisAGzcyU50sY/jrcxND26XP6KEI93ZHMs91aQ6zKtF2w5uL5o 8N3Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@efficios.com header.s=smtpout1 header.b=xuygN5Uk; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j9-20020a170903024900b001a63ba2c894si36268225plh.548.2023.05.03.18.44.20; Wed, 03 May 2023 18:44:45 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@efficios.com header.s=smtpout1 header.b=xuygN5Uk; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=efficios.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229761AbjEDB3g (ORCPT <rfc822;lhua1029@gmail.com> + 99 others); Wed, 3 May 2023 21:29:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39686 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229660AbjEDB30 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 3 May 2023 21:29:26 -0400 Received: from smtpout.efficios.com (smtpout.efficios.com [167.114.26.122]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BADD510F4 for <linux-kernel@vger.kernel.org>; Wed, 3 May 2023 18:29:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=efficios.com; s=smtpout1; t=1683163763; bh=ChUVnmTcKYnskiJX03Msq+VaqE0BCYSG0uVoH9Ivm5k=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=xuygN5Uk+Hvtln/jqZyQY5taMTYykXzZu7+wCO+uCyRbm0ZIuMmTTsY20OVKDcLlr 7fbMY8KxL3WB1WB5t0XoAP1KqH8RHZ5SxcGGhliMmFNn3kNi9FhgbblkCKmHU3pJRC Gm/5Uwj5Jquo7VWNULpF5BoBXA0A87/18pKeg19+eujMsfwNCA0/1L1ftETPKCAz/D rcUu3Sx3wyV2j7molHKbwRfMP1RL9XksCsg9vCtwDNUzkZAfFvnMcUeZTKP4bbTIEa IoaMbjSuW/wHXNbJy523mfiqu4kky5EJ20UvlzlBgj/i1y/NMQ5zV0Y9OTymCeO6oV EFlybCFs2SS+A== Received: from localhost.localdomain (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) by smtpout.efficios.com (Postfix) with ESMTPSA id 4QBbnv61Kbz11fg; Wed, 3 May 2023 21:29:23 -0400 (EDT) From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: linux-kernel@vger.kernel.org, Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Huang Ying <ying.huang@intel.com>, Peter Zijlstra <peterz@infradead.org> Subject: [RFC PATCH 4/4] llist.h: Fix parentheses around macro pointer parameter use Date: Wed, 3 May 2023 21:29:14 -0400 Message-Id: <20230504012914.1797355-4-mathieu.desnoyers@efficios.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230504012914.1797355-1-mathieu.desnoyers@efficios.com> References: <20230504012914.1797355-1-mathieu.desnoyers@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1764926092550666915?= X-GMAIL-MSGID: =?utf-8?q?1764926092550666915?= |
Series |
[RFC,1/4] rcu: rcupdate.h: Add missing parentheses around macro pointer dereference
|
|
Commit Message
Mathieu Desnoyers
May 4, 2023, 1:29 a.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
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(-)
Comments
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
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) /**