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 2020/05/05 15:59:53 UTC
[GitHub] [incubator-nuttx] patacongo opened a new pull request #977: Implements TLS for unaligned stacks.
patacongo opened a new pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977
## Summary
TLS has been NuttX for a long time, but has only been truly usable KERNEL build mode. That was due primarily large stack alignement that was required for TLS to work.
This PR implements TLS using an unaligned stack. This is less efficient since it does involve a system call to get the base stack address. However, it can no be generalized to all build modes.
This is an initial step in implementing an errno that supports read/write access in all build modes.
## Impact
Since TLS is not currently used in any configuration, there should be no impact of this change.
## Testing
The change was verified using the sim:ostest configuration. The configuration was modified to enable TLS. This enables the TLS test at apps/testing/ostest/tls.c
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #977: Implements TLS for unaligned stacks.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977#discussion_r420345572
##########
File path: arch/arm/include/tls.h
##########
@@ -97,11 +82,15 @@ static inline uint32_t up_getsp(void)
*
****************************************************************************/
+#ifdef CONFIG_TLS_ALIGNED
static inline FAR struct tls_info_s *up_tls_info(void)
{
DEBUGASSERT(!up_interrupt_context());
return TLS_INFO((uintptr_t)up_getsp());
Review comment:
Yes, but sim up_getsp can call __builtin_frame_address for us, right?
The problem is that:
1.up_getsp is implemented by most arch
2.the aglined stack allocation also support by several arch
3.but tls.h is just implemented on arm/sim
or1k is one of case we need modify too, some arch(riscv/xtensa) implment the partial tls which is better to remove or enhance.
The only difference what I can image is how to get the sp.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #977: Implements TLS for unaligned stacks.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977#discussion_r420322277
##########
File path: arch/arm/include/tls.h
##########
@@ -97,11 +82,15 @@ static inline uint32_t up_getsp(void)
*
****************************************************************************/
+#ifdef CONFIG_TLS_ALIGNED
static inline FAR struct tls_info_s *up_tls_info(void)
{
DEBUGASSERT(!up_interrupt_context());
return TLS_INFO((uintptr_t)up_getsp());
Review comment:
Should we remove up_tls_info and tls.h but move up_getsp to arch.h? So tls_get_info can implement align/unalign in the common place and simplify arch tls support a little bit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #977: Implements TLS for unaligned stacks.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977#discussion_r420322277
##########
File path: arch/arm/include/tls.h
##########
@@ -97,11 +82,15 @@ static inline uint32_t up_getsp(void)
*
****************************************************************************/
+#ifdef CONFIG_TLS_ALIGNED
static inline FAR struct tls_info_s *up_tls_info(void)
{
DEBUGASSERT(!up_interrupt_context());
return TLS_INFO((uintptr_t)up_getsp());
Review comment:
Should we remove up_tls_info and tls.h but move up_getsp to arch.h? So tls_get_info can implement align/unalign in the common place.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #977: Implements TLS for unaligned stacks.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977#discussion_r420329945
##########
File path: arch/arm/include/tls.h
##########
@@ -97,11 +82,15 @@ static inline uint32_t up_getsp(void)
*
****************************************************************************/
+#ifdef CONFIG_TLS_ALIGNED
static inline FAR struct tls_info_s *up_tls_info(void)
{
DEBUGASSERT(!up_interrupt_context());
return TLS_INFO((uintptr_t)up_getsp());
Review comment:
I don't think so. arch/arm/include/tls.h and arch/sim/include/tls.h implement this in very different ways. sim uses a builtin.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #977: Implements TLS for unaligned stacks.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977#discussion_r420330577
##########
File path: arch/arm/include/tls.h
##########
@@ -97,11 +82,15 @@ static inline uint32_t up_getsp(void)
*
****************************************************************************/
+#ifdef CONFIG_TLS_ALIGNED
static inline FAR struct tls_info_s *up_tls_info(void)
{
DEBUGASSERT(!up_interrupt_context());
return TLS_INFO((uintptr_t)up_getsp());
Review comment:
I think it is good to keep the architecture-specific hook to handle any future differences we encounter. There is no benefit to removing it. There is benefit to keeping it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #977: Implements TLS for unaligned stacks.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #977:
URL: https://github.com/apache/incubator-nuttx/pull/977#discussion_r420345572
##########
File path: arch/arm/include/tls.h
##########
@@ -97,11 +82,15 @@ static inline uint32_t up_getsp(void)
*
****************************************************************************/
+#ifdef CONFIG_TLS_ALIGNED
static inline FAR struct tls_info_s *up_tls_info(void)
{
DEBUGASSERT(!up_interrupt_context());
return TLS_INFO((uintptr_t)up_getsp());
Review comment:
Yes, but sim up_getsp can call __builtin_frame_address for us, right?
The problem is that:
1.up_getsp is implemented by most arch
2.the aglined stack allocation also support by several arch
3.but tls.h is just implemented on arm/sim
or1k is one of case we need modify too, some arch(riscv/xtensa) implment the partial tls which is better to remove or enhance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org