__attribute__ macro redefinition in public headers on non GCC/Clang compilers

[kjedruczyk]

On compilers other than gcc and clang __attribute__ macro gets hijacked and replaced with empty one in https://github.com/jedisct1/libsodium/blob/794ec886e71d143d916f0c12020f82ab72b7690f/src/libsodium/include/sodium/export.h#L5-L10

It is extremely unfortunate as this is public header, so the redefinition leaks to translation units of sodium users. So anything that relies on any kind of __attribute__ that appears after including sodium headers will be unintentionally affected.

As a concrete example - in bloomberg's BDE libraries we rely on attribute to declare weak symbols on Solaris: https://github.com/bloomberg/bde/blob/cdfbfda476e9f82e7d918efd6f67e57427f17987/groups/bsl/bsls/bsls_linkcoercion.h#L72-L77

Including sodium headers before including parts of bde will unfortunately cause linker issues as __weak__ declarations disappear silently.

[jedisct1]

What compiler also supports __attribute__()? Is there a specific macro that should be used to identify that compiler?

[jedisct1]

Can you confirm that 74a4496cc5931cd5a72486768a84a40a19491cc4 fixes it so that it can be backported to stable?

[kjedruczyk]

What compiler also supports attribute()? Is there a specific macro that should be used to identify that compiler?

I don't believe this is right way of looking at it.

If there is a need to apply compiler-specific attributes there probably should be libsodium-specific definition that would map into appropriate __attribute__ on supported compilers (and would be empty for anything not explicitly supported).

Undefining compiler-provided macro - especially in a way that is visible to library clients - is not really appropriate.

[kjedruczyk]

Can you confirm that 74a4496 fixes it so that it can be backported to stable?

I don't think this is what you want to do. As far as I can see this patch makes libsodium try to apply clang/gcc specific attributes when compiled with SUNPRO compiler.

My guess is that the reason the undef trick was implemented in first place, was to make compilation succeed on compilers other than ones for which attributes were originally applied.

What should probably happen is attribute macro should be selectively applied. But that is normallly achieved by introducing custom macro, that maps into what it needs to on every platform.

Anyway, thanks for looking into that. I appreciate effort needed to make libsodium work on non-mainstream platforms.

[kjedruczyk]

To answer your question about the patch mentioned above (74a4496cc5931cd5a72486768a84a40a19491cc4): the patch is likely to create more issues than it solves - at very best it will end up triggering lots of warnings anytime gcc-specific attribute is applied.

[jedisct1]

While renaming __attribute__ would work, I would really try to avoid different names for standard language and compiler features. Using autoconf may be an alternative, but some people use different build systems.

What is the current behavior of the Sunpro compiler when hitting an unsupported attribute? If it's just emitting a warning, this can be ignored.

[jedisct1]

https://docs.oracle.com/cd/E18659_01/html/821-1384/gjzke.html provides an exhaustive list, which seems to be super set of the attributes currently used by the library.

[jedisct1]

I take it back, warn_unused_result doesn't seem to be in the list. This is extremely annoying.

[FSMaxB]

You could make a separate macro for every attribute used by libsodium.

For example:

#define warn_on_unused_result __attribute__((warn_unused_result))

etc.

This can then selectively enabled and disabled.

[jedisct1]

This is exactly what I would like to avoid. Giving a different name to standard compiler features, especially to work around an uncommon, unsupported platform.

[jedisct1]

Something like that has been done for hydrogen here https://github.com/jedisct1/libhydrogen/commit/21d41eef7e424a4c3152b09a9a7660a6739f067f

I still don't feel very comfortable with that approach.

We may end up doing the same thing in sodium but definitely not in the stable branch.

[kjedruczyk]

While renaming attribute would work, I would really try to avoid different names for standard language and compiler features. Using autoconf may be an alternative, but some people use different build systems.

I don't really think __attribute__ is a standard feature. It's common across compilers these days, but still, C++ standard specifically says any "global name starting with double underscore is reserved to implementation for any use".

Even if we stick to C standard (as I see most of libsodium is C, not C++):

• "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use"
• "The behaviour is undefined in following circumstances: [...] The program removes the definition of a macro whose name begins with an underscore and either an uppercase letter or another underscore (7.1.3)"

I don't think there is a way of both maintaining portability and using implementation-defined keywords at the same time, unfortunately.

[jedisct1]

It's a compiler extension supported by many compilers and static analysis tools, in addition to be something developers immediately understand. So, it's a bit sad to hide it as something unique to that library. If the wrapping macros are self-explanatory, it remains okayish, I guess :/

[kjedruczyk]

I share your frustration, keeping code portable can be a bit of a pain in C/C++. It's really great though that library like libsodium compiles with native tools on Solaris / AIX, thanks for supporting that.

Let me know if you'd like me to help with preparing or testing any patches.