| Message ID | alpine.LNX.2.00.1106260009530.23991@swampdragon.chaosbits.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 25 Jun 2011, David Miller wrote: > From: Jesper Juhl <jj@chaosbits.net> > Date: Sun, 26 Jun 2011 00:11:12 +0200 (CEST) > > > Introduce a sequence point (;) between two writes to llc_sk(sk)->vR in > > net/llc/llc_c_ac.c:llc_conn_ac_inc_vr_by_1() so that the order in > > which the writes happen become well defined. > > > > While the code may work fine now it may break at any time with a > > different compiler, a new version of current compiler or even just a > > different optimization level of the current compiler. Much better to > > clearly express what's intended in a way that guarantees the result. > > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > > How about fixing the macro so that it doesn't have side effects > like this? > > That's much better than a 6 line (improperly formatted, BTW) comment > every time someone tried to use that macros with an lvalue that isn't > a local variable. Sure, that's also a way to go - better probably. I just thought that I would fix up the one call site I found that was problematic... But gimme a couple of days (I don't have much free time) and I'll cook up a different patch.
From: Jesper Juhl <jj@chaosbits.net> Date: Sun, 26 Jun 2011 00:11:12 +0200 (CEST) > Introduce a sequence point (;) between two writes to llc_sk(sk)->vR in > net/llc/llc_c_ac.c:llc_conn_ac_inc_vr_by_1() so that the order in > which the writes happen become well defined. > > While the code may work fine now it may break at any time with a > different compiler, a new version of current compiler or even just a > different optimization level of the current compiler. Much better to > clearly express what's intended in a way that guarantees the result. > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> How about fixing the macro so that it doesn't have side effects like this? That's much better than a 6 line (improperly formatted, BTW) comment every time someone tried to use that macros with an lvalue that isn't a local variable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
different compiler, a new version of current compiler or even just a different optimization level of the current compiler. Much better to clearly express what's intended in a way that guarantees the result. Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- net/llc/llc_c_ac.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/net/llc/llc_c_ac.c b/net/llc/llc_c_ac.c index ea225bd..e535ca4 100644 --- a/net/llc/llc_c_ac.c +++ b/net/llc/llc_c_ac.c @@ -1296,7 +1296,14 @@ int llc_conn_ac_set_vr_0(struct sock *sk, struct sk_buff *skb) int llc_conn_ac_inc_vr_by_1(struct sock *sk, struct sk_buff *skb) { - llc_sk(sk)->vR = PDU_GET_NEXT_Vr(llc_sk(sk)->vR); + /* Do not consolidate this on one line. Since the PDU_GET_NEXT_Vr + macro increments its argument which is the same as what we are + writing to, then we'll have two writes to the same variable + without an intervening sequence point, which leads to the + situation where we can't really know what gets stored as the + result since the compiler is free to do those in any order. */ + const u8 new_vr = PDU_GET_NEXT_Vr(llc_sk(sk)->vR); + llc_sk(sk)->vR = new_vr; return 0; }