mackron / dr_libs

Audio decoding libraries for C/C++, each in a single source file.
Other
1.24k stars 205 forks source link

tweak DRFLAC_INLINE macro for gcc: #230

Closed sezero closed 2 years ago

sezero commented 2 years ago
mackron commented 2 years ago

Thanks. This looks good, but just to clarify, for versions of GCC that support always_inline, that'll still be used? Where supported by the compiler, I specifically need to enforce inlining rather than letting the compiler decide because it makes some bad decisions in some inner decoding functionality that needs inlining for performance. I don't think __inline__ enforces it (it's more just a hint), but obviously if always_inline is not supported by the compiler I'm happy to use it as the fallback.

sezero commented 2 years ago

f or versions of GCC that support always_inline, that'll still be used?

Of course

sezero commented 2 years ago

I don't think __inline__ enforces it (it's more just a hint)

I don't think that's correct: IIRC it's pre-c99 gcc extension for inline, but I'd like to be proven otherwise.

sezero commented 2 years ago

I don't think __inline__ enforces it (it's more just a hint)

I don't think that's correct: IIRC it's pre-c99 gcc extension for inline, but I'd like to be proven otherwise.

BTW, SDL uses the __inline__ version too: https://github.com/libsdl-org/SDL/blob/main/include/begin_code.h#L118 https://github.com/libsdl-org/SDL/blob/main/include/begin_code.h#L141

mackron commented 2 years ago

I know for a fact that one of the inline keywords used by GCC is just a hint because I encountered a situation first hand where it wasn't inlining when I was optimizing dr_flac. I think that's why you need the always_inline in the first place. Regardless, it doesn't matter for the purpose of this PR so long as always_inline is used when available.

That said, there's one thing that needs to change. This point you made here:

inline is always available in gcc, therefore always use it instead of checking for __STRICT_ANSI__.

As per the comment in the code, I've had a report where there are cases where __inline__ is actually defined to nothing which then results in the compiler throwing a warning about a function not being inlinable. Exactly what the comment in the code says:

I've had a bug report where GCC is emitting warnings about functions possibly not being inlineable. This warning happens when the __attribute__((always_inline)) attribute is defined without an "inline" statement. I think therefore there must be some case where __inline__ is not always defined, thus the compiler emitting these warnings.

Unfortunately I can't find the original bug report for reference (it was probably from another project), but I do remember it and I need to trust that comment and keep the original logic in place for that part. The fall back to omit always_inline when it isn't available is a good change, but the logic to use inline instead of __inline__ with __STRICT_ANSI__ needs to stay in place.

sezero commented 2 years ago

If you really insist, I can add back the __STRICT_ANSI__ stuff. Note however: in ansi-only mode, inline is not available but __inline and __inline__ are always available in gcc, so are you sure?

mackron commented 2 years ago

It wasn't that __inline__ wasn't available, it was. It's just that it was defined as the equivalent to this:

#define __inline__

