"Use of offsetof() with non-POD type" warning in PGI 19.10+

[Paul Hargrove]

With the new release of PGI-19.10 (installed on Dirac) I am seeing the following warning in CI when nobs is performing its lazy build of the library:

mpicxx -std=c++11 -D_GNU_SOURCE=1 -I/home/data2/upcnightly/dirac/EX-dirac-ibv-pgi_1910/work/dbg/upcxx/.nobs/art/13d7940579106f14bdb27be6e0e507ac43dfdbac -DUPCXX_ASSERT_ENABLED=1 -DUPCXX_MPSC_QUEUE_ATOMIC=1 -mp -O0 -g -c /home/data2/upcnightly/dirac/EX-dirac-ibv-pgi_1910/work/dbg/upcxx/src/future/core.cpp -o /home/data2/upcnightly/dirac/EX-dirac-ibv-pgi_1910/work/dbg/upcxx/.nobs/art/1956549d518c3a6f685f3e9112dcf3da7cda68f9.core.cpp.o
"/home/data2/upcnightly/dirac/EX-dirac-ibv-pgi_1910/work/dbg/upcxx/src/future/c
          ore.cpp", line 26: warning: offsetof applied to non-POD types is
          nonstandard
    offsetof(future_header_promise<>, pro_meta),
    ^

This is concerning, since compilers are not required (IIUC) to provide a correct implementation of this construct. The only mitigation of this concern that I can see, is that this code is (currently) only reachable for PGI compilers (part of UPCXX_PROMISE_VTABLE_HACK logic).

I am reporting against the Development Branch, but have no specific reason to doubt this usage is in our 2019.0.0 release.

[Dan Bonachea]

@Paul Hargrove said:

The only mitigation of this concern that I can see, is that this code is (currently) only reachable for PGI compilers (part of UPCXX_PROMISE_VTABLE_HACK logic).

This is true, but nearly identical code also appears outside the #ifdef at src/future/core.hpp:588. I don't understand why PGI is warning about one but not the other - both should be behaviorally equal according to the spec. I suspect the issue is the other use is constexpr so the offsetof expression is getting folded away before the analysis phase that complains this use is problematic.

It's also worth noting the current design goes to significant effort to attempt to ensure this offsetof is well-defined:

525     struct promise_meta {
526       // We derive from `lpc_base` but have to use "first member of standard
527       // layout" instead of proper inheritance because we need this type to
528       // be standard layout so that the containing header is also standard layout
529       // so we can ultimately use `offsetof` to know the offset of this type
530       // in the header.
531       lpc_base base;

My best guess is the lpc_base field shown above is breaking POD because the intru_queue_intruder field it contains is not standard-layout.

However I suspect that this code will always run afowl of conditionally supported offsetof for promise<T&> since storing any reference-type result field automatically forfeits standard layout - so there may be no way to guarantee this offsetof always works.

If these suppositions are correct, it potentially means that any compilers that chose to get picky could start rejecting all these calls.

[john bachan]

intru_queue_intruder ought to be StandardLayout. It's only field is std::atomic<T*> which according to https://en.cppreference.com/w/cpp/atomic/atomic is guaranteed StandardLayout (see partial specializations). PGI is probably just wrong. If it is constexpr which is getting things to work in the .hpp, perhaps I can do the calculation in a constexpr context and store its value at const init.

[john bachan]

And seeing as offsetof only requires StandardLayout and not full POD (https://en.cppreference.com/w/cpp/types/offsetofa), the PGI warning message basically advertises that it isn't implemented correctly.

@Dan Bonachea the choice of T in promise<T> ought not affect the layout of the classes involved in the offset calculation since I internally use untyped std::aligned_storage<sizeof(T),alignof(T)>::type storage with manual construction/destruction.

[Dan Bonachea]

intru_queue_intruder ought to be StandardLayout.

Agreed - I withdraw that comment.

I've also confirmed that std::aligned_storage is reported as StandardLayout, even though std::tuple<...> sometimes is NOT (even with POD elements, like std::tuple<int,int>).

I agree the PGI warning message is an incorrect paraphrasing of the spec, and may reflect why the warning is implemented incorrectly.

Finally I've confirmed that PGI triggers the warning even for non-empty parameter packs when offsetof(upcxx::detail::future_header_promise<int>, pro_meta) is encountered outside constexpr context:

"upcxx-pod.cpp", line 65: warning: offsetof applied to non-POD types is
          nonstandard
    offsetof(upcxx::detail::future_header_promise<int>);
    ^

I'll open a new issue for the apparent PGI bug that should be reported.
Let's keep this issue to track our workaround efforts.

[Dan Bonachea]

This was resolved in PR#134

Resolve issue #286 (#284) via LIBUPCXX_CXXFLAGS

This commit adds the appropriate warning suppression flags to compilations of C++ sources in libupcxx.a to eliminate the incorrect warnings identified in issue #286.

→ <<cset af0d21d2ef8e>>