Black Lives Matter - Action and Equality.
0

gcc hates me and how i know
Moderators: adafruit_support_bill, adafruit

Please be positive and constructive with your questions and comments.

gcc hates me and how i know

by mtbf0 on Wed Jun 25, 2008 10:56 pm

i'm working on an boarduino brain machine. the code to generate the binaural beats is similar to this project. i spent two weeks trying to figure out a really peculiar bug and today after printing out some strange intermediate results the damnedest thing turned up.

given the following declarations for a bunch of 24 bit fixed point values...
Code: Select all | TOGGLE FULL SIZE
struct notestep{                // type for a 24 bit fixed point value
  uint8_t integer;
  uint16_t fraction;
};

const notestep basestep = { 3, 0x46dc };    // 200Hz
notestep offsetstep = { 3, 0x70cd };      // 210Hz
notestep baseaccumulator;
notestep offsetaccumulator;


this works...

Code: Select all | TOGGLE FULL SIZE
  offsetaccumulator.fraction += offsetstep.fraction;
  if (SREG & (1 << SREG_C))        // check for carry
    offsetaccumulator.integer++;
  offsetaccumulator.integer += offsetstep.integer;


and this doesn't ...

Code: Select all | TOGGLE FULL SIZE
  baseaccumulator.fraction += basestep.fraction;
  if (SREG & (1 << SREG_C))        // check for carry
    baseaccumulator.integer++;
  baseaccumulator.integer += basestep.integer;


i finally printed out enough intermediate values to figure out that the carries were not happening when they should, so i disassembled the binary and it turned out that with baseoffset declared as a constant the addition compiles to a subtraction, as if i had written ...

Code: Select all | TOGGLE FULL SIZE
  baseaccumulator.fraction -= -(0x46dc);
  if (SREG & (1 << SREG_C))       // check for carry
    baseaccumulator.integer++;
  baseaccumulator.integer += basestep.integer;


... and the damned carry flag is not set when i expect it to be.

removing the const attribute from the baseoffset declaration fixed things.

now, maybe i can start working on some real bugs.
"i want to lead a dissipate existence, play scratchy records and enjoy my decline" - iggy pop, i need more
User avatar
mtbf0
 
Posts: 1645
Joined: Sat Nov 10, 2007 12:59 am
Location: oakland ca

by mtbf0 on Thu Jun 26, 2008 10:54 pm

i figured out what's happening.

checked the mega168 datasheet and it turns out that it has subtract immediate instructions, but no add immediate instructions, so if you want to add a constant to a number gcc will replace the addition with a subtraction.

in this example, offsetstep is declared without a const attribute. note that it compiles to 4 load instructions, 2 register to register adds and 2 stores.

Code: Select all | TOGGLE FULL SIZE
    offsetaccumulator.fraction += offsetstep.fraction;
 338:   20 91 1a 01     lds     r18, 0x011A
 33c:   30 91 1b 01     lds     r19, 0x011B
 340:   80 91 20 01     lds     r24, 0x0120
 344:   90 91 21 01     lds     r25, 0x0121
 348:   28 0f           add     r18, r24
 34a:   39 1f           adc     r19, r25
 34c:   30 93 21 01     sts     0x0121, r19
 350:   20 93 20 01     sts     0x0120, r18


basestep in this example has been declared with the const attribute. it's value is 0x46dc. instead of adding 0x46dc, gcc decides to subtract 0xb924 using subtract immediate instructions. this eliminates 2 register loads saving 8 bytes of program space and breaks my code.

Code: Select all | TOGGLE FULL SIZE
    baseaccumulator.fraction += basestep.fraction;
 392:   20 91 1d 01     lds     r18, 0x011D
 396:   30 91 1e 01     lds     r19, 0x011E
 39a:   24 52           subi    r18, 0x24       ; 36
 39c:   39 4b           sbci    r19, 0xB9       ; 185
 39e:   30 93 1e 01     sts     0x011E, r19
 3a2:   20 93 1d 01     sts     0x011D, r18


the result is the same as it would have been if it had compiled to an addition except for the state of the carry flag. which i'm about to check. rats.

removing the const from the basestep declaration gets me this...

Code: Select all | TOGGLE FULL SIZE
    baseaccumulator.fraction += basestep.fraction;
 392:   20 91 1f 01     lds     r18, 0x011F
 396:   30 91 20 01     lds     r19, 0x0120
 39a:   80 91 11 01     lds     r24, 0x0111
 39e:   90 91 12 01     lds     r25, 0x0112
 3a2:   28 0f           add     r18, r24
 3a4:   39 1f           adc     r19, r25
 3a6:   30 93 20 01     sts     0x0120, r19
 3aa:   20 93 1f 01     sts     0x011F, r18


and the carry flag gets set when it should and all is right with the world.
"i want to lead a dissipate existence, play scratchy records and enjoy my decline" - iggy pop, i need more
User avatar
mtbf0
 
Posts: 1645
Joined: Sat Nov 10, 2007 12:59 am
Location: oakland ca

by westfw on Fri Jun 27, 2008 4:15 am

I would be VERY surprised if you can check the carry bit of the status register in C with any sort of reliability. This is one the real annoyances of C compared to assembly language; in assembly language, you have access to the 9-bit result of math on 8-bit quantities, and in C you don't. To do this sort of think efficiently, use all assembler. To do it less efficiently, use the next bigger datatype supported by C... IP code is full of checksum algorithms that collect the carries from adding a bunch of 16bit numbers to a 32bit long, for instance; turns out you don't have to add them each time, but only at the end...

westfw
 
Posts: 1634
Joined: Fri Apr 27, 2007 1:01 pm
Location: SF Bay area

by mtbf0 on Fri Jun 27, 2008 8:16 am

westfw wrote:I would be VERY surprised if you can check the carry bit of the status register in C with any sort of reliability.


Code: Select all | TOGGLE FULL SIZE
//
// then get next base frequency sample
//
    baseaccumulator.fraction += basestep.fraction;
 392:   20 91 1f 01     lds     r18, 0x011F
 396:   30 91 20 01     lds     r19, 0x0120
 39a:   80 91 11 01     lds     r24, 0x0111
 39e:   90 91 12 01     lds     r25, 0x0112
 3a2:   28 0f           add     r18, r24
 3a4:   39 1f           adc     r19, r25
 3a6:   30 93 20 01     sts     0x0120, r19
 3aa:   20 93 1f 01     sts     0x011F, r18
    if (SREG & (1 << SREG_C))           // on carry
 3ae:   0f b6           in      r0, 0x3f        ; 63
 3b0:   00 fe           sbrs    r0, 0
 3b2:   05 c0           rjmp    .+10            ; 0x3be <__vector_13+0x194>
 


Surprise!

checking pages 347-349 of the mega168 datasheet reveals that only two instructions in the above sequence touch the carry flag. guess which ones the are. given that this code executes inside an interrupt service routine and that it's the only interrupt i have enabled, i'd say i'm golden.

i have also disabled the timer0 interrupt that the arduino normally uses for timing, though looking at it in the disassembled listing i see that the status register is saved and restored, so no problem anyway.

the point of the arduino, other than it being a nice affordabe little development platform, is that you hardly have to learn c. i'm old and tired and the last assembler i learned was macro32 and since dec went under my heart's just not in it enough to learn a new one.
"i want to lead a dissipate existence, play scratchy records and enjoy my decline" - iggy pop, i need more
User avatar
mtbf0
 
Posts: 1645
Joined: Sat Nov 10, 2007 12:59 am
Location: oakland ca

Please be positive and constructive with your questions and comments.