So it was essentially just placeholder, probably for some compatibility reason. Problem is, when always_inline is used without an explicit "inline" statement (either inline or __inline__), the compiler will throw a warning about the function not being inlined (or something like that - can't remember the exact wording). Granted, I have a feeling this particular user was probably doing something atypical with their compiler set up, but this fixed their issue and I don't particularly want to re-introduce it again.

sezero commented 2 years ago

Tried the following, __inline__ is there:

$ cat 0.c 
__inline__ int foo (void) {
  return 1;
}

$ gcc -c -xc -ansi -std=c89 0.c
 # <-- NO WARNINGS, ETC

$ gcc -E -dD -xc -ansi -std=c89 /dev/null 0.c
# 1 "0.c"
# 1 "<built-in>"
#define __STDC__ 1
#define __STDC_HOSTED__ 1
#define __GNUC__ 4
#define __GNUC_MINOR__ 4
#define __GNUC_PATCHLEVEL__ 7
#define __GNUC_RH_RELEASE__ 23
#define __SIZE_TYPE__ unsigned int
#define __PTRDIFF_TYPE__ int
#define __WCHAR_TYPE__ long int
#define __WINT_TYPE__ unsigned int
#define __INTMAX_TYPE__ long long int
#define __UINTMAX_TYPE__ long long unsigned int
#define __CHAR16_TYPE__ short unsigned int
#define __CHAR32_TYPE__ unsigned int
#define __GXX_ABI_VERSION 1002
#define __SCHAR_MAX__ 127
#define __SHRT_MAX__ 32767
#define __INT_MAX__ 2147483647
#define __LONG_MAX__ 2147483647L
#define __LONG_LONG_MAX__ 9223372036854775807LL
#define __WCHAR_MAX__ 2147483647
#define __CHAR_BIT__ 8
#define __INTMAX_MAX__ 9223372036854775807LL
#define __FLT_EVAL_METHOD__ 2
#define __DEC_EVAL_METHOD__ 2
#define __FLT_RADIX__ 2
#define __FLT_MANT_DIG__ 24
#define __FLT_DIG__ 6
#define __FLT_MIN_EXP__ (-125)
#define __FLT_MIN_10_EXP__ (-37)
#define __FLT_MAX_EXP__ 128
#define __FLT_MAX_10_EXP__ 38
#define __FLT_MAX__ 3.40282347e+38F
#define __FLT_MIN__ 1.17549435e-38F
#define __FLT_EPSILON__ 1.19209290e-7F
#define __FLT_DENORM_MIN__ 1.40129846e-45F
#define __FLT_HAS_DENORM__ 1
#define __FLT_HAS_INFINITY__ 1
#define __FLT_HAS_QUIET_NAN__ 1
#define __DBL_MANT_DIG__ 53
#define __DBL_DIG__ 15
#define __DBL_MIN_EXP__ (-1021)
#define __DBL_MIN_10_EXP__ (-307)
#define __DBL_MAX_EXP__ 1024
#define __DBL_MAX_10_EXP__ 308
#define __DBL_MAX__ 1.7976931348623157e+308
#define __DBL_MIN__ 2.2250738585072014e-308
#define __DBL_EPSILON__ 2.2204460492503131e-16
#define __DBL_DENORM_MIN__ 4.9406564584124654e-324
#define __DBL_HAS_DENORM__ 1
#define __DBL_HAS_INFINITY__ 1
#define __DBL_HAS_QUIET_NAN__ 1
#define __LDBL_MANT_DIG__ 64
#define __LDBL_DIG__ 18
#define __LDBL_MIN_EXP__ (-16381)
#define __LDBL_MIN_10_EXP__ (-4931)
#define __LDBL_MAX_EXP__ 16384
#define __LDBL_MAX_10_EXP__ 4932
#define __DECIMAL_DIG__ 21
#define __LDBL_MAX__ 1.18973149535723176502e+4932L
#define __LDBL_MIN__ 3.36210314311209350626e-4932L
#define __LDBL_EPSILON__ 1.08420217248550443401e-19L
#define __LDBL_DENORM_MIN__ 3.64519953188247460253e-4951L
#define __LDBL_HAS_DENORM__ 1
#define __LDBL_HAS_INFINITY__ 1
#define __LDBL_HAS_QUIET_NAN__ 1
#define __DEC32_MANT_DIG__ 7
#define __DEC32_MIN_EXP__ (-94)
#define __DEC32_MAX_EXP__ 97
#define __DEC32_MIN__ 1E-95DF
#define __DEC32_MAX__ 9.999999E96DF
#define __DEC32_EPSILON__ 1E-6DF
#define __DEC32_SUBNORMAL_MIN__ 0.000001E-95DF
#define __DEC64_MANT_DIG__ 16
#define __DEC64_MIN_EXP__ (-382)
#define __DEC64_MAX_EXP__ 385
#define __DEC64_MIN__ 1E-383DD
#define __DEC64_MAX__ 9.999999999999999E384DD
#define __DEC64_EPSILON__ 1E-15DD
#define __DEC64_SUBNORMAL_MIN__ 0.000000000000001E-383DD
#define __DEC128_MANT_DIG__ 34
#define __DEC128_MIN_EXP__ (-6142)
#define __DEC128_MAX_EXP__ 6145
#define __DEC128_MIN__ 1E-6143DL
#define __DEC128_MAX__ 9.999999999999999999999999999999999E6144DL
#define __DEC128_EPSILON__ 1E-33DL
#define __DEC128_SUBNORMAL_MIN__ 0.000000000000000000000000000000001E-6143DL
#define __REGISTER_PREFIX__ 
#define __USER_LABEL_PREFIX__ 
#define __VERSION__ "4.4.7 20120313 (Red Hat 4.4.7-23)"
#define __GNUC_GNU_INLINE__ 1
#define __NO_INLINE__ 1
#define __FINITE_MATH_ONLY__ 0
#define __STRICT_ANSI__ 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
#define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1
#define __SIZEOF_INT__ 4
#define __SIZEOF_LONG__ 4
#define __SIZEOF_LONG_LONG__ 8
#define __SIZEOF_SHORT__ 2
#define __SIZEOF_FLOAT__ 4
#define __SIZEOF_DOUBLE__ 8
#define __SIZEOF_LONG_DOUBLE__ 12
#define __SIZEOF_SIZE_T__ 4
#define __SIZEOF_WCHAR_T__ 4
#define __SIZEOF_WINT_T__ 4
#define __SIZEOF_PTRDIFF_T__ 4
#define __SIZEOF_POINTER__ 4
#define __i386 1
#define __i386__ 1
#define __i686 1
#define __i686__ 1
#define __pentiumpro 1
#define __pentiumpro__ 1
#define __gnu_linux__ 1
#define __linux 1
#define __linux__ 1
#define __unix 1
#define __unix__ 1
#define __ELF__ 1
#define __DECIMAL_BID_FORMAT__ 1
#define __BIGGEST_ALIGNMENT__ 16
# 1 "<command-line>"
# 1 "0.c"
__inline__ int foo (void) {
  return 1;
}
sezero commented 2 years ago

The only thing I can think of is, the user specifically defining __inline__ as an empty macro, maybe with a cmdline switch like -D__inline__=, which would be most weird because it means that he/she is fighting the compiler..

mackron commented 2 years ago

I've gone ahead and manually applied this change, the difference being that I'm keeping that __STRICT_ANSI__ logic we were talking about.

Just a quick clarification. I noticed in your comment that you mentioned version >= 3.1, whereas your code change implies >= 3.2:

(__GNUC__ == 3 && __GNUC_MINOR__ >= 2)

Could you clarify which one of those is correct? I tried a quick internet search but wasn't able to find any info.

Thanks for reporting this one.

sezero commented 2 years ago

Just a quick clarification. I noticed in your comment that you mentioned version >= 3.1, whereas your code change implies >= 3.2:

The macro check is correct, the text note is my typo (sorry.)

mackron commented 2 years ago

Thanks for clarifying that. I've gone ahead and released this fix. Thanks for the report.