Bug 61382 - parameter pack expansion unexpected order
Summary: parameter pack expansion unexpected order
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: 4.9.1
Assignee: Jason Merrill
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-01 21:17 UTC by Johannes Steinmetz
Modified: 2014-07-08 22:34 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-06-04 00:00:00


Attachments
small example that expands like 3,2,1,0 ... should be 0,1,2,3 (270 bytes, text/x-c++src)
2014-06-01 21:17 UTC, Johannes Steinmetz
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Johannes Steinmetz 2014-06-01 21:17:42 UTC
Created attachment 32882 [details]
small example that expands like 3,2,1,0 ... should be 0,1,2,3

C++11 variadic templated parameter pack expansion happens for {}-initialiser in reversed oder. As far as I understand this, they should be expanded in order. I am not sure if its expansion or evaluation thats out of order.... clang++ does the correct(?) ordering.
Comment 1 Jonathan Wakely 2014-06-01 22:27:14 UTC
It's definitely a bug ... I think there's an existing PR about it.
Comment 2 Thibaut LUTZ 2014-06-03 23:03:38 UTC
@Jonathan: you might be referring to 56774. 59716 was a similar issue.

However I think this case is definitely NOT a bug. Here is why:

- let's decompose the line:
std::tuple<ARGS...> r { get_single<ARGS>(posfoo++)... };

* std::tuple<ARGS...> r{ ... }: this is a constructor call: std::tuple(args...)
* get_single<ARGS>(posfoo++)... expands to get_single<int>(posfoo++), get_single<float>(posfoo++), get_single<float>(posfoo++), get_single<int>(posfoo++)

Putting the two together, with the previous line:
int posfoo = 0;
std::tuple<ARGS...> r ( get_single<int>(posfoo++),
                        get_single<float>(posfoo++),
                        get_single<float>(posfoo++),
                        get_single<int>(posfoo++));

This is the root of your problem: 
N3797 §8.3.6.9: The order of evaluation of function arguments is unspecified.

The pack expansion is working fine, but the increments to posfoo are not being sequenced. Clang does evaluate arguments in the opposite order as GCC does, which makes it look correct in this case, but it is still undefined behavior. 

