|
From: | David Brown |
Subject: | Re: [avr-libc-dev] [bug #34423] util/crc16.h: with -Os option inline functions are called causing registers value loss |
Date: | Thu, 29 Sep 2011 09:45:31 +0200 |
User-agent: | Mozilla/5.0 (Windows NT 5.1; rv:6.0.1) Gecko/20110830 Thunderbird/6.0.1 |
On 28/09/2011 22:33, Karol Grzybowski wrote:
URL: <http://savannah.nongnu.org/bugs/?34423> Summary: util/crc16.h: with -Os option inline functions are called causing registers value loss Project: AVR C Runtime Library Submitted by: karol_grzybowski Submitted on: Wed 28 Sep 2011 08:33:29 PM GMT Category: Header Severity: 3 - Normal Priority: 5 - Normal Item Group: libc code Status: None Percent Complete: 0% Assigned to: None Open/Closed: Open Discussion Lock: Any Release: 1.7.1 Fixed Release: None _______________________________________________________ Details: gcc 4.5.1, AVR_8_bit_GNU_Toolchain_3.2.3_315 gcc options: -funsigned-char -funsigned-bitfields -Os -fpack-struct -fshort-enums -Wall -c -std=gnu99 -mmcu=atmega16 function: static __inline__ uint8_t _crc_ibutton_update(uint8_t __crc, uint8_t __data) { uint8_t __i, __pattern; __asm__ __volatile__ ( " eor %0, %4" "\n\t" " ldi %1, 8" "\n\t" " ldi %2, 0x8C" "\n\t" "1: lsr %0" "\n\t" " brcc 2f" "\n\t" " eor %0, %2" "\n\t" "2: dec %1" "\n\t" " brne 1b" "\n\t" : "=r" (__crc), "=d" (__i), "=d" (__pattern) : "0" (__crc), "r" (__data)); return __crc; } should be always inlined. In my code reading DS18B20 temperature: // LSB of temp data = ds_read_byte(); temp = (int16_t)data; crc = _crc_ibutton_update(0, data); // MSB of temp data = ds_read_byte(); temp |= (int16_t)(data)<< 8; crc = _crc_ibutton_update(crc, data); doesn't and that's why the MSB of temp is lost: 0e 94 44 00 call 0x88 ; 0x88<_crc_ibutton_update> Adding fallowing line to header seems to be the simplest solution of this issue: static __inline__ uint8_t _crc_ibutton_update(uint8_t __crc, uint8_t __data) __attribute__((always_inline)); Probably the same problem may occur with others inline Assembly.
Using "__inline__" is a hint to the compiler, it is not a command. The compiler is free to implement the call as a normal function call even if it is declared "inline", and free to inline the function call even if it is /not/ "inline". When deciding on the inline heuristics, it will take the hint into account, along things like optimisation levels, and usage patterns. You can use the "-Winline" flag to give you some information about why an "inline" function is not inlined. With "-Os", the compiler is much less inclined to inline code than with "-O2". It is a known problem with gcc, and avr-gcc in particular, that the heuristics for determining the size cost of inlining is not accurate (it is especially difficult for the compiler to predict how it will affect the size of the rest of the code in the calling function). While this has improved in later versions of gcc, it is still useful to use "always_inline" when you know better than the compiler.
<http://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/Warning-Options.html#index-Winline-445>__attribute__((always_inline)), on the other hand, /is/ a command - the compiler should follow it unless it has an overwhelming reason not to (and that should probably give an error message). If you are interested in forcing the compiler's inlining on code, it's worth learning about the "flatten" and "noinline" attributes as well.
If you are going to write inline assembly like this, I've a couple of hints that will make it easier and improve the maintainability of the code. First, the use of "%0", "%1", etc., within the assembly is poor style, except for very small sequences - use "%[crc]" style:
<http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html>(Perhaps the "avrlibc: Inline Assembler Cookbook" could be updated to include this, if anyone thinks it is worth the effort.)
Secondly, inline functions are not macros - you don't have to worry about namespace conflicts. You should use names like "crc", not "__crc".
You also don't need to declare your assembly code "volatile" - indeed, you should not do so unless you have good reason for it (such as side effects that the compiler doesn't know about). Your crc_ibutton_update function is a function that takes two parameters and returns a value that depends solely on those inputs, has no side effects and makes no read or write access to memory. Not only should the "volatile" be removed, but you could even declare the function with __attribute__((const)) to give the compiler more freedom to optimise.
Of course, if you have the code space (256 bytes), it's faster to do these CRC's using a lookup table :-)
[Prev in Thread] | Current Thread | [Next in Thread] |