mackron / dr_libs

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

Use of “int32_t” type causes build failure on ARMv6 #193

Closed musicinmybrain closed 3 years ago

musicinmybrain commented 3 years ago

In dr_mp3.h, the int32_t type is used on ARMv6:

#if defined(__ARM_ARCH) && (__ARM_ARCH >= 6) && !defined(__aarch64__) && !defined(_M_ARM64)
#define DRMP3_HAVE_ARMV6 1
static __inline__ __attribute__((always_inline)) drmp3_int32 drmp3_clip_int16_arm(int32_t a)
{
    drmp3_int32 x = 0;
    __asm__ ("ssat %0, #16, %1" : "=r"(x) : "r"(a));
    return x;
}
#else
#define DRMP3_HAVE_ARMV6 0
#endif

Since the library is C89 without stdint.h/inttypes.h, this type is unknown and the build fails:

In file included from ./tests/mp3/dr_mp3_test_0.c:2:
./tests/mp3/../../dr_mp3.h:704:83: error: unknown type name 'int32_t'
  704 | static __inline__ __attribute__((always_inline)) drmp3_int32 drmp3_clip_int16_arm(int32_t a)
      |                                                                                   ^~~~~~~
In file included from ./tests/mp3/dr_mp3_test_0.c:2:
./tests/mp3/../../dr_mp3.h: In function 'drmp3d_scale_pcm':
./tests/mp3/../../dr_mp3.h:1937:22: warning: implicit declaration of function 'drmp3_clip_int16_arm' [-Wimplicit-function-declaration]
 1937 |     s = (drmp3_int16)drmp3_clip_int16_arm(s32);
      |                      ^~~~~~~~~~~~~~~~~~~~

The appropriate fix seems to be to replace the int32_t parameter with an drmp3_int32 one; this type is already used in the function body and the function return value. I have confirmed this solves the problem, so I will follow up with a PR.

sezero commented 3 years ago

Out of curiosity, which library is used ?

musicinmybrain commented 3 years ago

This was on Fedora Rawhide (development version) with gcc 11.1.1 and glibc 2.33.9000 (a pre-relase for glibc 2.34), and -std=c89 to verify C89 compatibility.

The fact that dr_mp3.h uses int32_t without otherwise requiring C99 support and without including stdint.h or inttypes.h is an issue regardless of the particular platform. There might exist some platforms where you can get away with it, especially in C99 mode. For example, on some platforms stddef.h, stdlib.h, or limits.h might recursively include some header that defines the int32_t type.

Since there is already a drmp3_int32 type in the header, and the affected function already uses it elsewhere, this seems like an obvious remedy with no disadvantages.

sezero commented 3 years ago

OK I see, then the initial comment is misleading: glibc obviously can do c99+, only thing is that dr_mp3.h isn't including stdint.h.

musicinmybrain commented 3 years ago

Ok, I see the misunderstanding. When I say “the library is C89,” I mean the library dr_mp3 is targeting C89 compatibility without assuming C99 or platform extensions. I say that based on the fact that it already puts significant effort into defining fixed-length integer types without including C99 headers stdint.h or inttypes.h, and based on the practice of compiling test code as C89 in https://github.com/mackron/dr_libs/blob/master/tests/build_flac_linux.sh and similar.

mackron commented 3 years ago

Thanks for the report and pull request. I've merged all of your PRs into the dev branch and will release to the master branch soon.