gtest: Do not reference temporaries past the expression that created them

[rwy7]

This is an error in gtest, not in OMR.

This same error is repeated in a few places, causing different types of
bad behaviour on AIX, including breaking almost all logging and option
processing.

Rather than bind a const mytype& to a temporary result, copy the result
into a local, which ensures it is safe to access the value later in the
program.

When a reference is bound to a temporary value, that reference is
invalid when the value dies, at the end of the "full expression" that
created it. Using this reference to a temporary in a later expression
results in a kind of "use after destruction" error.

Fixes: #4537
Signed-off-by: Robert Young rwy0717@gmail.com

[smlambert]

Is this a change that can or should be contributed back to gtest directly?

[rwy7]

It should be, I'm in the middle of going through the google CLA right now. I figure it will be at least a few days, though

EDIT: Nevermind, already done 💃 !

[mgaudet]

Side note: I think this is actually an AIX compiler bug. My understanding is that this pattern should be fine due to lifetime extension

All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation. This is true even if that evaluation ends in throwing an exception.

There are two exceptions from that:

• The lifetime of a temporary object may be extended by binding to a const lvalue reference or to an rvalue reference (since C++11), see reference initialization for details.

(emphasis mine)

That fact that it -only- blows up on AIX is another argument in favour of that.

[rwy7]

Yes, I actually I meant to update this issue, I am beginning to suspect compiler bug as well. When I first started looking at this bug, I wasn't aware of the lifetime extension rule :). However, I have checked, and the rule is correctly applied by AIX in simple cases. I need to spend more time looking at the crash. As you said, the pattern of code should be valid.

[rwy7]

@genie-omr build aix

[rwy7]

@genie-omr build aix

[rwy7]

This is a compiler bug in XLC 12.1, possibly older versions as well. The problem is not present in XLC 13.1, which we are using in OMR CI. The problematic code was introduced with gtest 1.8 all the way back in 2017.

The two problem lines were:

• A reference bound to the temporary result of string concatenation
• A reference bound to a temporary string pulled from a string stream

I've tried to update every line that binds a reference to a temporary, just just to make sure there are no problems we don't know about yet. In this case, I'm willing to make a code change because it is such a minor patch, in tests only, for a toolchain we officially support. Since this is a bug in XLC, I'm not going to be contributing these changes upstream to gtest. Before we upgrade gtest again, we should look at finally dropping support for XLC 12.1.

Without this patch, the gtest output looks like this:

./omrthreadtest
[==========] Running  tests from  test cases.
[==========]  tests from  test cases ran. ( ms total)
[  PASSED  ]  tests.
[  ALL TESTS PASSED  ] 
./omrthreadtest --gtest_also_run_disabled_tests --gtest_filter=ThreadCreateTest.DISABLED_SetAttrThreadWeight
[==========] Running  tests from  test cases.
[==========]  tests from  test cases ran. ( ms total)
[  PASSED  ]  tests.
[  ALL TESTS PASSED  ] 
ALL omr_threadtest PASSED
DONE

Since this PR is just hacking around a toolchain bug, I can understand why someone wouldn't want this contributed, so if anyone opposes this PR, please comment or just thumbs down the issue.

[youngar]

@genie-omr build all

[youngar]

Travis testing has passed, although github's UI is not reflecting this (for me). Merging!