Re: Memory corruption (?) I don't understand

[Senthil Kumar]

This looks like a miscompilation to me. Specifically, in good_2.S, you have

    a318: f6 01       movw r30, r12
...

    a33c: e3 50       subi r30, 0x03 ; 3
    a33e: ff 4f       sbci r31, 0xFF ; 255
    a340: 01 90       ld r0, Z+
    a342: f0 81       ld r31, Z
    a344: e0 2d       mov r30, r0

This loads Z with R13:R12, and then later offsets it by -0xFF03 to
obtain the address of self->handler, which is then used by the icall
instruction.

a31c: f6 01       movw r30, r12
...

 a340: 68 01       movw r12, r16
    a342: ff e4       ldi r31, 0x4F ; 79
    a344: cf 1a       sub r12, r31
    a346: fe ef       ldi r31, 0xFE ; 254
    a348: df 0a       sbc r13, r31
    a34a: e3 50       subi r30, 0x03 ; 3
    a34c: ff 4f       sbci r31, 0xFF ; 255
    a34e: 01 90       ld r0, Z+
    a350: f0 81       ld r31, Z
    a352: e0 2d       mov r30, r0

Note the clobbering of R31:R30 with immediate values *before* the
offsetting is done. I think this is a codegen bug - the compiler
should have either picked a different set of regs to subtract R13:R12
from, or should have restored Z with a movw r30, r12 before 0xa34a.

What version of the compiler are you using? Can you make a reduced
testcase containing just the LDL_MAC_otaa function?

Regards
Senthil


On Wed, Jun 23, 2021 at 3:12 PM BERTRAND Joël <joel.bertrand@systella.fr> wrote:
>
> David Brown a écrit :
> > On 23/06/2021 10:37, BERTRAND Joël wrote:
> >
> >> In bad.S, self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg) gives :
> >>     a340:   68 01           movw    r12, r16
> >>     a342:   ff e4           ldi r31, 0x4F   ; 79
> >>     a344:   cf 1a           sub r12, r31
> >>     a346:   fe ef           ldi r31, 0xFE   ; 254
> >>     a348:   df 0a           sbc r13, r31
> >>     a34a:   e3 50           subi    r30, 0x03   ; 3
> >>     a34c:   ff 4f           sbci    r31, 0xFF   ; 255
> >>     a34e:   01 90           ld  r0, Z+
> >>     a350:   f0 81           ld  r31, Z
> >>     a352:   e0 2d           mov r30, r0
> >>     a354:   ae 01           movw    r20, r28
> >>     a356:   4f 5a           subi    r20, 0xAF   ; 175
> >>     a358:   5f 4f           sbci    r21, 0xFF   ; 255
> >>     a35a:   65 e0           ldi r22, 0x05   ; 5
> >>     a35c:   70 e0           ldi r23, 0x00   ; 0
> >>     a35e:   d6 01           movw    r26, r12
> >>     a360:   8d 91           ld  r24, X+
> >>     a362:   9c 91           ld  r25, X
> >>     a364:   09 95           icall
> >>
> >>      I have rebuilt a second firmware that works as expected with:
> >>
> >> if (self->handler == NULL) for(;;);
> >>             self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg);
> >>
> >>      Asmb output contains :
> >>
> >> if (self->handler == NULL) for(;;);
> >>     a33c:   e3 50           subi    r30, 0x03   ; 3
> >>     a33e:   ff 4f           sbci    r31, 0xFF   ; 255
> >>     a340:   01 90           ld  r0, Z+
> >>     a342:   f0 81           ld  r31, Z
> >>     a344:   e0 2d           mov r30, r0
> >>     a346:   30 97           sbiw    r30, 0x00   ; 0
> >>     a348:   09 f4           brne    .+2         ; 0xa34c
> >>     a34a:   4a c0           rjmp    .+148       ; 0xa3e0
> >>             self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg);
> >>     a34c:   68 01           movw    r12, r16
> >>     a34e:   2f e4           ldi r18, 0x4F   ; 79
> >>     a350:   c2 1a           sub r12, r18
> >>     a352:   2e ef           ldi r18, 0xFE   ; 254
> >>     a354:   d2 0a           sbc r13, r18
> >>     a356:   ae 01           movw    r20, r28
> >>     a358:   4f 5a           subi    r20, 0xAF   ; 175
> >>     a35a:   5f 4f           sbci    r21, 0xFF   ; 255
> >>     a35c:   65 e0           ldi r22, 0x05   ; 5
> >>     a35e:   70 e0           ldi r23, 0x00   ; 0
> >>     a360:   d6 01           movw    r26, r12
> >>     a362:   8d 91           ld  r24, X+
> >>     a364:   9c 91           ld  r25, X
> >>     a366:   09 95           icall
> >>
> >>      I'm not a AVR ASMB specialist, but I don't understand why
> >> self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg) produces a
> >> different code (with of course the same compilation options) when it was
> >> preceded by if (self->handler == NULL) for(;;);
> >>
> >
> > I haven't looked at any of the rest of your code.  But here I think the
> > differences are just that after the NULL check, the compiler knows that
> > self->handler is already in Z, and this can be re-used later in the
> > indirect call.  So the registers used for getting the other parameters,
> > and perhaps the order they are collected, can change accordingly.
> > Optimisations and register allocation algorithms can often make the
> > assembly appear significantly differently when in fact the changes are
> > typically cosmetic.
>
>         Maybe, but how do you can explain that with a NULL test, code runs as
> expected and without, it crashes ?
>
>         I have tried another test :
>
>             if (self->handler != app_handler) for(;;);
>             self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg);
>
> and firmware runs as expected. But this one crashes :
>
>             if (self->handler != app_handler) {}
>             self->handler(self->app, LDL_MAC_DEV_NONCE_UPDATED, &arg);
>
>         I have uploded three firmwares (built with -Os) :
>
> https://hilbert.systella.fr/public/good.S
> https://hilbert.systella.fr/public/good_2.S
> https://hilbert.systella.fr/public/bad.S
>
>         Best regards,
>
>         JKB
>