Re: [avr-libc-dev] [bug #34423] util/crc16.h: with -Os option inline functions are called causing registers value loss

[David Brown]

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 :-)