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

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

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

tegimeki
Copy link
Contributor

@tegimeki tegimeki commented Oct 1, 2023

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:

  • I have tested my changes. No regression in existing tests. so far only tested with my own tests
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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

@tegimeki tegimeki requested a review from a team as a code owner October 1, 2023 22:39
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

All modified lines are covered by tests ✅

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

❗ 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
@@           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 Coverage Δ
unittests 93.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chinglee-iot
Copy link
Member

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

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 (FreeRTOS#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.

Also adds a typecast to the `mach_vm_round_page()` call, as
it is actually a macro which casts to `mach_vm_offset_t` and
the result here is used as a `StackType_t` pointer.

Tested on ARM64 and Intel MacOS, as well as ARM64 and Intel
Linux.  The test code included a single-task case, as well
as a case with two tasks passing queue messages.
@tegimeki tegimeki force-pushed the mfairman/fix-macos-pthread-stack branch from b534a8e to 69e6b75 Compare October 2, 2023 16:00
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2023

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
Copy link
Contributor Author

tegimeki commented Oct 2, 2023

@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 *.

@aggarg aggarg merged commit 57f9eed into FreeRTOS:main Oct 3, 2023
15 checks passed
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

Successfully merging this pull request may close these issues.

4 participants