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.

Sign up for GitHub

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

Jump to bottom

add picolibc tls support #8392

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add picolibc tls support #8392

wants to merge 2 commits into from

Conversation

iDings
Copy link
Contributor

@iDings iDings commented Dec 21, 2023

拉取/合并请求描述：(PR description)

[

为什么提交这份PR (why to submit this PR)

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

  • BSP:
  • .config:
  • action:

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality：

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确，包括缩进空格，命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码，代码尽量精简，不包含#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的，并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification

@iDings
Copy link
Contributor Author

iDings commented Dec 21, 2023

之前picolibc的tls的讨论 #8220 (comment)
初步用qemue-vexpress-a9测试__thread tls是可以工作的.
主要参考riot的修改:RIOT-OS/RIOT@0ceb080
对于picolibc上的关于链接脚本的对齐的讨论，暂时没有理解的非常明白 picolibc/picolibc#411.

BernardXiong
BernardXiong reviewed Dec 21, 2023
View reviewed changes
src/thread.c Outdated
@@ -39,6 +39,13 @@
#include <rtthread.h>
#include <stddef.h>

#ifdef RT_USING_PICOLIBC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rtdef.h中增加了，这里也需要增加吗？

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个确实不需要增加, 我修改下

BernardXiong
BernardXiong reviewed Dec 21, 2023
View reviewed changes
bsp/qemu-vexpress-a9/link.lds
__tdata_size = SIZEOF(.tdata);

/*
* TLS zeroed data is relocated as if it immediately followed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这部分，如果是其他工具链 + 其他libc会怎么样？

Copy link
Contributor Author

@iDings iDings Dec 22, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我现在的理解，这部分是看编译器，编译器如果使能了tls支持的的话，它会生成tdata，tbss的sections，picolibc的tls部分的话是封装了一些接口方便使用.
arm提供的gcc+newlibc的编译器的话，我现在看到的都是--disable-tls的，不会生成tdata,tbss，linker script里面虽然定义了tdata，tbss，从linker map里面看这部分都不会占用空间, 对于--disable-tls走的是emutls,
image
image
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/picolibc/picolibc/blob/main/doc/tls.md
picolibc的tls-mode使用的local-exec模式, 看zephyr的话也是支持这种模式
image
image

iDings
iDings commented Dec 22, 2023
View reviewed changes
examples/libc/SConscript
@@ -1,6 +1,9 @@
from building import *

src = Glob('*.c')
group = DefineGroup('UTest', src, depend = ['RT_USING_NEWLIBC', 'RT_USING_PTHREADS'])
if GetDepend(['RT_USING_PICOLIBC']):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example/libc目录rtthread现在没有编译，也没有看到选项可以打开去编译，但是这个tls的例子暂时没有找到合适放的位置，就先放在这个下面了

@BernardXiong BernardXiong added the proposal proposal for future version label Dec 30, 2023
@BernardXiong
Copy link
Member

@Rbb666 @mysterywolf
可以帮忙看看这份，是否有可能做为一份完整的TLS支持来对待？（覆盖picolibc，newlib等，同时评估musl libc下是否有什么影响）

@mysterywolf mysterywolf added the discussion This PR/issue needs to be discussed later label Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BernardXiong BernardXiong BernardXiong left review comments

@Guozhanxin Guozhanxin Awaiting requested review from Guozhanxin Guozhanxin is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
discussion This PR/issue needs to be discussed later proposal proposal for future version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@iDings @BernardXiong @mysterywolf