Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebooting issue with variadic arguments, zero-pointer execution #5236

Closed
tekka007 opened this issue Aug 12, 2016 · 8 comments
Closed

Rebooting issue with variadic arguments, zero-pointer execution #5236

tekka007 opened this issue Aug 12, 2016 · 8 comments

Comments

@tekka007
Copy link

AVR crashes / reboots with this sketch, asm hints to a zero-pointer call.

// 20160812 tekka
// rebooting issue due to zero-pointer execution

// (un)comment
#define issue

void before(void) __attribute__((weak));

void va_pseudo(uint8_t flag, ...) {
    va_list args;
    va_start(args, flag);
    va_end(args);
}

int main(void) {
    Serial.begin(115200);
  Serial.println("start");
  Serial.flush();


#if defined(issue)
    va_pseudo(1, 2, 3, 4);
#else
    va_pseudo(1, 2, 3);
#endif

    if (before) {
    Serial.println("This should not execute");
    Serial.flush();
        before();
    }

    Serial.println("end");
  Serial.flush();

    return 0;
}
@matthijskooijman
Copy link
Collaborator

At first glance, the code looks sane. What does it print?

Reboot does indeed suggest zero call or jump, so the call to "before" seems the most likely place for this. I assume that removing the va_pseudo call also prevents the issue from happening, which means that somehow the vararg function messes something up that prevents the before check from happening correctly.

This sounds like a gcc/compiler issue. Did you try with both 1.6.9 and 1.6.10? (the latter has a newer gcc version)?

Thanks for reducing the example, this is something that should be reproducible from here (no Arduino around to try now, though).

@tekka007
Copy link
Author

tekka007 commented Aug 16, 2016

This is the serial output with both, Arduino 1.6.9 and 1.6.10:

#define issue:

start
This should not execute
start
This should not execute
start
This should not execute
start
This should not execute
...

//#define issue:

start
end

@tekka007
Copy link
Author

tekka007 commented Aug 16, 2016

ASM output:

#define issue:

if (before) {
 10c:   8d b7       in  r24, 0x3d   ; 61
 10e:   9e b7           in  r25, 0x3e   ; 62
 110:   07 96           adiw    r24, 0x07   ; 7
 112:   0f b6           in  r0, 0x3f    ; 63
 114:   f8 94           cli
 116:   9e bf           out 0x3e, r25   ; 62
 118:   0f be           out 0x3f, r0    ; 63
 11a:   8d bf           out 0x3d, r24   ; 61
 11c:   80 e0           ldi r24, 0x00   ; 0
 11e:   90 e0           ldi r25, 0x00   ; 0
 120:   61 f0           breq    .+24        ; 0x13a <main+0x6e>
    Serial.println("This should not execute");
 122:   66 e0           ldi r22, 0x06   ; 6
 124:   71 e0           ldi r23, 0x01   ; 1
 126:   84 e4           ldi r24, 0x44   ; 68
 128:   91 e0           ldi r25, 0x01   ; 1
 12a:   0e 94 8b 02     call    0x516   ; 0x516 <_ZN5Print7printlnEPKc>
    Serial.flush();
 12e:   84 e4           ldi r24, 0x44   ; 68
 130:   91 e0           ldi r25, 0x01   ; 1
 132:   0e 94 f9 00     call    0x1f2   ; 0x1f2 <_ZN14HardwareSerial5flushEv>
        before();
 136:   0e 94 00 00     call    0   ; 0x0 <__vectors>
}

//#define issue:

if (before) {
 106:   0f 90           pop r0
 108:   0f 90           pop r0
 10a:   0f 90           pop r0
 10c:   0f 90           pop r0
 10e:   0f 90           pop r0
 110:   80 e0           ldi r24, 0x00   ; 0
 112:   90 e0           ldi r25, 0x00   ; 0
 114:   89 2b       or  r24, r25
 116:   61 f0           breq    .+24        ; 0x130 <main+0x64>
    Serial.println("This should not execute");
 118:   66 e0           ldi r22, 0x06   ; 6
 11a:   71 e0           ldi r23, 0x01   ; 1
 11c:   84 e4           ldi r24, 0x44   ; 68
 11e:   91 e0           ldi r25, 0x01   ; 1
 120:   0e 94 86 02     call    0x50c   ; 0x50c <_ZN5Print7printlnEPKc>
    Serial.flush();
 124:   84 e4           ldi r24, 0x44   ; 68
 126:   91 e0           ldi r25, 0x01   ; 1
 128:   0e 94 f4 00     call    0x1e8   ; 0x1e8 <_ZN14HardwareSerial5flushEv>
        before();
 12c:   0e 94 00 00     call    0   ; 0x0 <__vectors>
}

