Fix #9947: make the ABI identical between debug and non-debug builds

[pitrou]

No description provided.

[pitrou]

cc @fowles @coryan

[coryan]

You probably know this, but just in case. This preserves the ABI, but would make it an ODR violation to link code with NDEBUG and without NDEBUG. The ODR rules require that all definitions of an inline function be identical:

Each such definition shall consist of the same sequence of tokens, where the definition of a closure type
is considered to consist of the sequence of tokens of the corresponding lambda-expression.

I am not sure how to quote the standard, but this draft:

https://isocpp.org/files/papers/N4860.pdf

"6.3 One-definition rule", paragraph 13.8

I am well aware that such violations of the ODR are mostly harmless.

[pitrou]

Yes, it was already stated on the linked issue as a reason to not bother with the ABI problem.
However, as you also noted, all Unix compilers pretty much have to deal nicely with this as the practice of building in debug mode against non-debug dependencies is extremely widespread. So the compiler/linker will end up selecting one implementation over another; which one exactly doesn't really matter in practice (the only difference being the added debug checks).

Windows is different and there you pretty much have to match the build mode accross all dependencies for other reasons.

[fowles]

Code looks ok. Kicking off a test cycle of our CI. I am seeing some unrelated C# failures already and have pinged the person responsible for those.

[fowles]

We have a 21.x release coming later this week (god willing and the creek don't rise). I will make sure this gets into it.

[fowles]

#10272 should have fixed the distcheck errors so you will need to rebase on top of that

[pitrou]

Ok, I rebased.

[pitrou]

Thank you @fowles !

[fowles]

No problem. I do want to be clear that this is never a configuration we will officially support. We will continue to review PRs on a best effort basis for it, but you are in the danger zone. I am sorry that a bunch of my earlier communication on this made it seem like a very hard "no" and not more of "you are in the land of dragons and we will only go so far with you".

[knedlsepp]

No problem. I do want to be clear that this is never a configuration we will officially support. We will continue to review PRs on a best effort basis for it, but you are in the danger zone. I am sorry that a bunch of my earlier communication on this made it seem like a very hard "no" and not more of "you are in the land of dragons and we will only go so far with you".

It sounds like this requirement could be made explicit by ensuring that -DNDEBUG is part of the pkg-config --cflags protobuf output in case protobuf was built with that. This would match quite well with what is already in the README:

protobuf/src/README.md

Lines 79 to 81 in 3b456bf

	To compile a package that uses Protocol Buffers, you need to pass
	various flags to your compiler and linker. As of version 2.2.0,
	Protocol Buffers integrates with pkg-config to manage this. If you

Apparently -DNDEBUG should be part of the flags mentioned there.

[adamnovak]

Apparently -DNDEBUG should be part of the flags mentioned there.

Wouldn't that make it quite difficult to use assert() in user code?

[fowles]

@adamnovak ideally, when compiling with asserts enabled you would also compile protobuf with asserts enabled. (We actually have a bunch of extra asserts that will help users catch bugs in their handling of protobuffers)

[knedlsepp]

Wouldn't that make it quite difficult to use assert() in user code?

Yes, and for that matter I'm very grateful that @fowles opted for still best-effort supporting this.
However if there were no negative side-effect to adding this flag for downstream users and it being a requirement for the protobuf build that downstream users must use the same flags that were used at build-time then pkg-config would be the way to go.

Ideally protobuf would not choose a macro that clashes with a libc option that usually can be freely set or unset.
Maybe something like -DPROTOBUF_OPTIMIZED_BUILD, which would then be free to ignore ABI-stability between debug and release builds.

[fowles]

I am not opposed to having a PROTOBUF_NDEBUG that is independent of NDEBUG. Unfortunately, that work would have to be done by a googler (not necessarily someone on the protobuf team). Protobuf's code is a bit of an iceberg where there is a large volume of google internal code. PRs that fixed this externally would leave our code base in a worse franken state.

[adamnovak]

I think Protobuf could still keep using the NDEBUG macro name internally if there were a way to make sure it had the right state in the Protobuf headers, independent of the state it needs to have in the application code that includes them.

One way to do this would be to frame the places where the ABI depends on NDEBUG with macros that make sure NDEBUG is defined or not defined so as to match what the library we will need to link against, while keeping changes from leaking out of the Protobuf headers into client code.

Something like:

Leading header (include after all other Protobuf headers in Protobuf headers with variable ABIs):

#ifndef PROTOBUF_ABI_COMPAT_ACTIVE
    #define PROTOBUF_ABI_COMPAT_ACTIVE
    #if defined(PROTOBUF_LIBRARY_IS_DEBUG) && defined(NDEBUG)
        // App is release, but needs to link against a debug Protobuf built without NDEBUG    
        #define PROTOBUF_RELEASE_APP_ON_DEBUG_LIBRARY
        #undef NDEBUG
    #elif defined(PROTOBUF_LIBRARY_IS_RELEASE) && !defined(NDEBUG)
        // App is debug but needs to link against a release protobuf built with NDEBUG
        #define PROTOBUF_DEBUG_APP_ON_RELEASE_LIBRARY
        #define NDEBUG
    #endif
#endif

Trailing header (include at the end of all Protobuf headers that include the leading header):

#ifdef PROTOBUF_ABI_COMPAT_ACTIVE
    #undef PROTOBUF_ABI_COMPAT_ACTIVE
    #if defined(PROTOBUF_RELEASE_APP_ON_DEBUG_LIBRARY)
        #undef PROTOBUF_RELEASE_APP_ON_DEBUG_LIBRARY
        #define NDEBUG
    #elif defined(PROTOBUF_DEBUG_APP_ON_RELEASE_LIBRARY)
        #undef PROTOBUF_DEBUG_APP_ON_RELEASE_LIBRARY
        #undef NDEBUG
    #endif
#endif

Then in use it would look like:

// Other includes

#include <google/protobuf/abi_compat_on.h>

class Whatsit {
#ifdef NDEBUG
    // One ABI for release
    void some_method();
#else
    // Another completely different ABI for debug
    int some_method();
#endif
};

#include <google/protobuf/abi_compat_off.h>

If you set it up like that, NDEBUG still controls the ABI if no additional macros are defined, or in Google-internal headers that haven't yet been changed. But an application build system can decide if it is dealing with a release or debug Protobuf library to link against, and send -DPROTOBUF_LIBRARY_IS_DEBUG or -DPROTOBUF_LIBRARY_IS_RELEASE as appropriate, and then the Protobuf headers will make sure to adopt the right NDEBUG setting to match the library, without letting it leak out when they themselves are included in user code.

Then if pkg-config wants to communicate information about the CPPFLAGS to use with the Protobuf library, it could then do it via Protobuf-namespaced macro definitions and not grab hold of the global NDEBUG.

Whatever the final answer is, it probably would want to work well with CMake's FindProtobuf in addition to just pkg-config, which might involve making contributions to CMake. It already knows how to find and manage debug and release versions of the library, but it also has just a default version that could be either, that it will let you try to link against.