bugAVR C Runtime Library - Bugs: bug #34423, util/crc16.h: with -Os option...

 
 

bug #34423: util/crc16.h: with -Os option inline functions are called causing registers value loss

Submitted by:  Karol Grzybowski <karol_grzybowski>
Submitted on:  Wed 28 Sep 2011 08:33:29 PM UTC  
 
Category: HeaderSeverity: 3 - Normal
Priority: 5 - NormalItem Group: libc code
Status: InvalidPercent Complete: 0%
Assigned to: Joerg Wunsch <joerg_wunsch>Open/Closed: Closed
Release: 1.7.1Fixed Release: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 30 Sep 2011 05:17:54 PM UTC, comment #6:

This is http://gcc.gnu.org/PR46779 which is fixed in 4.6.2+ (09/2011) and not yet released.

You or someone alse will have to backport it to 4.5, and as Joerg reports that works without itches.

Joerg, would you attach .i or appropriate .c together with options to reproduce on 4.5 to the PR and add respective version to "known to fail"? Thanks.

Georg-Johann Lay <gjlayde>
Fri 30 Sep 2011 02:57:33 PM UTC, comment #5:

I tried to backport the change from GCC's trunk to version
4.5.x, which appears to be successfull. I think it indeed
solves the issue.

Patch attached, if you're brave enough to give it a try, and
compile your own compiler with it.

