Build failure with gcc-8.1.0

[Paul Hargrove]

We now have gcc-8.1.0 on Dirac (module switch gcc gcc/8.1.0). Unfortunately it is unhappy with our use of __thread:

/usr/local/pkg/openmpi-3.0.0/gcc-8.1.0/bin/mpicxx -std=c++17 -pie -fpie -std=c++11 -D_GNU_SOURCE=1 -I/home/upcnightly/gcc81-EX/runtime/bld/dbg/upcxx/.nobs/art/bbfa263ad99b5672a4ac81336ca516330f820c86 -DUPCXX_ASSERT_ENABLED=1 -DUPCXX_BACKEND=1 -DUPCXX_BACKEND_GASNET_SEQ=1 -DUPCXX_LPC_INBOX_lockfree=1 -D_GNU_SOURCE=1 -DGASNET_SEQ -D_REENTRANT -I/home/upcnightly/gcc81-EX/runtime/inst/dbg/include -I/home/upcnightly/gcc81-EX/runtime/inst/dbg/include/ibv-conduit -O0 -g -Wall -g3 -Wno-unused -Wunused-result -Wno-unused-parameter -Wno-address -c /home/upcnightly/gcc81-EX/runtime/bld/dbg/upcxx/src/persona.cpp -o /home/upcnightly/gcc81-EX/runtime/bld/dbg/upcxx/.nobs/art/c82c131ef613ba150237acbaaa6c4fc0e9aae83f.persona.cpp.o

/home/upcnightly/gcc81-EX/runtime/bld/dbg/upcxx/src/persona.cpp:5:42: error: non-local variable 'upcxx::detail::the_persona_tls' declared '__thread' needs dynamic initialization
     __thread persona_tls the_persona_tls{};
                                          ^
/home/upcnightly/gcc81-EX/runtime/bld/dbg/upcxx/src/persona.cpp:5:26: note: C++11 'thread_local' allows dynamic initialization and destruction
     __thread persona_tls the_persona_tls{};
                          ^~~~~~~~~~~~~~~

There are also many warnings.

I will attach the full log shortly.

[Paul Hargrove]

Full log for the failed build

[john bachan]

This feels like a compiler bug. The constructor is marked as constexpr, and per C++ constexpr initialization is definitely included in static initialization, not dynamic initialization. See https://en.cppreference.com/w/cpp/language/constant_initialization.

So far I have verified two worrisome workarounds, both which involve mismatched variable declaration/definitions:

1. Make the variable definition (in the cpp) use thread_local while retaining __thread in the header. Keeping the header __thread makes consumer code fast since it obviates the need for the lazy tls init check.

2. Make the variable definition use std::aligned_storage<sizeof(persona_tls),alignof(persona_tls)>::type as the type, which will be zero init'd by default (which is all the constexpr function is doing), and retain the correct type in the header.

[Dan Bonachea]

If we believe this to be a compiler bug and decide to apply a workaround that bends or breaks the C++ rules (but happens to solve the problem for that compiler), then I'd encourage use the preprocessor to ensure that workaround is only active for the compiler version(s) that are both affected by the bug and validated to be correct with the workaround.

[Paul Hargrove]

Is this old gcc bug relevant: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58408

[john bachan]

It looks like the same bug, but I can't test the workaround posted at the bottom due to lack of copy constructor... without significant reengineering anyway.

[Dan Bonachea]

This issue was triaged at the 2018-06-13 Pagoda meeting and assigned a new milestone/priority.

This issue is directly relevant to the Sept 19 milestone, and we resolved that before then we need to either deploy a workaround or document the compiler version as blacklisted due to bug.

[Paul Hargrove]

Some light retesting shows this issue may have been resolved.
I will report on complete CI results when they are available.

[Paul Hargrove]

I have complete runs of the GASNet CI testing against the current upcxx develop branch with gcc-8.1.0 and gcc-8.2.0.
Both are on the same system where I observed the issue I originally reported here.
However, neither compiler failed.

I am not closing (yet) since I am not sure what has changed (and thus don't know if this is "really" fixed).
One thought is that the fixes for icc also addressed this issue, but I don't know if that even makes sense.

[Paul Hargrove]

As I mentioned in my prev comment, I cannot claim to know what had changed to resolve this issue, but all indications are that it has been resolved. Given the timing, it is likely that the work done for icpc support was related in some way.

We are now testing g++-8.2.0 on multiple Linux and macOS systems without any sign of this problem.
I am satisfied that this issue is resolved with respect to 8.2.0.
If it still exists with g++-8.1.0 (though it is not in manual testing today), then the recommended fix is to update to 8.2.0.