[PATCH] Fix inline functions to use 'static inline'
With just 'inline', if the compiler decides not to inline them, it isn't required to emit them at all. For some targets with -Os that is causing build failures. Perhaps we might consider using '__attribute__((always_inline))' for GCC builds, but 'static inline' is a good start. --- src/rtl_adsb.c | 8 ++++---- src/rtl_power.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rtl_adsb.c b/src/rtl_adsb.c index 9087de4..7aea8dd 100644 --- a/src/rtl_adsb.c +++ b/src/rtl_adsb.c @@ -183,7 +183,7 @@ int magnitute(uint8_t *buf, int len) return len/2; } -inline uint16_t single_manchester(uint16_t a, uint16_t b, uint16_t c, uint16_t d) +static inline uint16_t single_manchester(uint16_t a, uint16_t b, uint16_t c, uint16_t d) /* takes 4 consecutive real samples, return 0 or 1, BADSAMPLE on error */ { int bit, bit_p; @@ -224,17 +224,17 @@ inline uint16_t single_manchester(uint16_t a, uint16_t b, uint16_t c, uint16_t d return BADSAMPLE; } -inline uint16_t min16(uint16_t a, uint16_t b) +static inline uint16_t min16(uint16_t a, uint16_t b) { return a<b ? a : b; } -inline uint16_t max16(uint16_t a, uint16_t b) +static inline uint16_t max16(uint16_t a, uint16_t b) { return a>b ? a : b; } -inline int preamble(uint16_t *buf, int i) +static inline int preamble(uint16_t *buf, int i) /* returns 0/1 for preamble at index i */ { int i2; diff --git a/src/rtl_power.c b/src/rtl_power.c index 00f4d9f..625d818 100644 --- a/src/rtl_power.c +++ b/src/rtl_power.c @@ -250,7 +250,7 @@ void sine_table(int size) } } -inline int16_t FIX_MPY(int16_t a, int16_t b) +static inline int16_t FIX_MPY(int16_t a, int16_t b) /* fixed point multiply and scale */ { int c = ((int)a * (int)b) >> 14; -- 2.17.1
Hi David, On 28.06.2018 17:43, David Woodhouse wrote:
With just 'inline', if the compiler decides not to inline them, it isn't required to emit them at all. For some targets with -Os that is causing build failures.
"It isn't required to emit them at all" - What the heck, which compiler on earth does such horrible things? I've taken at look at the C99 standard for the function specifier 'inline' and there is nothing that would justify such behaviour. Of course, if that's a bug with a specific compiler version we can merge that, but the explanation in the commit doesn't make any sense to me. Regards, Steve
Hi all, althought the argument about compiller is a bit strange the change is in my opinion in right course. If the goal is to use those functions only in this one particular .c file and not elsewhere, the good practice is to limit scope by using static. Can You please let us know how to reproduce behaviour You have described? with best regards, Pinky. * Steve Markgraf <steve@steve-m.de> [2018-06-28 20:29:34 +0200]:
Hi David,
On 28.06.2018 17:43, David Woodhouse wrote:
With just 'inline', if the compiler decides not to inline them, it isn't required to emit them at all. For some targets with -Os that is causing build failures.
"It isn't required to emit them at all" - What the heck, which compiler on earth does such horrible things?
I've taken at look at the C99 standard for the function specifier 'inline' and there is nothing that would justify such behaviour.
Of course, if that's a bug with a specific compiler version we can merge that, but the explanation in the commit doesn't make any sense to me.
Regards, Steve
Hi David, Pinky, Steve and all, I'd agree with "uh, I need to know where this goes wrong please, I'm scared". I'd also agree, an inlined function should probably have compilation unit scope, anyway, so `static` would be appropriate. I've taken a look at the patched function declarations and would recommend to just remove `min16` and `max16` alltogether (only used in a comment, and frankly, not that great a function `(a<>b?a:b)`). I'd really like to know the compile error! Maybe there's actually something we can do – simply because this really *shouldn't* fail, imho. Best regards, Marcus On Thu, 2018-06-28 at 20:50 +0200, Pinky wrote:
Hi all,
althought the argument about compiller is a bit strange the change is in my opinion in right course. If the goal is to use those functions only in this one particular .c file and not elsewhere, the good practice is to limit scope by using static.
Can You please let us know how to reproduce behaviour You have described?
with best regards, Pinky.
* Steve Markgraf <steve@steve-m.de> [2018-06-28 20:29:34 +0200]:
Hi David,
On 28.06.2018 17:43, David Woodhouse wrote:
With just 'inline', if the compiler decides not to inline them, it isn't required to emit them at all. For some targets with -Os that is causing build failures.
"It isn't required to emit them at all" - What the heck, which compiler on earth does such horrible things?
I've taken at look at the C99 standard for the function specifier 'inline' and there is nothing that would justify such behaviour.
Of course, if that's a bug with a specific compiler version we can merge that, but the explanation in the commit doesn't make any sense to me.
Regards, Steve
On Fri, 2018-06-29 at 08:25 +0000, Müller, Marcus (CEL) wrote:
Hi David, Pinky, Steve and all,
I'd agree with "uh, I need to know where this goes wrong please, I'm scared".
GCC just fails to emit the offending functions and then the link fails: [ 36%] Linking C executable rtl_power CMakeFiles/rtl_power.dir/rtl_power.c.o: In function `fix_fft': rtl_power.c:(.text+0x41a): undefined reference to `FIX_MPY' rtl_power.c:(.text+0x445): undefined reference to `FIX_MPY' rtl_power.c:(.text+0x476): undefined reference to `FIX_MPY' rtl_power.c:(.text+0x49d): undefined reference to `FIX_MPY' collect2: error: ld returned 1 exit status You should be able to reproduce this really easily by building with -O0. Admittedly it's fairly bloody stupid to use -O0 for rtl-sdr but for the MIPS target this was even happening with -Os, which is slightly saner.
True, can reproduce. Reading the issue and the linked "differences in C99" page, this all makes sense. Thanks for enlightening me! Best regards, Marcus On Fri, 2018-06-29 at 10:11 +0100, David Woodhouse wrote:
On Fri, 2018-06-29 at 08:25 +0000, Müller, Marcus (CEL) wrote:
Hi David, Pinky, Steve and all,
I'd agree with "uh, I need to know where this goes wrong please, I'm scared".
GCC just fails to emit the offending functions and then the link fails:
[ 36%] Linking C executable rtl_power CMakeFiles/rtl_power.dir/rtl_power.c.o: In function `fix_fft': rtl_power.c:(.text+0x41a): undefined reference to `FIX_MPY' rtl_power.c:(.text+0x445): undefined reference to `FIX_MPY' rtl_power.c:(.text+0x476): undefined reference to `FIX_MPY' rtl_power.c:(.text+0x49d): undefined reference to `FIX_MPY' collect2: error: ld returned 1 exit status
You should be able to reproduce this really easily by building with -O0.
Admittedly it's fairly bloody stupid to use -O0 for rtl-sdr but for the MIPS target this was even happening with -Os, which is slightly saner.
On Thu, 2018-06-28 at 20:29 +0200, Steve Markgraf wrote:
Hi David,
On 28.06.2018 17:43, David Woodhouse wrote:
With just 'inline', if the compiler decides not to inline them, it isn't required to emit them at all. For some targets with -Os that is causing build failures.
"It isn't required to emit them at all" - What the heck, which compiler on earth does such horrible things?
I've taken at look at the C99 standard for the function specifier 'inline' and there is nothing that would justify such behaviour.
Yeah, you're probably right. My initial reading of the docs was a little hasty and mostly based on the assumption (haha) that GCC would be doing the right thing for something like this, given all the churn that went on some years ago with static/extern inline handling. This was seen in the OpenWRT build. https://github.com/openwrt/packages/pull/6377 Of course, I question whether we should be building rtl-sdr with -O0 or -Os and eschewing inlining... that does seem a bit daft for this *particular* piece of software even if the general policy of the OpenWRT is to reduce size. But that's a separate discussion, really.
Of course, if that's a bug with a specific compiler version we can merge that, but the explanation in the commit doesn't make any sense to me.
Agreed. Here's another example FWIW: $ cat foo.c inline int foo(int a) { return a+1; } int bar(int a) { return foo(a); } $ gcc -O0 -o- -S foo.c .file "foo.c" .text .globl bar .type bar, @function bar: .LFB1: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset 6, -16 movq %rsp, %rbp .cfi_def_cfa_register 6 subq $16, %rsp movl %edi, -4(%rbp) movl -4(%rbp), %eax movl %eax, %edi call foo leave .cfi_def_cfa 7, 8 ret .cfi_endproc .LFE1: .size bar, .-bar .ident "GCC: (GNU) 8.1.1 20180502 (Red Hat 8.1.1-1)" .section .note.GNU-stack,"",@progbits
On Thu, 2018-06-28 at 20:29 +0200, Steve Markgraf wrote:
Hi David,
On 28.06.2018 17:43, David Woodhouse wrote:
With just 'inline', if the compiler decides not to inline them, it isn't required to emit them at all. For some targets with -Os that is causing build failures.
"It isn't required to emit them at all" - What the heck, which compiler on earth does such horrible things?
I've taken at look at the C99 standard for the function specifier 'inline' and there is nothing that would justify such behaviour.
Of course, if that's a bug with a specific compiler version we can merge that, but the explanation in the commit doesn't make any sense to me.
participants (4)
-
David Woodhouse -
Müller, Marcus (CEL) -
Pinky -
Steve Markgraf