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

Wiring Thread causes crash (Regression in 0.7.0) #1527

Closed
monkbroc opened this issue Apr 17, 2018 · 0 comments

Comments

@monkbroc
Copy link
Member

commented Apr 17, 2018

Bug Report

Using the Wiring Thread API to start a new user thread leads to a crash (SOS HardFault).

Expected Behavior

Programs using Thread that work in 0.6.3 should still work in 0.7.0

Observed Behavior

Crash as soon as the Wiring thread runs.

Steps to Reproduce

Flash the app below. It works on a Photon running 0.6.3 (prints increasing numbers on USB serial) but crashes with SOS on 0.7.0.

#include "application.h"

Thread t;

int count = 0;

void run() {
  while(true) {
    count++;
    delay(1);
  }
}

void setup() {
  Serial.begin();
  t = Thread("count", run);
}

void loop() {
  Serial.printlnf("count=%d", count);
  delay(500);
}

Note: using the other Thread constructor (accepting a wiring_thread_fn_t) also leads to a crash.

Debugging notes

Running this app on the debugger gave some hints of what's going on.

The crash occurs in Thread::run. The Thread* th parameter contains garbage instead of the target function so the MCU jumps to a junk address leading to a HardFault.

I see the root cause by looking at the code now.

The constructor passes this as the "thread function parameter" to os_thread_create. When the Thread::run static method runs, it uses the "thread function parameter" to refer to the instance of the Thread class.

However in my example code above, the Thread instance was created on the stack and assigned to a global statically-allocated variable. If Thread::run runs after the assignment takes places, the "thread function parameter" will point to the old instance on the stack instead of the new global instance so the pointer will contain junk.

Proposed solution

The solution is to wait the new thread to run before returning from the Thread constructor, the same way the C++ thread constructor does.

  • Add a Thead::started boolean defaulting to false
  • Set Thread::started to true in Thread::run
  • Add while (!started) os_thread_yield(); to both Thread constructors.

Workaround

Avoid the Thread assignment operator to make sure the instance of Thread will be the same when the constructor is called and when Thread::run is called.

#include "application.h"

Thread *t;

int count = 0;

void run() {
  while(true) {
    count++;
    delay(1);
  }
}

void setup() {
  Serial.begin();
  t = new Thread("count", run);
}

void loop() {
  Serial.printlnf("count=%d", count);
  delay(500);
}
monkbroc added a commit that referenced this issue Apr 17, 2018
Wait for Wiring Thread to reach Thread::run before returning from the constructor to avoid HardFault crash when the Thread instance is not available anymore. Fixes #1527
@monkbroc monkbroc referenced this issue Apr 17, 2018
5 of 6 tasks complete
monkbroc added a commit that referenced this issue Apr 19, 2018
Wait for Wiring Thread to reach Thread::run before returning from the constructor to avoid HardFault crash when the Thread instance is not available anymore. Fixes #1527
monkbroc added a commit that referenced this issue Apr 19, 2018
Wait for Wiring Thread to reach Thread::run before returning from the constructor to avoid HardFault crash when the Thread instance is not available anymore. Fixes #1527
@monkbroc monkbroc added the track label Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.