FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.76k stars 1.12k forks source link

bugfix: correct computation of stack size on Mac Posix port #816

Closed tegimeki closed 1 year ago

tegimeki commented 1 year ago

Description

Aligns the stack end to a page boundary before computing its size, since the size depends on both the start and end.

The original change which introduced stack alignment (#674) only worked for cases where the round + trunc operation would wind up within the same area, but would lead to segfaults in other cases.

Tested on ARM64 and Intel MacOS, as well as ARM64 and Intel Linux. The test cases included a single-task case, as well as a case with two tasks passing queue messages.

Test Steps

Using the Posix port on an M1 Mac, create a task with the default PTHREAD_STACK_MIN or some other "lucky" size. The task can do nothing more than delay in a loop, and you should get a segfault. If not, a different stack size will cause one. Example backtrace:

(lldb) bt
* thread #2, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000000000000
    frame #1: 0x000000010000750c single`prvWaitForStart(pvParams=0x000000010002bff0) at port.c:478:5 [opt]
    frame #2: 0x00000001a2d1ffa8 libsystem_pthread.dylib`_pthread_start + 148
(lldb) f 1
warning: single was compiled with optimization - stepping may behave oddly; variables may not be available.
frame #1: 0x000000010000750c single`prvWaitForStart(pvParams=0x000000010002bff0) at port.c:478:5 [opt]
   475     vPortEnableInterrupts();
   476  
   477     /* Call the task's entry point. */
-> 478     pxThread->pxCode( pxThread->pvParams );
   479  
   480     /* A function that implements a task must not exit or attempt to return to
   481      * its caller as there is nothing to return to. If a task wants to exit it

Checklist:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (5a9d7c8) 93.62% compared to head (b534a8e) 93.62%.

:exclamation: Current head b534a8e differs from pull request most recent head 69e6b75. Consider uploading reports for the commit 69e6b75 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #816 +/- ## ======================================= Coverage 93.62% 93.62% ======================================= Files 6 6 Lines 2508 2508 Branches 598 598 ======================================= Hits 2348 2348 Misses 107 107 Partials 53 53 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/816/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/816/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.62% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chinglee-iot commented 1 year ago

@tegimeki Thank you for creating this PR. We will look into it and discuss with you here.

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

tegimeki commented 1 year ago

@tegimeki Thank you for creating this PR. We will look into it and discuss with you here.

Thanks @chinglee-iot, I just pushed a new commit which addresses the formatting issue (unbalanced whitespace) and added another note in the commit message about a (pre-existing, now fixed) compilation warning on Mac OS:

FreeRTOS-Kernel/portable/ThirdParty/GCC/Posix/port.c:153:22: warning: incompatible integer to pointer conversion assigning to 'StackType_t *' (aka 'unsigned long *') from 'unsigned long long' [-Wint-conversion]
        pxEndOfStack = mach_vm_round_page( pxEndOfStack );
                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This was addressed by casting the result to StackType_t *.