Bug 70792 - Incorrect sequence point warning with uniform initializer syntax
Summary: Incorrect sequence point warning with uniform initializer syntax
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, wrong-code
Depends on:
Blocks:
 
Reported: 2016-04-25 20:06 UTC by Jason Turner
Modified: 2021-04-06 04:15 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-10-01 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Turner 2016-04-25 20:06:04 UTC
The following code:


#include <utility>

struct Do_Call
{
  template<typename Func, typename ... T>
  Do_Call(const Func &f, T && ... t)
  {
    f(std::forward<T>(t)...);
  }
};

int main()
{
  const auto sum = [](int w, int x, int y, int z) {
    return w + x + y + z;
  };
  
  int i = 0;
  Do_Call{sum, ++i, ++i, ++i, ++i};
}

Generates the -Wsequence-point warning for the variable 'i'. However, the standard states:

"Within the initializer-list of a braced-init-list, the initializer-clauses, including any that result from pack expansions (14.5.3), 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. [Note: This evaluation ordering holds regardless of the semantics of the initialization; for example, it applies when the elements of the initializer-list are interpreted as arguments of a constructor call, even though ordinarily there are no sequencing constraints on the arguments of a call. —end note ]"

The "Note: This evaluation ordering holds regardless of the semantics of the initialization; for example, it applies when the elements of the initializer-list are interpreted as arguments of a constructor call, even though ordinarily there are no sequencing constraints on the arguments of a call. —end note" is the most applicable to this bug report.

This occurs in all versions tested between 4.7.3->6
Comment 1 Jason Turner 2016-04-25 20:37:35 UTC
Note: I tested this code further and the compiler seems to be generating incorrect code based on this standard, not just warning incorrectly.
Comment 2 Jason Turner 2016-04-25 23:34:41 UTC
Follow up, this is a better test case that does not pass by &&


struct MyType {
  MyType(int i, int j, int k, int l)
    : sum(i + j + k + l)
  {

  }

  int sum;
};

int main()
{
  int i = 0;
  std::cout << MyType{ ++i, ++i, ++i, ++i }.sum << '\n';
}
Comment 3 Eric Gallager 2017-08-24 21:47:09 UTC
Confirmed that g++ prints 3 -Wsequence-point warnings on each testcase. 

(the followup one is missing an include for <iostream>, but after that, it behaves as reported)
Comment 4 Matthijs van Duin 2018-09-30 13:53:42 UTC
It seems bug 51253 previously addressed this, which means this is a regression. Also bug 65866 exists for the warning itself.

I can confirm incorrect code generation using g++ 8.2.0 on this simpler testcase:


#include <utility>

int main() {
	int i = -1;
	return std::pair{ ++i, ++i }.first;
}


$ g++ -Wall -std=c++17 -o list-init-sequence{,.cc}
list-init-sequence.cc: In function ‘int main()’:
list-init-sequence.cc:6:20: warning: operation on ‘i’ may be undefined [-Wsequence-point]
  return std::pair{ ++i, ++i }.first;
                    ^~~
$ ./list-init-sequence || echo fail
fail


Interestingly, this variation does not produce a warning but still produces incorrect code:

#include <utility>

int main() {
	int i = 0;
	return std::pair{ i, ++i }.first;
}

$ g++ -Wall -std=c++17 -o list-init-sequence{,.cc}
$ ./list-init-sequence || echo fail
fail
Comment 5 Jonathan Wakely 2018-10-01 12:49:07 UTC
(In reply to Matthijs van Duin from comment #4)
> It seems bug 51253 previously addressed this, which means this is a
> regression.

No. To be a regression it has to have previously worked, then stopped working. All versions of GCC warn about this code, so it's not a regression.

The fix for bug 51253 only changed the code generation, it didn't stop the warnings being printed.
Comment 6 Jonathan Wakely 2018-10-01 12:53:06 UTC
(In reply to Jonathan Wakely from comment #5)
> The fix for bug 51253 only changed the code generation, it didn't stop the
> warnings being printed.

And it didn't affect the result for any of the testcases in this bug.

So it seems the fix for 51253 was simply incomplete, but there's no regression.
Comment 7 Matthijs van Duin 2018-10-01 13:33:28 UTC
Ah! I should have checked the actual tests of 51253 before calling it a regression, apologies. I just kinda assumed that these simple cases would be covered by the work done back then.
Comment 8 Matthijs van Duin 2019-01-26 02:42:16 UTC
(In reply to Matthijs van Duin from comment #4)
> 	return std::pair{ ++i, ++i }.first;

My bad! This isn't an exhibit of the bug. I simply forgot that std::pair is not really a struct, and this isn't aggregate initialization: the constructor takes references, so correct code is generated in this case.

And in fact, if you do use an aggregate, the test works correctly.

However, if you replace std::pair by a class whose constructor takes (int, int), similar to the one used in the existing testcase (g++.dg/cpp0x/initlist86.C) then it fails again.

Looking at the disassembly (on ARM since I don't know x86 asm) shows that gcc loads both arguments from the storage allocated for i, after both increments have been done. Effectively it's copy-constructing the first argument too late.

The more general issue appears to be that if the arguments are trivially copyable lvalues, then gcc keeps these as lvalues and copy-constructs the actual arguments way too late. If I look at this disassembly of this code:

  struct Foo {
    char x[64]; // too big to pass in register
    Foo( Foo const &other ) = default; // but still trivially copyable
    Foo &mutate();
  };
  
  struct Pair {
    Pair( Foo x, Foo y );
  };
  
  void test( Foo &foo ) {
    Pair{ foo.mutate(), foo.mutate() };
  }

Then test() effectively does:

  Foo &temp1 = foo.mutate();
  Foo &temp2 = foo.mutate();
  Pair{ temp1, temp2 }  // copy-construct arguments and call Pair constructor

(Also, interestingly, temp2 is copy-constructed before temp1 is!)

If Foo is not trivially copyable, even if merely due to the presence of a destructor, then the problem disappears.