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

Ensure Thread stack is 8 byte aligned #5405

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
8 participants
@c1728p9
Contributor

c1728p9 commented Oct 30, 2017

Ensure both the stack and stack size used in the Thread class are aligned to 8 bytes. This prevents the runtime error "Thread 0 error -11: Unknown" due to incorrect stack alignment.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 30, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 30, 2017

Build : SUCCESS

Build number : 381
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5405/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 requested review from bulislaw, pan- and scartmell-arm Oct 31, 2017

rtos/Thread.cpp Outdated
@@ -36,15 +45,21 @@ namespace rtos {

void Thread::constructor(osPriority priority,
uint32_t stack_size, unsigned char *stack_mem, const char *name) {

const uintptr_t unaligned_mem = (uintptr_t)stack_mem;

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 31, 2017

Contributor

This might be better using a reinterpret_cast.

rtos/Thread.cpp Outdated
_attr.name = name ? name : "application_unnamed_thread";
_attr.stack_mem = (uint32_t*)stack_mem;
_attr.stack_mem = (uint32_t*)aligned_mem;

This comment has been minimized.

@scartmell-arm

scartmell-arm Oct 31, 2017

Contributor

and this as a static_cast

@@ -23,6 +23,15 @@

#include "mbed.h"
#include "rtos/rtos_idle.h"
#include "mbed_assert.h"

#define ALIGN_UP(pos, align) ((pos) % (align) ? (pos) + ((align) - (pos) % (align)) : (pos))

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

We don't need to complicate it with conditionals, how about standard: (pos + align - 1) & ~(align - 1)

This comment has been minimized.

@c1728p9

c1728p9 Oct 31, 2017

Contributor

The reason I used conditionals is so this works with all numbers and not just powers of 2.

This comment has been minimized.

@bulislaw

bulislaw Oct 31, 2017

Member

Ok fair enough.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

@c1728p9 please address the review comments

@adbridge adbridge added needs: work and removed needs: review labels Oct 31, 2017

@c1728p9 c1728p9 force-pushed the c1728p9:align_stack branch Oct 31, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 31, 2017

Updated this pr to use reinterpret_cast and static_cast.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 31, 2017

/morph build

@scartmell-arm

This comment has been minimized.

Contributor

scartmell-arm commented Oct 31, 2017

Hm. Sorry, my bad, the static_cast broke everything.

Ensure Thread stack is 8 byte aligned
Ensure both the stack and stack size used in the Thread class are
aligned to 8 bytes. This prevents the runtime error
"Thread 0 error -11: Unknown" due to incorrect stack alignment.

@c1728p9 c1728p9 force-pushed the c1728p9:align_stack branch to d98a011 Oct 31, 2017

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 31, 2017

Changed it to use reinterpret_cast for both.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Oct 31, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 31, 2017

Build : SUCCESS

Build number : 395
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5405/

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@pan-

Would it be possible to have ALIGN_UP and ALIGN_DOWN as functions rather than macros ?

Those macros will yield an invalid result if the pos in input is a pointer rather than an integer type due to pointer arithmetic.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Nov 1, 2017

@pan- these macros won't yield an invalid result if you use a pointer. You'll get a compiler error as a function or macro if pos is a pointer, as % cannot be used on pointers. Without this fix PRs on master may fail the test tests-mbedmicro-rtos-mbed-threads randomly (but deterministically) due to slight ram changes causing the stack passed to Thread being unaligned.

@0xc0170 0xc0170 removed the needs: work label Nov 2, 2017

@0xc0170

0xc0170 approved these changes Nov 2, 2017

@theotherjimmy theotherjimmy merged commit 49883c3 into ARMmbed:master Nov 2, 2017

5 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment