[lttng-dev] Problem in userspace-rcu cds_hlist_del_rcu.c caused by #define cds_hlist_for_each_entry_safe_2

[Mathieu Desnoyers]

----- Original Message -----
> From: "Daniel Thibault" <Daniel.Thibault at drdc-rddc.gc.ca>
> To: lttng-dev at lists.lttng.org
> Sent: Tuesday, April 29, 2014 8:59:36 PM
> Subject: [lttng-dev] Problem in userspace-rcu cds_hlist_del_rcu.c caused by #define cds_hlist_for_each_entry_safe_2
> 
> When compiling userspace-rcu (commit 81dd913), I get a pair of warnings for
> /userspace-rcu/doc/examples/hlist/cds_hlist_del_rcu.c at the
> cds_hlist_for_each_entry_safe_2 call:
> 
> 	cds_hlist_for_each_entry_safe_2(node, n, &mylist, node) {
> 		if (node->value > 0) {
> 			cds_hlist_del_rcu(&node->node);
> 			/*
> 			 * We can only reclaim memory after a grace
> 			 * period has passed after cds_hlist_del_rcu().
> 			 */
> 			call_rcu(&node->rcu_head, free_node_rcu);
> 		}
> 	}
> 
>    The warnings are:
> "attention: assignment makes pointer from integer without a cast"
> "attention: left operand of comma operator has no effect"
> 
>    The #define comes from userspace-rcu/urcu/hlist.h (here re-arranged a
>    bit):
> 
> #define cds_hlist_for_each_entry_safe_2(entry, e, head, member) \
> 	for (entry = ((head)->next == NULL ? NULL : cds_hlist_entry((head)->next,
> 	__typeof__(*entry), member)); \
> 	        (entry != NULL) && \
> 	        (e = (entry->member.next == NULL ? NULL :
> 	        cds_hlist_entry(entry->member.next, __typeof__(*entry), member),
> 	        1)); \
> 	        entry = e)
> 
>    The cds_hlist_del_rcu.c occurrence is the only use of
>    cds_hlist_for_each_entry_safe_2 and the problem seems to me to lie in
>    this bit:
> 
> (e = (entry->member.next == NULL ? NULL : cds_hlist_entry(entry->member.next,
> __typeof__(*entry), member), 1))
> 
>    Because the comma operator has the lowest precedence of all operators, the
>    statement is equivalent to:
> 
> (e = ((entry->member.next == NULL ? NULL :
> cds_hlist_entry(entry->member.next, __typeof__(*entry), member)), 1))
> 
>    Which is in turn the same as:
> 
> (e = 1)
> 
>    Now, if the macro is using the comma to make the second part of the &&
>    test succeed regardless of the value fetched for e, then it should have
>    been written:
> 
> #define cds_hlist_for_each_entry_safe_2(entry, e, head, member) \
> 	for (entry = ((head)->next == NULL ? NULL : cds_hlist_entry((head)->next,
> 	__typeof__(*entry), member)); \
> 	        (entry != NULL) && \
> 	        ((e = (entry->member.next == NULL ? NULL :
> 	        cds_hlist_entry(entry->member.next, __typeof__(*entry), member))),
> 	        1); \
> 	        entry = e)
> 
>    Which does compile without the warnings.

Hi Daniel,

Good catch ! For some reason my compiler did not complain about
it.

I propose to remove the extra ((e = ...), 1) from your proposal,
turning the modified segment of expression into:

(e = ..., 1)

since the comma operator has the lowest precedence, the extra
parenthesis are redundant.

Fortunately, this bug only affects the master branch, and not
stable branches.

Here is the fix commit info:

commit 79efd9b350e07a499b3620ddde4860b8378fcf00
Author: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
Date:   Tue Apr 29 22:01:57 2014 +0200

    Fix: incorrect parenthesis in cds_hlist_for_each_entry_safe_2

Thanks for pointing it out!

Mathieu


> 
> Daniel U. Thibault
> Protection des systèmes et contremesures (PSC) | Systems Protection &
> Countermeasures (SPC)
> Cyber sécurité pour les missions essentielles (CME) | Mission Critical Cyber
> Security (MCCS)
> R & D pour la défense Canada - Valcartier (RDDC Valcartier) | Defence R&D
> Canada - Valcartier (DRDC Valcartier)
> 2459 route de la Bravoure
> Québec QC  G3J 1X5
> CANADA
> Vox : (418) 844-4000 x4245
> Fax : (418) 844-4538
> NAC : 918V QSDJ <http://www.travelgis.com/map.asp?addr=918V%20QSDJ>
> Gouvernement du Canada | Government of Canada
> <http://www.valcartier.drdc-rddc.gc.ca/>
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com