avr-gcc-list
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [avr-gcc-list] Incorrect code


From: David Brown
Subject: Re: [avr-gcc-list] Incorrect code
Date: Sat, 09 Aug 2014 18:56:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

Hi,

I haven't studied the generated code fully, but this is actually perfectly reasonable behaviour from the compiler's viewpoint. The logic is as follows:

Since ptr1 is never assigned, the loop involving it is undefined behaviour. It could therefore do /anything/, including making daemons fly out your nose. And it certainly could do nothing at all. Since the programmer has written code with undefined behaviour, he clearly doesn't care what the results are. So since doing nothing is the shortest and simplest behaviour, that is the result.

And since that loop now does nothing, it can be removed entirely in the optimisation - and code can be generated without the "if (cs == 0)" conditional.


As a general rule, user code contains more bugs than the compiler. gcc is not /quite/ perfect, and bugs do occur, but incorrect code generation is very rare (missed optimisation opportunities are much more common). Statistically, the chances are that in a situation like this, the bug will be in the user's code.

mvh.,

David



On 09/08/14 15:56, Erik Christiansen wrote:
On 09.08.14 05:51, Thomas D. Dean wrote:
Is this a problem with -Os?

Good question. Is the second "if" restored with optimisation off?

It looks like the compiler has decided that cs is always zero in the code
below.

It's nearly midnight here, and I'm viewing the code from down under, but
ISTM that it thinks cs can never be zero.

The cs calculation appears correct.

Yep.

In spi_copy.c

...
uint8_t spi_copy_in(uint8_t *dest, uint8_t *source, unsigned int max_len) {
   uint8_t cs;
   uint8_t len;
   uint8_t *ptr1, *ptr2;

   cs = 0;
   len = source[0];
   if (len > max_len) return 0xff;
   ptr2 = &source[1];
   do {
        cs += *ptr2++;
   } while (--len);
   if (cs == 0) {
        len = source[0] - 1; // do not need the checksum
        ptr2 = &source[1];
        do {
          *ptr1++ = *ptr2++;
        } while (--len);
   }
   return cs;
}

...
                                        # The way I see it:
0000077c <spi_copy_in>:
                                        # len = source[0];
  77c:   fb 01           movw    r30, r22
  77e:   20 81           ld      r18, Z
                                        # if (len > max_len) return 0xff;
  780:   82 2f           mov     r24, r18
  782:   90 e0           ldi     r25, 0x00       ; 0
  784:   48 17           cp      r20, r24
  786:   59 07           cpc     r21, r25
  788:   38 f0           brcs    .+14            ; 0x798 <spi_copy_in+0x1c>
                                        # ptr2 = &source[1];
  78a:   31 96           adiw    r30, 0x01       ; 1
                                        # cs = 0;
  78c:   80 e0           ldi     r24, 0x00       ; 0
                                        # cs += *ptr2++;
  78e:   91 91           ld      r25, Z+
  790:   89 0f           add     r24, r25
                                        # while (--len);
  792:   21 50           subi    r18, 0x01       ; 1
  794:   e1 f7           brne    .-8             ; 0x78e <spi_copy_in+0x12>
                # Who took Tom's cheese?
                # Does it deduce that cs cannot ever be zero?
  796:   08 95           ret
  798:   8f ef           ldi     r24, 0xFF       ; 255
  79a:   08 95           ret

That looks scary. Which version is it? (Others will ask. ;-)

Erik





reply via email to

[Prev in Thread] Current Thread [Next in Thread]