Bug 69924

Comment on attachment 86792 [details] [review]
fix patch

FALSE. In language C, lvalue = rexpr means lvalue calculated first, so nodes[i] = value_enerator (&i) is just work as expected.

This patch in fact doesn't change anything but a little more complicated. :-)

(In reply to comment #1)
> Comment on attachment 86792 [details] [review]
> fix patch
> 
> FALSE. In language C, lvalue = rexpr means lvalue calculated first, so
> nodes[i] = value_enerator (&i) is just work as expected.
> 
> This patch in fact doesn't change anything but a little more complicated. :-)

Hmm, Seems I was wrong, this will cause undefined behavior and your patch looks good because it inserts a sequence point between the two modifications. So this may work as the author's expectation in some compiler but fail in some others.

Comment on attachment 86792 [details] [review]
fix patch

looks good

This one is either MSVC-bug or undefined behavior:

The mingw and msvc give different results.


Test program:

#include <stdio.h>
int inc( int* i ){
	*i = *i +1;
	return *i;
}
int main(void){
	int array[2] = { -1 , -1 };
	int i        = 0;
	array[i] = inc(&i);
	printf("array[0] = %d\n",array[0]);
	printf("array[1] = %d\n",array[1]);
	return 0;
}



MSVC output (32 and 64 bit):
array[0] = -1
array[1] = 1

MinGW output:
array[0] = 1
array[1] = -1


So this patch can be treated as "compatibility" patch for removal of undefined behavior (because msvc bug?).

I think this is indeed undefined behaviour (read about "sequence points" for more details).

Comment on attachment 86792 [details] [review]
fix patch

Review of attachment 86792 [details] [review]:
-----------------------------------------------------------------

> test bug fix1

That's not a commit message. Please see, for instance, <http://robots.thoughtbot.com/5-useful-tips-for-a-better-commit-message>.

I'll change this to something more like this when I commit it:

> make_and_run_test_nodes: avoid undefined behaviour
>
> In code that looks like n[i] = v(&i), where v alters the
> value of i, C leaves it undefined whether the old or new
> value of i is used to locate n[i]. Fix this by inserting
> a sequence point to disambiguate the intended order.
>
> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=69924

::: dbus/dbus-marshal-recursive-util.c
@@ +1778,5 @@
>  
>      i = 0;
> +    while ((node = value_generator (&i)))
> +      {
> +        nodes[i-1]=node;

Coding style: we leave spaces around operators, "nodes[i - 1] = node;". I'll fix that when I commit it.

Fixed in git for 1.6.20, 1.7.10.