Re: Incorrect code

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
>