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 :-)
_______________________________________________
AVR-libc-dev mailing list
AVR-libc-dev@nongnu.org
https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