You can re-write your get_single function to do a pack traversal instead.
Comment 3 Jonathan Wakely 2014-06-04 01:06:14 UTC
(In reply to Thibaut LUTZ from comment #2)
> @Jonathan: you might be referring to 56774. 59716 was a similar issue.

They don't look related. I meant PR 51253

> However I think this case is definitely NOT a bug. Here is why:
> 
> - let's decompose the line:
> std::tuple<ARGS...> r { get_single<ARGS>(posfoo++)... };
> 
> * std::tuple<ARGS...> r{ ... }: this is a constructor call:
> std::tuple(args...)

No, this is not a valid transformation, the semantics of T r{args...} are not the same as T r(args...), see [dcl.init.list] p4.
Comment 4 Thibaut LUTZ 2014-06-04 08:50:14 UTC
You're right, my bad. Thanks for correcting me.

The exact quote is 
> Within the initializer-list of a braced-init-list, the initializer-clauses, including any that result from pack expansions, are evaluated in the order in which they appear. That is, every value computation and side effect associated with a given initializer-clause is sequenced before every value computation and side effect associated with any initializer-clause that follows it in the comma-separated list of the initializer-list.

So the increments *should* be sequenced.

Sorry about the noise.
Comment 5 Jason Merrill 2014-06-04 15:51:34 UTC
Author: jason
Date: Wed Jun  4 15:51:01 2014
New Revision: 211235

URL: http://gcc.gnu.org/viewcvs?rev=211235&root=gcc&view=rev
Log:
	PR c++/51253
	PR c++/61382
gcc/
	* gimplify.c (gimplify_arg): Non-static.
	* gimplify.h: Declare it.
gcc/cp/
	* cp-gimplify.c (cp_gimplify_expr): Handle CALL_EXPR_LIST_INIT_P here.
	* semantics.c (simplify_aggr_init_expr): Not here, just copy it.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/initlist86.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-gimplify.c
    trunk/gcc/cp/semantics.c
    trunk/gcc/gimplify.c
    trunk/gcc/gimplify.h
Comment 6 Jason Merrill 2014-06-04 15:51:45 UTC
Fixed on trunk for now.
Comment 7 Johannes Steinmetz 2014-06-11 21:35:06 UTC
(In reply to Jason Merrill from comment #6)
> Fixed on trunk for now.

Ohh great. Thank You!
Comment 8 Jason Merrill 2014-06-30 14:25:56 UTC
Author: jason
Date: Mon Jun 30 14:25:21 2014
New Revision: 212150

URL: https://gcc.gnu.org/viewcvs?rev=212150&root=gcc&view=rev
Log:
	DR 1030
	PR c++/51253
	PR c++/61382
	* cp-tree.h (CALL_EXPR_LIST_INIT_P): New.
	* call.c (struct z_candidate): Add flags field.
	(add_candidate): Add flags parm.
	(add_function_candidate, add_conv_candidate, build_builtin_candidate)
	(add_template_candidate_real): Pass it.
	(build_over_call): Set CALL_EXPR_LIST_INIT_P.
	* tree.c (build_aggr_init_expr): Copy it.
	* semantics.c (simplify_aggr_init_expr): Copy it.
	* cp-gimplify.c (cp_gimplify_expr): Handle it.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/g++.dg/cpp0x/initlist86.C
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/cp/ChangeLog
    branches/gcc-4_9-branch/gcc/cp/call.c
    branches/gcc-4_9-branch/gcc/cp/cp-gimplify.c
    branches/gcc-4_9-branch/gcc/cp/cp-tree.h
    branches/gcc-4_9-branch/gcc/cp/semantics.c
    branches/gcc-4_9-branch/gcc/cp/tree.c
    branches/gcc-4_9-branch/gcc/gimplify.c
    branches/gcc-4_9-branch/gcc/gimplify.h
Comment 9 Brooks Moses 2014-07-03 02:00:51 UTC
FWIW, the new initlist86.C test that this adds is failing on the google/gcc-4_9 branch on powerpc64le and aarch64, though it passes on x86_64.  I haven't yet checked on the upstream 4.9 branch, but I figured a heads-up might be useful.
Comment 10 Jason Merrill 2014-07-03 02:29:58 UTC
(In reply to Brooks Moses from comment #9)
> FWIW, the new initlist86.C test that this adds is failing on the
> google/gcc-4_9 branch on powerpc64le and aarch64, though it passes on
> x86_64.  I haven't yet checked on the upstream 4.9 branch, but I figured a
> heads-up might be useful.

Thanks.  Does removing "PUSH_ARGS_REVERSED &&" from the cp_gimplify_expr change fix it?
Comment 11 Brooks Moses 2014-07-03 03:37:03 UTC
(In reply to Jason Merrill from comment #10)
> Thanks.  Does removing "PUSH_ARGS_REVERSED &&" from the cp_gimplify_expr
> change fix it?

Nope -- I just gave it a try, and it doesn't seem to change things.  Still passes on x86_64 and fails on aarch64.
Comment 12 Jakub Jelinek 2014-07-03 08:28:55 UTC
I'm seeing:
FAIL: g++.dg/cpp0x/initlist86.C -std=c++11 execution test
FAIL: g++.dg/cpp0x/initlist86.C -std=c++1y execution test
on 4.9 branch (but not on trunk) on x86_64-linux -m32.
make check-g++ \
RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=initlist86.C'
Comment 13 Jason Merrill 2014-07-04 07:24:02 UTC
(In reply to Jakub Jelinek from comment #12)
> I'm seeing:
> FAIL: g++.dg/cpp0x/initlist86.C -std=c++11 execution test
> FAIL: g++.dg/cpp0x/initlist86.C -std=c++1y execution test
> on 4.9 branch (but not on trunk) on x86_64-linux -m32.

Hmm, I can't reproduce that compiling it by hand with my 4.9 branch build.
Comment 14 Jakub Jelinek 2014-07-04 08:24:01 UTC
Author: jakub
Date: Fri Jul  4 08:23:28 2014
New Revision: 212289

URL: https://gcc.gnu.org/viewcvs?rev=212289&root=gcc&view=rev
Log:
	PR c++/61382
	Backport from mainline
	2014-06-05  Andreas Schwab  <schwab@suse.de>

	* g++.dg/cpp0x/initlist86.C (main): Initialize i.

Modified:
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_9-branch/gcc/testsuite/g++.dg/cpp0x/initlist86.C
Comment 15 Brooks Moses 2014-07-08 02:57:25 UTC
(In reply to Jakub Jelinek from comment #14)
[...]
> 	* g++.dg/cpp0x/initlist86.C (main): Initialize i.
[...]

Aha ... yes, that would do it.  And, indeed, I can confirm that this fixes the failures I was seeing.  Thanks!

Jason, should this be marked as "resolved"?
Comment 16 Jason Merrill 2014-07-08 22:34:47 UTC
Yes.