MSVC C++ compliance: Using __builtin_offsetof

[Radnyx]

Hi folks,

This is in regard to the definition of PROTOBUF_FIELD_OFFSET:

protobuf/src/google/protobuf/port_def.inc

Lines 311 to 314 in 0644388

	#define PROTOBUF_FIELD_OFFSET(TYPE, FIELD) \
	static_cast< ::uint32_t>(reinterpret_cast<const char*>( \
	&reinterpret_cast<const TYPE*>(16)->FIELD) - \
	reinterpret_cast<const char*>(16))

The Visual C++ team found that this macro is supposed to cause a compiler error. It uses reinterpret_cast in constexpr statements, which isn't allowed by the standard. We intend to emit an error for this macro in future versions of MSVC.

Fortunately, MSVC supports __builtin_offsetof, so we ask that you use that instead. Let me know if you have any questions.

Thank you.

[JasonLunn]

@Radnyx - can you confirm whether the change that landed in 92f8c5b addresses this issue for you?

[Radnyx]

Hi @JasonLunn, yes this is what we had in mind!

One thing to address: Since MSVC is introducing a compiler error, we're worried that if customers upgrade MSVC, they might run into friction and need to regenerate their protobuf code.

Is that the case here? And if so, what's protobuf's expectation for customers? Are compiler versions expected to be compatible with older generated protobuf code?

Thank you for the quick response.

[esrauchg]

@Radnyx Can you advise on when this change in behavior to MSVC might reach end users? Is there an expected version that it'll be included in?

This isn't just about the generated code, but the "runtime" code too (for C++ we only support having an exact match between generated code and runtime version though so its a bit of an academic distinction in that case, but the difference is more relevant for other languages).

It will help inform the discussion about potentially doing backported point releases with this change to older LTS versions, or if it would be expected that users would just need to use a sufficiently new Protobuf version to be compatible with that MSVC version.

Thanks!

[Radnyx]

@esrauchg No expected version as we'd like to measure the impact first. Still deciding whether to let users "opt-in" to the conformant behavior.

We appreciate the willingness to either backport or expect users to upgrade. I'll message back shortly on our plan.

[Radnyx]

Alrighty, thanks for your patience. We've decided not to add the error soon. Conformance there is still a future goal though.
So in the meantime, it would be ideal to backport the __builtin_offsetof change to older LTS versions.