You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/09 14:36:02 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
xiaoxiang781216 opened a new pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103
## Summary
set PTHREAD_STACK_MIN to DEFAULT_TASK_STACKSIZE.
## Impact
pass LTP
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#discussion_r667141342
##########
File path: sched/Kconfig
##########
@@ -1819,7 +1819,7 @@ config IDLETHREAD_STACKSIZE
config PTHREAD_STACK_MIN
int "Minimum pthread stack size"
- default 256
+ default DEFAULT_TASK_STACKSIZE
Review comment:
256B is normally too small to run a thread, can you any thread on Cortex M4 MCU with so small stack? especially if the interrupt stack is disabled. PTHREAD_STACK_MIN is seldom used macro, we found this issue in one LTP test case which run create a thread with PTHREAD_STACK_MIN(256B) stack and crash immediately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#discussion_r667314530
##########
File path: sched/Kconfig
##########
@@ -1819,7 +1819,7 @@ config IDLETHREAD_STACKSIZE
config PTHREAD_STACK_MIN
int "Minimum pthread stack size"
- default 256
+ default DEFAULT_TASK_STACKSIZE
Review comment:
DEFAULT_TASK_STACKSIZE is the default stack size for the normal task, which doesn't have the special(large) stack size requirement, so it's a good candidate for PTHREAD_STACK_MIN. Do you have a better suggestion? At least, 256B is wrong here since the default value can't work with most SoC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4103: LTP: fix stacke overflow in ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#issuecomment-878767676
Ok, I will close this PR if you still think the broken(crash) default value is a good thing to keep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 closed pull request #4103: LTP: fix stacke overflow in ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4103: LTP: fix stacke overflow in ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#issuecomment-877608538
> Please, describe the issue that this PR is intending to fix.
> Otherwise there is no way for the reviewers to evaluate the proposed change.
@gustavonihei done, please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#discussion_r667103728
##########
File path: sched/Kconfig
##########
@@ -1819,7 +1819,7 @@ config IDLETHREAD_STACKSIZE
config PTHREAD_STACK_MIN
int "Minimum pthread stack size"
- default 256
+ default DEFAULT_TASK_STACKSIZE
Review comment:
This changes from 256 to 2K - that is not a ok default change to make.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a change in pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#discussion_r667177517
##########
File path: sched/Kconfig
##########
@@ -1819,7 +1819,7 @@ config IDLETHREAD_STACKSIZE
config PTHREAD_STACK_MIN
int "Minimum pthread stack size"
- default 256
+ default DEFAULT_TASK_STACKSIZE
Review comment:
It use to work.
I just did an uptake and stack penetration has gone way up (>300 bytes) - we need to keep an eye on that!
It is better to up PTHREAD_STACK_MIN to what works not commingle it with DEFAULT_TASK_STACKSIZE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#discussion_r667314530
##########
File path: sched/Kconfig
##########
@@ -1819,7 +1819,7 @@ config IDLETHREAD_STACKSIZE
config PTHREAD_STACK_MIN
int "Minimum pthread stack size"
- default 256
+ default DEFAULT_TASK_STACKSIZE
Review comment:
DEFAULT_TASK_STACKSIZE is the stack size for all normal tasks, which don't have the special(large) stack size requirement, so it's a good candidate for PTHREAD_STACK_MIN. Do you have a better suggestion? At least, 256B is wrong here since the default value can't work with most SoC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4103: LTP: fix stacke overflow in ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#issuecomment-878238071
I am not sure whether this change is a good thing. But one thing that concerns me is that both the MIN and DEFAULT stack size values for a pthread are now the same.
> BTW, DEFAULT_TASK_STACKSIZE is a reasonable default value for PTHREAD_STACK_MIN because DEFAULT_TASK_STACKSIZE is designed to as a safe minial value for the target platform.
I don't have data to support my argument, but I believe that the DEFAULT stack size should have a growth margin above the minimum value, while still maintaining the safe characteristic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4103: LTP: fix stacke overflow bug in case ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#issuecomment-877332308
Please, describe the issue that this PR is intending to fix.
Otherwise there is no way for the reviewers to evaluate the proposed change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4103: LTP: fix stacke overflow in ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#discussion_r667318755
##########
File path: sched/Kconfig
##########
@@ -1819,7 +1819,7 @@ config IDLETHREAD_STACKSIZE
config PTHREAD_STACK_MIN
int "Minimum pthread stack size"
- default 256
+ default DEFAULT_TASK_STACKSIZE
Review comment:
BTW, DEFAULT_TASK_STACKSIZE is a reasonable default value for PTHREAD_STACK_MIN because DEFAULT_TASK_STACKSIZE is designed to as a safe minial value for the target platform.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4103: LTP: fix stacke overflow in ltp_interfaces_pthread_attr_setstacksize_1_1
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4103:
URL: https://github.com/apache/incubator-nuttx/pull/4103#issuecomment-878767676
Ok, I will close this PR if you think the broken default value is a good thing to keep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org