Where it goes wrong is here:

 11c:   80 e0           ldi r24, 0x00   ; 0
 11e:   90 e0           ldi r25, 0x00   ; 0
 120:   61 f0           breq    .+24        ; 0x13a <main+0x6e>

vs

110:    80 e0           ldi r24, 0x00   ; 0
112:    90 e0           ldi r25, 0x00   ; 0
114:    89 2b       or  r24, r25
116:    61 f0           breq    .+24        ; 0x130 <main+0x64>

The zero check
114: 89 2b or r24, r25
is omitted where the zero pointer execution happens.

@matthijskooijman
Copy link
Collaborator

Agreed on your analysis, the zero check is indeed needed to let breq do the right thing there.

I'm not sure if it's related (I don't think so), but you are using main() instead of setup() and loop().

This looks like a compiler bug to me. Could you perhaps reduce the testcase further (removing the serial prints) and compile it on its own with avr-gcc (without including any of the Arduino code)? If the problem then still occurs (from looking at the asm), this is something we can report to the gcc folks.

Also, could you try compiling with gcc 5, I think a build should be available here: #660 (comment)

Also, could you try building with lto disabled in 1.6.10? That means removing the -flto flags from hardware/arduino/avr/platform.txt.

@tekka007
Copy link
Author

tekka007 commented Aug 21, 2016

I've transferred the example to Atmel studio, removed all Arduino code and using only the m328p led as indicator.

Compiler avr-gcc 4.8.1, settings: Size optimisation -Os

This is the code:

#include <stddef.h>
#include <stdarg.h>
#include <avr/io.h>

#define issue

void test(void) __attribute__((weak));

void va_pseudo(int flag,...){
    va_list ap;
    va_start (ap, flag);     
    va_end (ap);           
}

int main(void) {
    // m328 led port output
    DDRB = 1 << 5;
    // led off
    PORTB = 0;

    #if defined(issue)
        va_pseudo(1, 2, 3, 4);
    #else
        va_pseudo(1, 2, 3);
    #endif

    if(test!=NULL) {
        test();
    }
    // led on
    PORTB = 1 << 5;
    while(1) {}
    return 0;
}

#define issue, ASM (led off):

 8e:    80 e2           ldi r24, 0x20   ; 32
  90:   84 b9           out 0x04, r24   ; 4
  92:   15 b8           out 0x05, r1    ; 5
  94:   1f 92           push    r1
  96:   84 e0           ldi r24, 0x04   ; 4
  98:   8f 93           push    r24
  9a:   1f 92           push    r1
  9c:   83 e0           ldi r24, 0x03   ; 3
  9e:   8f 93           push    r24
  a0:   1f 92           push    r1
  a2:   82 e0           ldi r24, 0x02   ; 2
  a4:   8f 93           push    r24
  a6:   1f 92           push    r1
  a8:   81 e0           ldi r24, 0x01   ; 1
  aa:   8f 93           push    r24
  ac:   0e 94 40 00     call    0x80    ; 0x80 <va_pseudo>
  b0:   8d b7           in  r24, 0x3d   ; 61
  b2:   9e b7           in  r25, 0x3e   ; 62
  b4:   08 96           adiw    r24, 0x08   ; 8
  b6:   0f b6           in  r0, 0x3f    ; 63
  b8:   f8 94           cli
  ba:   9e bf           out 0x3e, r25   ; 62
  bc:   0f be           out 0x3f, r0    ; 63
  be:   8d bf           out 0x3d, r24   ; 61
  c0:   80 e0           ldi r24, 0x00   ; 0
  c2:   90 e0           ldi r25, 0x00   ; 0
  c4:   11 f0           breq    .+4         ; 0xca <main+0x3c>
  c6:   0e 94 00 00     call    0   ; 0x0 <__vectors>
  ca:   80 e2           ldi r24, 0x20   ; 32
  cc:   85 b9           out 0x05, r24   ; 5
  ce:   ff cf           rjmp    .-2         ; 0xce <main+0x40>

//#define issue, ASM (led on)

  8e:   80 e2           ldi r24, 0x20   ; 32
  90:   84 b9           out 0x04, r24   ; 4
  92:   15 b8           out 0x05, r1    ; 5
  94:   1f 92           push    r1
  96:   83 e0           ldi r24, 0x03   ; 3
  98:   8f 93           push    r24
  9a:   1f 92           push    r1
  9c:   82 e0           ldi r24, 0x02   ; 2
  9e:   8f 93           push    r24
  a0:   1f 92           push    r1
  a2:   81 e0           ldi r24, 0x01   ; 1
  a4:   8f 93           push    r24
  a6:   0e 94 40 00     call    0x80    ; 0x80 <va_pseudo>
  aa:   0f 90           pop r0
  ac:   0f 90           pop r0
  ae:   0f 90           pop r0
  b0:   0f 90           pop r0
  b2:   0f 90           pop r0
  b4:   0f 90           pop r0
  b6:   80 e0           ldi r24, 0x00   ; 0
  b8:   90 e0           ldi r25, 0x00   ; 0
  ba:   89 2b           or  r24, r25
  bc:   11 f0           breq    .+4         ; 0xc2 <main+0x34>
  be:   0e 94 00 00     call    0   ; 0x0 <__vectors>
  c2:   80 e2           ldi r24, 0x20   ; 32
  c4:   85 b9           out 0x05, r24   ; 5
  c6:   ff cf           rjmp    .-2         ; 0xc6 <main+0x38>

** surprisingly, disabling size optimisation (i.e. -O0) solves the issue:

#define issue, ASM (led on):

9c: cf 93           push    r28
  9e:   df 93           push    r29
  a0:   cd b7           in  r28, 0x3d   ; 61
  a2:   de b7           in  r29, 0x3e   ; 62
  a4:   84 e2           ldi r24, 0x24   ; 36
  a6:   90 e0           ldi r25, 0x00   ; 0
  a8:   20 e2           ldi r18, 0x20   ; 32
  aa:   fc 01           movw    r30, r24
  ac:   20 83           st  Z, r18
  ae:   85 e2           ldi r24, 0x25   ; 37
  b0:   90 e0           ldi r25, 0x00   ; 0
  b2:   fc 01           movw    r30, r24
  b4:   10 82           st  Z, r1
  b6:   1f 92           push    r1
  b8:   84 e0           ldi r24, 0x04   ; 4
  ba:   8f 93           push    r24
  bc:   1f 92           push    r1
  be:   83 e0           ldi r24, 0x03   ; 3
  c0:   8f 93           push    r24
  c2:   1f 92           push    r1
  c4:   82 e0           ldi r24, 0x02   ; 2
  c6:   8f 93           push    r24
  c8:   1f 92           push    r1
  ca:   81 e0           ldi r24, 0x01   ; 1
  cc:   8f 93           push    r24
  ce:   0e 94 40 00     call    0x80    ; 0x80 <va_pseudo>
  d2:   8d b7           in  r24, 0x3d   ; 61
  d4:   9e b7           in  r25, 0x3e   ; 62
  d6:   08 96           adiw    r24, 0x08   ; 8
  d8:   0f b6           in  r0, 0x3f    ; 63
  da:   f8 94           cli
  dc:   de bf           out 0x3e, r29   ; 62
  de:   0f be           out 0x3f, r0    ; 63
  e0:   cd bf           out 0x3d, r28   ; 61
  e2:   80 e0           ldi r24, 0x00   ; 0
  e4:   90 e0           ldi r25, 0x00   ; 0
  e6:   89 2b           or  r24, r25
  e8:   11 f0           breq    .+4         ; 0xee <main+0x52>
  ea:   0e 94 00 00     call    0   ; 0x0 <__vectors>
  ee:   85 e2           ldi r24, 0x25   ; 37
  f0:   90 e0           ldi r25, 0x00   ; 0
  f2:   20 e2           ldi r18, 0x20   ; 32
  f4:   fc 01           movw    r30, r24
  f6:   20 83           st  Z, r18
  f8:   ff cf           rjmp    .-2         ; 0xf8 <main+0x5c>

//#define issue, ASM (led on):

 9c:    cf 93           push    r28
  9e:   df 93           push    r29
  a0:   cd b7           in  r28, 0x3d   ; 61
  a2:   de b7           in  r29, 0x3e   ; 62
  a4:   84 e2           ldi r24, 0x24   ; 36
  a6:   90 e0           ldi r25, 0x00   ; 0
  a8:   20 e2           ldi r18, 0x20   ; 32
  aa:   fc 01           movw    r30, r24
  ac:   20 83           st  Z, r18
  ae:   85 e2           ldi r24, 0x25   ; 37
  b0:   90 e0           ldi r25, 0x00   ; 0
  b2:   fc 01           movw    r30, r24
  b4:   10 82           st  Z, r1
  b6:   1f 92           push    r1
  b8:   83 e0           ldi r24, 0x03   ; 3
  ba:   8f 93           push    r24
  bc:   1f 92           push    r1
  be:   82 e0           ldi r24, 0x02   ; 2
  c0:   8f 93           push    r24
  c2:   1f 92           push    r1
  c4:   81 e0           ldi r24, 0x01   ; 1
  c6:   8f 93           push    r24
  c8:   0e 94 40 00     call    0x80    ; 0x80 <va_pseudo>
  cc:   0f 90           pop r0
  ce:   0f 90           pop r0
  d0:   0f 90           pop r0
  d2:   0f 90           pop r0
  d4:   0f 90           pop r0
  d6:   0f 90           pop r0
  d8:   80 e0           ldi r24, 0x00   ; 0
  da:   90 e0           ldi r25, 0x00   ; 0
  dc:   89 2b           or  r24, r25
  de:   11 f0           breq    .+4         ; 0xe4 <main+0x48>
  e0:   0e 94 00 00     call    0   ; 0x0 <__vectors>
  e4:   85 e2           ldi r24, 0x25   ; 37
  e6:   90 e0           ldi r25, 0x00   ; 0
  e8:   20 e2           ldi r18, 0x20   ; 32
  ea:   fc 01           movw    r30, r24
  ec:   20 83           st  Z, r18
  ee:   ff cf           rjmp    .-2         ; 0xee <main+0x52>

See

or r24,25

in both scenarios

  e2:   80 e0           ldi r24, 0x00   ; 0
  e4:   90 e0           ldi r25, 0x00   ; 0
  e6:   89 2b           or  r24, r25
  e8:   11 f0           breq    .+4         ; 0xee <main+0x52>

vs.

  d8:   80 e0           ldi r24, 0x00   ; 0
  da:   90 e0           ldi r25, 0x00   ; 0
  dc:   89 2b           or  r24, r25
  de:   11 f0           breq    .+4         ; 0xe4 <main+0x48>

avr-gcc 5.1.0 shows a similar behavior with -Os, however, this issue seems resolved in 6.1.0

@matthijskooijman
Copy link
Collaborator

Thanks for further minimizing. It seems I missed your later comments, since no notification is sent when you edit a comment (so best not do that when you add significant info). I've reproduced your issue and slightly minimized it further, and submitte it upstream here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77326

You say it is resolved in 6.1.0. Did you compile that yourself, or is there source of prebuilt (Linux?) binaries of avr-gcc? I couldn't find any just now (for Debian / generic Linux).

@tekka007
Copy link
Author

I've found some win binaries here: http://blog.zakkemble.co.uk/avr-gcc-6-1-0/

@matthijskooijman
Copy link
Collaborator

Upstream fixed this issue (see the bug report for details), indicating it is fixed in 6.3+ and backported to 5.5+.

I'm closing this issue, as there isn't much Arduino can do at this point, other than upgrade to a newer version when Atmel updates its patches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@matthijskooijman @tekka007 and others