Optional.Basic test fails with GCC 9 and `-fstack-protector`

[lw]

I'm trying to build and run the test suite on my company's toolchain and I'm seeing an error in the Optional.Basic test. I managed to narrow it down to a small repro. I am using GCC 9.2.1. First, I modify the Makefile as follows:

-HOST_CFLAGS := -g -O2 -Wall -Werror -Wextra -Iinclude
+HOST_CFLAGS := -fstack-protector -g -O2 -Wall -Werror -Wextra -Iinclude

Then I build with make out/test. And when I run out/test I get:

[----------] 7 tests from Optional
[ RUN      ] Optional.Basic
test/optional_tests.cpp:205: Failure
Value of: std::equal(expected.begin(), expected.end(), value.get().begin())
  Actual: false
Expected: true
[  FAILED  ] Optional.Basic (0 ms)

Which comes from here: https://github.com/google/libnop/blob/a7e824f28b0fd138e68c35ca21452de2a1d7d7a2/test/optional_tests.cpp#L200-L206 If I comment out that EXPECT_TRUE the entire suite passes. If I print the items of value.get() I see that they are 1 and 20 rather than 10 and 20.

I am not sure whether this is a compiler bug or an issue in libnop, as I don't really know what -fstack-protector does. Could someone with more expertise look into this? Perhaps running those tests with ASAN could shed some light on the issue... Thanks!

[eieio]

Thanks for the detailed report! I'll take a look and follow up with you when I understand the issue.

[lw]

By the way, the same test also fails when building with optimizations enabled. To be more precise, I am undoing the above change, and modifying the Makefile as follows:

-HOST_CFLAGS := -g -O2 -Wall -Werror -Wextra -Iinclude
+HOST_CFLAGS := -g -O2 -Wall -Wextra -Iinclude

-M_CFLAGS := -I$(GTEST_INCLUDE) -O0 -g
+M_CFLAGS := -I$(GTEST_INCLUDE) -g

I had to remove -Werror as otherwise the code I added to the test to print the contents of value.get() would error:

compile test/optional_tests.cpp
test/optional_tests.cpp: In member function ‘virtual void Optional_Basic_Test::TestBody()’:
test/optional_tests.cpp:207:21: error: ‘<anonymous>’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  207 |         oss << v << ", ";
      |                     ^~~~
test/optional_tests.cpp:207:21: error: ‘*((void*)&<anonymous> +4)’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
cc1plus: all warnings being treated as errors

When I build with the above change, the test fails like this:

[----------] 7 tests from Optional
[ RUN      ] Optional.Basic
test/optional_tests.cpp:211: Failure
Value of: std::equal(expected.begin(), expected.end(), value.get().begin())
  Actual: false
Expected: true
-1914865933, 32524, 
[  FAILED  ] Optional.Basic (0 ms)

I'll try to run this with ASAN to see what I get.

[lw]

I have a theory on what the problem might be. On cppreference.com I found this about std::initializer_list:

The underlying array is a temporary array of type const T[N], in which each element is copy-initialized (except that narrowing conversions are invalid) from the corresponding element of the original initializer list.

Copying a std::initializer_list does not copy the underlying objects.

If we look at these lines of the test code:

 Optional<std::initializer_list<int>> value{ 
     InPlace{}, std::initializer_list<int>{10, 20}}; 

we'll see that we create a temporary initializer_list as an argument, which has a temporary array of ints, and the one stored inside the Optional will share that array. But as soon as that instruction is over, the temporary will be destroyed and the initializer_list inside the Optional will hold a stale reference.

I tried to verify the theory by changing the code to:

    std::initializer_list<int> foo = {10, 20};
    Optional<std::initializer_list<int>> value{InPlace{}, foo};

and it seems to work, because the original initializer_list isn't a temporary anymore and thus its storage is kept alive for the entire duration of the test.

[eieio]

Yes, you're right. There is a lifetime issue with this subtest block.

Copying a std::initializer_list does not copy the underlying objects.

Given that, I think removing the subtest of Optional<std::initializer_list<int>> is the right fix, as there is coverage of the in-place Optional constructor in other subtest blocks.

I'm happy to take a pull request or I can address this myself later this week.

[lw]

If you could do it that would be great, as otherwise I need to figure out how to deal with the CLA...

[eieio]

Ah, I understand. Fixed on master.

Cheers!

[lw]

Awesome, thanks!