Skip to content

Conversation

@kurisaW
Copy link
Member

@kurisaW kurisaW commented Jul 25, 2025

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

[

Fixed: #10528

feature:object name超过RT_NAME_MAX-1,返回断言
improve:完善object测试用例

image

为什么提交这份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
  • 如果是新增bsp, 已经添加ci检查到.github/workflows/bsp_buildings.yml 详细请参考链接BSP自查

@kurisaW kurisaW requested a review from Rbb666 as a code owner July 25, 2025 01:22
@github-actions github-actions bot added Kernel PR has src relate code testcase labels Jul 25, 2025
@github-actions
Copy link

github-actions bot commented Jul 25, 2025

📌 Code Review Assignment

🏷️ Tag: kernel

Reviewers: GorrayLi ReviewSun hamburger-os lianux-mm wdfk-prog xu18838022837

Changed Files (Click to expand)
  • src/klibc/utest/TC_rt_memcmp.c
  • src/object.c

🏷️ Tag: workflow

Reviewers: Rbb666 kurisaW supperthomas

Changed Files (Click to expand)
  • .github/utest/kernel/object.cfg
  • .github/workflows/utest_auto_run.yml

📊 Current Review Status (Last Updated: 2025-07-30 10:36 CST)

  • GorrayLi Pending Review
  • Rbb666 Pending Review
  • ReviewSun Pending Review
  • hamburger-os Pending Review
  • kurisaW Pending Review
  • lianux-mm Pending Review
  • supperthomas Pending Review
  • wdfk-prog Pending Review
  • xu18838022837 Pending Review

📝 Review Instructions

  1. 维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
    Maintainers can refresh the review status by clicking here: 🔄 Refresh Status

  2. 确认审核通过后评论 LGTM/lgtm
    Comment LGTM/lgtm after confirming approval

  3. PR合并前需至少一位维护者确认
    PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

@kurisaW kurisaW marked this pull request as draft July 25, 2025 02:31
@github-actions github-actions bot added the action github action yml imporve label Jul 25, 2025
@kurisaW kurisaW force-pushed the object branch 2 times, most recently from 8bdbbce to 9263b8a Compare July 28, 2025 01:16
@kurisaW kurisaW marked this pull request as ready for review July 28, 2025 01:19
@kurisaW kurisaW requested a review from supperthomas as a code owner July 28, 2025 01:19
@kurisaW
Copy link
Member Author

kurisaW commented Jul 28, 2025

object utest ci测试通过:

image

@Rbb666
Copy link
Member

Rbb666 commented Jul 28, 2025

如果这种传入的字符串超过 RT_NAME_MAX 限制的设备,万一匹配错设备怎么办,比如 RT_NAME_MAX=8 ,字符串是:norflash01norflash02

建议:直接在对象注册那边卡一下,如果名字超过长度,直接注册设备失败,给个失败警告,然后返回。
然后针对object的测试用例也可以针对这种情况完善下

@kurisaW kurisaW marked this pull request as draft July 28, 2025 03:32
@kurisaW kurisaW force-pushed the object branch 2 times, most recently from 87ed8fd to 1c61ab2 Compare July 28, 2025 07:47
@Rbb666 Rbb666 merged commit 62f3fb4 into RT-Thread:master Jul 30, 2025
63 checks passed
unicornx added a commit to unicornx/rt-thread that referenced this pull request Aug 18, 2025
Fixes: 62f3fb4: fix(kernel)/improve(utest):fix the legacy issue related to the length of the object name version RT-Thread#10537

After this patch, if length of object name exceeds (RT_NAME_MAX - 1),
RTT will assert and oops when runing, but not in period of building.
Though I don't think it's a good solution, but don't want to argue more
about this.

Old RT_NAME_MAX is 8 for k230, and some object names, such as
"hwtimer0", which name length is 8, breaking the new rule.

Just update configuration of k230 bsp and increase RT_NAME_MAX from
8 to 16, which should be long enough for k230.

Signed-off-by: Chen Wang <[email protected]>
unicornx added a commit to unicornx/rt-thread that referenced this pull request Aug 18, 2025
Fixes: 62f3fb4: fix(kernel)/improve(utest):fix the legacy issue related to the length of the object name version RT-Thread#10537

After this patch, if length of object name exceeds (RT_NAME_MAX - 1),
RTT will assert and oops when runing, but not in period of building.
Though I don't think it's a good solution, but don't want to argue more
about this.

Old RT_NAME_MAX is 8 for k230, and some object names, such as
"hwtimer0", which name length is 8, breaking the new rule.

Just update configuration of k230 bsp and increase RT_NAME_MAX from
8 to 16, which should be long enough for k230.

Signed-off-by: Chen Wang <[email protected]>
Rbb666 pushed a commit that referenced this pull request Aug 18, 2025
Fixes: 62f3fb4: fix(kernel)/improve(utest):fix the legacy issue related to the length of the object name version #10537

After this patch, if length of object name exceeds (RT_NAME_MAX - 1),
RTT will assert and oops when runing, but not in period of building.
Though I don't think it's a good solution, but don't want to argue more
about this.

Old RT_NAME_MAX is 8 for k230, and some object names, such as
"hwtimer0", which name length is 8, breaking the new rule.

Just update configuration of k230 bsp and increase RT_NAME_MAX from
8 to 16, which should be long enough for k230.

Signed-off-by: Chen Wang <[email protected]>
yandld pushed a commit to yandld/rt-thread that referenced this pull request Aug 20, 2025
Fixes: 62f3fb4: fix(kernel)/improve(utest):fix the legacy issue related to the length of the object name version RT-Thread#10537

After this patch, if length of object name exceeds (RT_NAME_MAX - 1),
RTT will assert and oops when runing, but not in period of building.
Though I don't think it's a good solution, but don't want to argue more
about this.

Old RT_NAME_MAX is 8 for k230, and some object names, such as
"hwtimer0", which name length is 8, breaking the new rule.

Just update configuration of k230 bsp and increase RT_NAME_MAX from
8 to 16, which should be long enough for k230.

Signed-off-by: Chen Wang <[email protected]>
tomjielii pushed a commit to tomjielii/rt-thread that referenced this pull request Sep 9, 2025
Fixes: 62f3fb4: fix(kernel)/improve(utest):fix the legacy issue related to the length of the object name version RT-Thread#10537

After this patch, if length of object name exceeds (RT_NAME_MAX - 1),
RTT will assert and oops when runing, but not in period of building.
Though I don't think it's a good solution, but don't want to argue more
about this.

Old RT_NAME_MAX is 8 for k230, and some object names, such as
"hwtimer0", which name length is 8, breaking the new rule.

Just update configuration of k230 bsp and increase RT_NAME_MAX from
8 to 16, which should be long enough for k230.

Signed-off-by: Chen Wang <[email protected]>
@kurisaW kurisaW deleted the object branch September 10, 2025 01:25
tomjielii pushed a commit to tomjielii/rt-thread that referenced this pull request Sep 12, 2025
Fixes: 62f3fb4: fix(kernel)/improve(utest):fix the legacy issue related to the length of the object name version RT-Thread#10537

After this patch, if length of object name exceeds (RT_NAME_MAX - 1),
RTT will assert and oops when runing, but not in period of building.
Though I don't think it's a good solution, but don't want to argue more
about this.

Old RT_NAME_MAX is 8 for k230, and some object names, such as
"hwtimer0", which name length is 8, breaking the new rule.

Just update configuration of k230 bsp and increase RT_NAME_MAX from
8 to 16, which should be long enough for k230.

Signed-off-by: Chen Wang <[email protected]>
@Ryan-CW-Code
Copy link
Contributor

Ryan-CW-Code commented Sep 17, 2025

这里改动还是太激进了,感觉还是加入兼容模式比较好。
很多对象也是到用的时候才创建,如果到用时才能通过打印日志 / RT_ASSERT发现错误话,可能会引入很多隐藏bug。
特别是很多工程发正式版本代码的时候都会关闭日志和RT_ASSERT检查,这样就会发生内存访问越界了。
直接给一个比较大的值又会占用额外的内存,我们一个产品的内存空间已经不支持这样做了。

最不济修改下 406行和522行 rt_memcpy的写法,保证不会发生访问越界吧

我也是发现打印线程信息的时候,有几个线程信息丢失了,才发现这里内存访问越界了,还是很可怕的

@kurisaW
Copy link
Member Author

kurisaW commented Sep 17, 2025

这里改动还是太激进了,感觉还是加入兼容模式比较好。 很多对象也是到用的时候才创建,如果到用时才能通过打印日志 / RT_ASSERT发现错误话,可能会引入很多隐藏bug。 特别是很多工程发正式版本代码的时候都会关闭日志和RT_ASSERT检查,这样就会发生内存访问越界了。 直接给一个比较大的值又会占用额外的内存,我们一个产品的内存空间已经不支持这样做了。

最不济修改下 406行和522行 rt_memcpy的写法,保证不会发生访问越界吧

我也是发现打印线程信息的时候,有几个线程信息丢失了,才发现这里内存访问越界了,还是很可怕的

对于兼容模式的话,大佬有什么好的想法吗,或许可以提个PR?

@Ryan-CW-Code
Copy link
Contributor

现在强制触发 assert 的做法是合理的,但感觉是一个漫长的兼容工作,很多第三方软件包都不怎么维护了,也就不会更新这个。
可以考虑增加一个宏开关,默认保持现有 assert 行为,旧工程可通过开启宏切换回原有的截断处理方式。
另一种方案是继续沿用此前的截断策略,在名称超长后仅给出警告提示,我更倾向于此方案,因为 RT_NAME_MAX 目前是开放给用户可配置的,只要明确告知用户后果就行。
如果保持现在的做法或者使宏切换,也建议给 RT_NAME_MAX 增加一个最小值限制,保证内核层不会因为用户设置的过小出现问题

@kurisaW
Copy link
Member Author

kurisaW commented Sep 17, 2025

assert应该在编译阶段就会截停RT_NAME_MAX问题吧,我觉得这应该不算很大问题,按照此处的逻辑,RT_NAME_MAX 本就是RT-Thread系统中对于对象定义的最大限制,用户要么选择名字长度设置小一点,要么选择扩大RT_NAME_MAX,如果还保留之前的模式的话,那么在查找对象的时候,一旦超出RT_NAME_MAX 限制,会存在查找错误的情况,这本身也不合理,已经有开发者出现踩坑情况了

另外如果说真的考虑说资源占用,会有一个比较大的数组,我觉得可以考虑说加入动态适应创建对象名称这个机制

@Ryan-CW-Code
Copy link
Contributor

问题就是不会编译期报错,要到运行时。 而且如果对象不是开机就创建而是特定条件才会创建,就更难发现了

@Ryan-CW-Code
Copy link
Contributor

动态创建应该很难,rt_malloc还有头部空间,反而占用可能会更高

@Ryan-CW-Code
Copy link
Contributor

Ryan-CW-Code commented Sep 17, 2025

您说的坑点,主要是因为之前没有任何提示开发者溢出了,现在已经明确提示了,发现ERROR错误肯定会进行修改或者更容易排查问题了。

或者有什么办法能在编译时就能检查出所有对象名称有没有超界,这样保持现在这样是没问题的,强制用户必须解决。
毕竟如果是软件包或者组件超界了,用户是很难自己发现的。

应该没办法编译时发现,还一些组件和软件包对象的名称是使用rt_snprintf拼接的

@kurisaW
Copy link
Member Author

kurisaW commented Sep 17, 2025

问题就是不会编译期报错,要到运行时。 而且如果对象不是开机就创建而是特定条件才会创建,就更难发现了

理解你的意思了,这样确实不会在程序运行之前就发现问题,另外考虑是用户可能没有开启assert检查还有LOG打印是吧,是不是换成rt_kprintf打印会好点,至少提示信息能打印出来,然后引导用户去修改大小,我能想出比较好的就是日志提示一下

@Ryan-CW-Code
Copy link
Contributor

继续使用之前截断方式,超长加个日志提示。
官方给个默认值比如12 / 16,鼓励开发者都按照这个默认值来?然后加个宏选择要不要assert报错,CI的时候检查是否符合默认的规则?

@kaidegit
Copy link
Contributor

好像"sys workq"这样子的直接就报断言了,每个bsp给改过来也比较麻烦吧,搜了下很多都是8。系统自带的这些要不检查一下改下名缩到8以内吧,要不跑bsp直接出断言错误还是比较烦的。

@milo-9
Copy link
Contributor

milo-9 commented Oct 31, 2025

现在才看到这个pr,我感觉太激进了吧,本来这个名字就没说一定要\0结尾,现在假如系统里有一个对象名字很长,就要所有对象占用的空间平白多那么多,这太不友好了,就我实际项目经验来说,这个名字基本没什么用,反而每次创建多个对象的时候,都为这个名字要搞半天,我甚至希望有创建匿名对象的API,省功夫省内存空间,如果担心重名并且需要在代码中去做匹配,那完全可以想办法通过别的方式去匹配,或者内核配置时添加宏开关来控制,毕竟内核本来就没有判断对象是否重名。

@armink
Copy link
Member

armink commented Oct 31, 2025

我也觉得这是一个有点仓促的 PR 。感觉像是内部讨论好了,就直接提交走流程的一个 PR。

开源项目,要很多人维护,这些 PR 信息写清楚了,信息更全面了,将来才能让好让更多人参与进来吧?

image

@aozima
Copy link
Member

aozima commented Oct 31, 2025

现在才看到这个pr,我感觉太激进了吧,本来这个名字就没说一定要\0结尾,现在假如系统里有一个对象名字很长,就要所有对象占用的空间平白多那么多,这太不友好了,就我实际项目经验来说,这个名字基本没什么用,反而每次创建多个对象的时候,都为这个名字要搞半天,我甚至希望有创建匿名对象的API,省功夫省内存空间,如果担心重名并且需要在代码中去做匹配,那完全可以想办法通过别的方式去匹配,或者内核配置时添加宏开关来控制,毕竟内核本来就没有判断对象是否重名。

支持+1

我说咋上周使用最新内核的项目中跑不起来,后面与能跑起来的版本全文对比,发现仅是NAME_MAX配置不同。。。。


其实有个想法,不一定实施,可以研究探讨下:

现在不同的项目中NAME_MAX不同会造成sizeof差异,有时项目发展过程中还多次调大这个配置,
如果是.a则需要重新编译,很是不便。

如果这里的name改为指针,就没这个问题了。但是name也需要额外的存储空间,如果是malloc就更费,常见的是跟在obj之后。

进一步优化,这个指针目前至少为32位,可以用其最高2位来表示这是指针还是直接就是数据,甚至可以用上int压缩算法。
这样当名称比较短,如1-4个字符,甚至匿名的时候,直接使用这4字节就足够了。
如果限制名称一定是ascii表中的字符的话,只要7位就够了。

如果名称超过4,就使用指针方式指向目标。这样再长的名称也能放得下。不再需要 NAME_MAX 这个配置项了。

对应地,64位同样适用,对应提升一下即可。

@kurisaW
Copy link
Member Author

kurisaW commented Oct 31, 2025

感谢大家的反馈,这个PR和开源社区这块同步的确实不足,下周我们会召开一次对外例会,同步一下最新主线v5.2.2版本的更新,同时梳理一下一些比较有争议的PR吧,想要参会的可以邮箱通知我一下:[email protected]

@dongly
Copy link
Contributor

dongly commented Nov 1, 2025

现在才看到这个pr,我感觉太激进了吧,本来这个名字就没说一定要\0结尾,现在假如系统里有一个对象名字很长,就要所有对象占用的空间平白多那么多,这太不友好了,就我实际项目经验来说,这个名字基本没什么用,反而每次创建多个对象的时候,都为这个名字要搞半天,我甚至希望有创建匿名对象的API,省功夫省内存空间,如果担心重名并且需要在代码中去做匹配,那完全可以想办法通过别的方式去匹配,或者内核配置时添加宏开关来控制,毕竟内核本来就没有判断对象是否重名。

支持+1

我说咋上周使用最新内核的项目中跑不起来,后面与能跑起来的版本全文对比,发现仅是NAME_MAX配置不同。。。。

其实有个想法,不一定实施,可以研究探讨下:

现在不同的项目中NAME_MAX不同会造成sizeof差异,有时项目发展过程中还多次调大这个配置, 如果是.a则需要重新编译,很是不便。

如果这里的name改为指针,就没这个问题了。但是name也需要额外的存储空间,如果是malloc就更费,常见的是跟在obj之后。

进一步优化,这个指针目前至少为32位,可以用其最高2位来表示这是指针还是直接就是数据,甚至可以用上int压缩算法。 这样当名称比较短,如1-4个字符,甚至匿名的时候,直接使用这4字节就足够了。 如果限制名称一定是ascii表中的字符的话,只要7位就够了。

如果名称超过4,就使用指针方式指向目标。这样再长的名称也能放得下。不再需要 NAME_MAX 这个配置项了。

对应地,64位同样适用,对应提升一下即可。

+1

@milo-9
Copy link
Contributor

milo-9 commented Nov 2, 2025

现在才看到这个pr,我感觉太激进了吧,本来这个名字就没说一定要\0结尾,现在假如系统里有一个对象名字很长,就要所有对象占用的空间平白多那么多,这太不友好了,就我实际项目经验来说,这个名字基本没什么用,反而每次创建多个对象的时候,都为这个名字要搞半天,我甚至希望有创建匿名对象的API,省功夫省内存空间,如果担心重名并且需要在代码中去做匹配,那完全可以想办法通过别的方式去匹配,或者内核配置时添加宏开关来控制,毕竟内核本来就没有判断对象是否重名。

支持+1

我说咋上周使用最新内核的项目中跑不起来,后面与能跑起来的版本全文对比,发现仅是NAME_MAX配置不同。。。。


其实有个想法,不一定实施,可以研究探讨下:

现在不同的项目中NAME_MAX不同会造成sizeof差异,有时项目发展过程中还多次调大这个配置,
如果是.a则需要重新编译,很是不便。

如果这里的name改为指针,就没这个问题了。但是name也需要额外的存储空间,如果是malloc就更费,常见的是跟在obj之后。

进一步优化,这个指针目前至少为32位,可以用其最高2位来表示这是指针还是直接就是数据,甚至可以用上int压缩算法。
这样当名称比较短,如1-4个字符,甚至匿名的时候,直接使用这4字节就足够了。
如果限制名称一定是ascii表中的字符的话,只要7位就够了。

如果名称超过4,就使用指针方式指向目标。这样再长的名称也能放得下。不再需要 NAME_MAX 这个配置项了。

对应地,64位同样适用,对应提升一下即可。

用指针的话假如真的需要写入名称感觉对空间浪费更严重了,内存分配出来的对象的头部空间占用太多了,名字短了的话空间利用率很低,并且现有代码中所有对象都是具名的,那样老代码的空间内存占用就上来了,而且还要考虑没有使能内存动态分配的情况,比较麻烦。

我之前的想法是重新添加一些创建匿名对象的API,比如rt_mutex_create_anonymous这样的API。
具体做法的话其实倒是有个方案,只是改动量有点大,我就没有提。

如果大家都觉得这是个痛点的话,我倒是觉得可以好好讨论讨论。

@kurisaW
Copy link
Member Author

kurisaW commented Nov 7, 2025

@dongly @Ryan-CW-Code @kaidegit @milo-9 @armink @aozima 几位大佬可以通过下面的会议链接参加下本周日上午的社区对外例会哈,届时可以一起讨论下此PR问题

会议主题:RT-Thread社区讨论会
会议时间:2025/11/09 10:00-12:00 (GMT+08:00) 中国标准时间 - 北京

点击链接入会,或添加至会议列表:
https://meeting.tencent.com/dm/85M3Jqnb0gu4

#腾讯会议:109-998-833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action github action yml imporve BSP component: drivers/core component: drivers/ipc component: drivers component: lwp Component Doc This PR/issue related with documents Kernel PR has src relate code RT-Smart RT-Thread Smart related PR or issues testcase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 使用 rt_object_find()时,当 name 的长度 > RT_NAME_MAX, 查找不到,同之前的行为不一致

9 participants