-std=gnu99 broke build on CentOS 7

[solardiz]

#5600 broke build (or rather the final linking) on our "well" as follows:

dynamic_fmt.o: In function `DynamicFunc__crypt_md5':
/home/solar/j/bleeding-jumbo-20241211/src/dynamic_fmt.c:5274: undefined reference to `MD5_body_for_thread'
dynamic_fmt.o: In function `DynamicFunc__POCrypt':
/home/solar/j/bleeding-jumbo-20241211/src/dynamic_fmt.c:5373: undefined reference to `MD5_body_for_thread'
dynamic_fmt.o: In function `DynamicFunc__crypt2_md5':
/home/solar/j/bleeding-jumbo-20241211/src/dynamic_fmt.c:5416: undefined reference to `MD5_body_for_thread'
dynamic_fmt.o: In function `DynamicFunc__crypt_md5_in1_to_out2':
/home/solar/j/bleeding-jumbo-20241211/src/dynamic_fmt.c:5500: undefined reference to `MD5_body_for_thread'
dynamic_fmt.o: In function `DynamicFunc__crypt_md5_in2_to_out1':
/home/solar/j/bleeding-jumbo-20241211/src/dynamic_fmt.c:5586: undefined reference to `MD5_body_for_thread'
dynamic_fmt.o:/home/solar/j/bleeding-jumbo-20241211/src/dynamic_fmt.c:5674: more undefined references to `MD5_body_for_thread' follow

I've tried adding -std=gnu99 also at link time, but this made no difference.

gcc version 4.8.5 20150623 (Red Hat 4.8.5-28) (GCC) 

[solardiz]

I've confirmed that b1d063a is the exact commit that causes this breakage. Checking out 6433323 (the previous commit) builds fine. Edit: Also, git revert b1d063a3d5da9abb66ed434a34d9e679ccc8fb18 on our latest tree as of today makes it build again.

[magnumripper]

I was convinced this would make it work (while figuring out why)

diff --git a/src/common.h b/src/common.h
index a0b7535..2a37e13 100644
--- a/src/common.h
+++ b/src/common.h
@@ -60,8 +60,6 @@
 #else
 #define MAYBE_INLINE __inline__
 #endif
-#elif __STDC_VERSION__ >= 199901L
-#define MAYBE_INLINE inline
 #else
 #define MAYBE_INLINE
 #endif

But it doesn't make any difference.

[magnumripper]

TIL that the idea of the C99 way of doing this, is you should put the definition of the inline function in the header file:

inline void func(...)
{
    ... actual code ...
}

and only a declaration (adding extern) in the corresponding .c file:

extern inline void func(...);

That last thing triggers external linkage.

It sounds backwards until you think again: Any file sourcing the header can get the full function for inlining, but if they don't inline it, the extern declaration ensured there's linkage. I never understood this until now.

[solardiz]

FWIW, the Linux kernel appears to do this backwards - their only uses of extern inline are in header files, but they redefine inline to mean pre-C99 __gnu_inline:

/*
 * Prefer gnu_inline, so that extern inline functions do not emit an
 * externally visible function. This makes extern inline behave as per gnu89
 * semantics rather than c99. This prevents multiple symbol definition errors
 * of extern inline functions at link time.
 * A lot of inline functions can cause havoc with function tracing.
 */
#define inline inline __gnu_inline __inline_maybe_unused notrace

... and there I was hoping to use Linux as our example of how to do it right. Well, it may be right for what's needed there, but not for a more portable project such as ours.

[solardiz]

Linux kernel appears to do this backwards - their only uses of extern inline are in header files, but they redefine inline

Also, they have extern inline directly followed by the function body. They do not use separate inline for definition + extern inline for declaration as in @magnumripper's comment above. I don't know if that shortcut is portable.

[solardiz]

@magnumripper I think you may partially misunderstand the extern line in .c file thing. I think it was only needed for MD5_body_for_thread because dynamic_types.h has it as extern without including a header file that would have it. I think this extern inline thing is not strictly required as a defense against the compiler just choosing not to inline despite of the inline keyword - that would work just fine anyhow when the right header file with the function definition is included where the function is needed (edit: but would be wasteful if this happens more than once in different translation units including that header file).

MD5_body_for_thread is neither defined nor declared in a header file other than dynamic_types.h. It is defined solely in a C file, MD5_std.c. So its reuse in dynamic_types.h was a hack and it remains a hack now.