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

 
 

You are not allowed to post comments on this tracker with your current authentication level.

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

Submitter:  Karol Grzybowski <karol_grzybowski>
Submitted:  Wed 28 Sep 2011 08:33:29 PM UTC
   
 
Category:  Header Severity:  3 - Normal
Priority:  5 - Normal Item Group:  libc code
Status:  Invalid Assigned to:  joerg_wunsch
Percent Complete:  0% Open/Closed:  Closed
Release:  1.7.1 Fixed Release:  None

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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group administrator
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.)

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
  • -email is unavailable- added by gjlayde (Posted a comment)
  • -email is unavailable- added by joerg_wunsch (Posted a comment)
  • -email is unavailable- added by karol_grzybowski (Submitted the item)
  • -email is unavailable- added by karol_grzybowski
  •  

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-09-30 joerg_wunsch Attached File- Added avr.c.diff, #24055
    2011-09-30 joerg_wunsch StatusNone Invalid
        Assigned toNone joerg_wunsch
        Open/ClosedOpen Closed
    2011-09-28 karol_grzybowski Carbon-Copy- Added karol_grzybowski

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code