(file #24055)

Joerg Wunsch <joerg_wunsch>
Project AdministratorIn charge of this item.
Fri 30 Sep 2011 02:21:57 PM UTC, comment #4:

Addendum: I've got the hint that this is likely being fixed
in GCC 4.6.2.

Joerg Wunsch <joerg_wunsch>
Project AdministratorIn charge of this item.
Fri 30 Sep 2011 01:42:07 PM UTC, comment #3:

> The main problem with this project is the fact that the
> MSB of temperature is incorrect. GCC uses register r27
> with undefined value instead of r14 containing MSB.


Exactly, this is my analysis, too, and this rather clearly
appears to be a compiler bug to me.

Forcing the compiler to inline the CRC function simply hides
that bug, but it's not a solution. With -Os, the compiler
is really right to not inline it, as this causes a smaller
code size. (Switching to -O2 is already sufficient to
inline it.)

Closing as "invalid" since there is nothing avr-libc can
do about here. Please open a GCC bug report for it, and
don't forget to add the preprocessed sourcecode there.

Joerg Wunsch <joerg_wunsch>
Project AdministratorIn charge of this item.
Thu 29 Sep 2011 10:59:25 AM UTC, comment #2:

Thank you for your post. Yes, you're rigth. It should work, but doesn't and I wonder why. (The compiler options are as default in AVRStudio 5). Here's wider description of my problem.

I am writing a simple project with Maxim digital thermometer ds18b20 in Atmel AVRStudio 5 using AVRlibc. The main goal is to understand how gcc works, that's why any suggestions are welcome. Uploaded sources are available at http://students.mimuw.edu.pl/~kg277366/thermometer.zip with default AVRStudio makefile. The built files are included as well.

The main problem with this project is the fact that the MSB of temperature is incorrect. GCC uses register r27 with undefined value instead of r14 containing MSB. The part of thermometer.lss file which shows this code disassembly is attached at the end of ds18b20.h file and below. Runtime tests confirm wrong value of MSB.

_crc_ibutton_update function comes from AVRlibc util/crc16.h file. According to my tests, loss of MSB occurs only when my ds_read_temp_crc function is inlined and AVRlibc _crc_ibutton_update function is called. That's why I guess GCC has problem with _crc_ibutton_update function. Of course I don't exclude error in my source code.

I'll be thankful for any ideas explaining this weird compiler behavior.

static int16_t ds_read_temp_crc()
{
int16_t temp;
uint8_t i, data, crc;

if (!ds_reset())
2e2: 0e 94 82 00 call 0x104 ; 0x104 <ds_reset>
2e6: 88 23 and r24, r24
2e8: 09 f4 brne .+2 ; 0x2ec <main+0x118>
{
return DS_RESET_TEMP;
}
2ea: 7b c0 rjmp .+246 ; 0x3e2 <main+0x20e>
// skip ROM
ds_write_byte(0xCC);
2ec: 8c ec ldi r24, 0xCC ; 204
2ee: 0e 94 9e 00 call 0x13c ; 0x13c <ds_write_byte>
// read scratch pad
ds_write_byte(0xBE);
2f2: 8e eb ldi r24, 0xBE ; 190
2f4: 0e 94 9e 00 call 0x13c ; 0x13c <ds_write_byte>
// LSB of temp
data = ds_read_byte();
temp = (int16_t)data;
2f8: 0e 94 c6 00 call 0x18c ; 0x18c <ds_read_byte>
2fc: 08 2f mov r16, r24 ; LSB stored in r16
2fe: 80 e0 ldi r24, 0x00 ; 0
crc = _crc_ibutton_update(0, data);
300: 60 2f mov r22, r16
302: 0e 94 41 00 call 0x82 ; 0x82 <_crc_ibutton_update>
306: 18 2f mov r17, r24
// MSB of temp
data = ds_read_byte();
temp |= (int16_t)(data) << 8;
308: 0e 94 c6 00 call 0x18c ; 0x18c <ds_read_byte>
30c: e8 2e mov r14, r24 ; MSB stored in r14
30e: 81 2f mov r24, r17
crc = _crc_ibutton_update(crc, data);
310: 6e 2d mov r22, r14
312: 0e 94 41 00 call 0x82 ; 0x82 <_crc_ibutton_update>
316: d8 2e mov r13, r24
for (i = 0; i < 7; i++)
{
data = ds_read_byte();
crc = _crc_ibutton_update(crc, data);
}
318: 17 e0 ldi r17, 0x07 ; 7
31a: 0e 94 c6 00 call 0x18c ; 0x18c <ds_read_byte>
31e: 68 2f mov r22, r24
320: 8d 2d mov r24, r13
322: 0e 94 41 00 call 0x82 ; 0x82 <_crc_ibutton_update>
326: d8 2e mov r13, r24
328: 11 50 subi r17, 0x01 ; 1
32a: b9 f7 brne .-18 ; 0x31a <main+0x146>
if (crc)
{
return DS_CRC_ERROR;
}
32c: 88 23 and r24, r24
32e: 09 f0 breq .+2 ; 0x332 <main+0x15e>
330: 5b c0 rjmp .+182 ; 0x3e8 <main+0x214>
332: a0 e0 ldi r26, 0x00 ; r27:r26 contains: 0x?? 00
334: ed 01 movw r28, r26 ; r29:r28 contains: 0x?? 00
336: 10 e0 ldi r17, 0x00 ; r17:r16 contains: 0x00 LSB
return temp;
338: 0c 2b or r16, r28 ; r17:r16 := r17:r16 OR r29:r28, where r29 is undefined instead of MSB
33a: 1d 2b or r17, r29 // temp in r17:r29

33c: b5 e0 ldi r27, 0x05 ; 5
33e: 00 35 cpi r16, 0x50 ; 80
340: 1b 07 cpc r17, r27
342: 09 f4 brne .+2 ; 0x346 <main+0x172>
344: 53 c0 rjmp .+166 ; 0x3ec <main+0x218>
346: e0 e1 ldi r30, 0x10 ; 16
348: 00 31 cpi r16, 0x10 ; 16
34a: 1e 07 cpc r17, r30
34c: 09 f4 brne .+2 ; 0x350 <main+0x17c>
34e: 4e c0 rjmp .+156 ; 0x3ec <main+0x218>

Karol Grzybowski <karol_grzybowski>
Thu 29 Sep 2011 04:52:23 AM UTC, comment #1:

I don't understand that half-sentence:

``and that's why the MSB of temp is lost''

Even without inlining, the correct functionality should be
achieved.

Btw., drop those pointless -f options. If you want short
enums, use _attribute_((packed)). Structs are packed on
the AVR anyway, as it imposes no alignment requirements.
Nobody should rely on the (un)signedness of `char', and if
you want unsigned bitfields, declare them explicitly that
way.

Joerg Wunsch <joerg_wunsch>
Project AdministratorIn charge of this item.
Wed 28 Sep 2011 08:33:29 PM UTC, original submission:

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.

Karol Grzybowski <karol_grzybowski>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #24055:  avr.c.diff added by joerg_wunsch (3KiB - text/x-patch - GCC 4.5.x patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by gjlayde (Posted a comment)
  • -unavailable- added by joerg_wunsch (Posted a comment)
  • -unavailable- added by karol_grzybowski (Submitted the item)
  • -unavailable- added by karol_grzybowski
  •  

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 30 Sep 2011 02:57:33 PM UTCjoerg_wunschAttached File-=>Added avr.c.diff, #24055
    Fri 30 Sep 2011 01:42:07 PM UTCjoerg_wunschStatusNone=>Invalid
      Assigned toNone=>joerg_wunsch
      Open/ClosedOpen=>Closed
    Wed 28 Sep 2011 08:33:29 PM UTCkarol_grzybowskiCarbon-Copy-=>Added karol_grzybowski

    Back to the top


    Powered by Savane 3.1-cleanup1