Use of offsetof macro on non standard-layout type is undefined at least prior to C++17

[astrelni]

To the best of my understanding benchmark.cc#L316, which calls offsetof on benchmark::State, is undefined behavior.

C++11 18.2.4 [support.types]

The macro offsetof(type, member-designator) accepts a restricted set of type arguments in this International Standard. If type is not a standard-layout class (Clause 9), the results are undefined.

benchmark::State is a non standard-layout type because all of its non-static members do not have the same access control.

I am not sure what would be lost by just removing the static_assert, so I am starting this off as an issue instead of going straight for a PR.

[dmah42]

@EricWF who added the check.

Simply, the static_assert exists to ensure that we keep the performance-necessary struct layout.

[astrelni]

For context, I am currently failing to build a benchmark with gcc because it is triggering Werror=invalid-offsetof on that line.

[dmah42]

I noticed that in the bazel builds. We currently disable that warning for our gcc builds: https://github.com/google/benchmark/blob/master/CMakeLists.txt#L123

[EricWF]

yeah, my bad. I figured this might come up. However all compilers seem to support the behavior as an extension even though they generate a warning about it.

[astrelni]

Would you be in favor of turning it into a runtime CHECK on &error_occured_ - this with reinterpret_casts to char* to keep standard compliant?

[astrelni]

Ping @EricWF @dominichamon

[EricWF]

Are we just complaining the the compiler issues a warning? If so we should just disable the warning. As long as the static assert passes there is no undefined behaviour; because there can't be undiagnosed undefined behaviour at compile time.

[astrelni]

So if IIUC we just need to add -Winvalid-offsetof to the bazel copts?

[EricWF]

Yeah, either that or we could wrap the static assert in a `#pragma diagnostic disable "-Winvalid-offsetof". Either option seems good to me.

[dmah42]

It's disabled in the cmake, so doing that in bazel sgtm. but only if it's gcc (ie, if the warning is supported).

[EricWF]

I decided to disable it in the source instead, just to ensure we don't have invalid applications of offsetof in other places we don't intend.