已合并
【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题 #1335
komorebi创建于 3月31日
【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题 #1335
已合并
komorebi创建于 3月31日
komorebi成员
3月31日

描述

修复订阅场景keypoint结构问题

关联的Issue

NA

测试

本地测试

文档更新

NA

类型标签

likedislike
Pull Request已成功合入, 合并人@CANN-robot
(感谢 komorebi 的贡献)
Kkomorebi成员
3月31日 创建了 pull request,commit 61979b80
CANN-robotCANN-robot成员
3月31日 添加了label:stat/needs-squash
CANN-robot
CANN-robot成员
3月31日 评论:

Thanks for your pull-request.
The full list of commands accepted by me can be found at here
You can get sig-info at here


PR Approval Progress

Congratulations! All modules have met the lgtm and approve requirements.

Module Approval Details

module lgtm status approve status
src/dfx zhuliangying, chenhao_1209 (2/2) zhuliangying (1/1)

💡 Tip:

  • Committer can comment /approve or /lgtm
  • Commenting /approve implies both code review (lgtm) and intent to merge (approve)

CLA Signature Pass

chenminghao11, thanks for your pull request. All authors of the commits have signed the CLA. 👍

likedislike
CANN-robotCANN-robot成员
3月31日 添加了label:cann-cla/yes
此处折叠了5条事件消息 查看更多
Kkomorebi成员
3月31日 修改标题为 “【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题”,原标题为“【Profiling】修复订阅场景keypoint结构问题”
komorebi成员
3月31日 评论:

compile

likedislike
CANN-robotCANN-robot成员
3月31日 添加了label:ci-pipeline-running
CANN-robot
CANN-robot成员
3月31日 评论:

流水线任务触发成功
任务链接 [4ce8b828f9c1447aba1945e96f1deb35][流水线指导]

任务名称状态日志下载链接
codecheck ✅ SUCCESS >>>>>
SCA ✅ SUCCESS >>>>>
anti_virus ✅ SUCCESS >>>>>
Check_Pr ✅ SUCCESS >>>>>
Compile_Ascend_X86 ✅ SUCCESS >>>>> >>>>>
Compile_Ascend_ARM ✅ SUCCESS >>>>> >>>>>
pre_comment ✅ SUCCESS >>>>>
UT_Test_acl ✅ SUCCESS >>>>>
UT_Test_rts ✅ SUCCESS >>>>>
UT_Test_rts_c ✅ SUCCESS >>>>>
UT_Test_platform ✅ SUCCESS >>>>>
UT_Test_qs ✅ SUCCESS >>>>>
UT_Test_aicpusd ✅ SUCCESS >>>>>
UT_Test_tsd ✅ SUCCESS >>>>>
UT_Test_dfx ✅ SUCCESS >>>>>
UT_Test_mmpa ✅ SUCCESS >>>>>

[2026-03-31 11:49:04]    CI执行结束

likedislike
Kkomorebi成员
3月31日 update merge request[project id: 8810053, iid: 1335, commit_id: a3a4a18478065c6f176b0065128ae77197657bd1] virtual merging success
Kkomorebi成员
3月31日 推送  1 个提交:094d4695-chenminghao11
Kkomorebi成员
3月31日 update merge request[project id: 8810053, iid: 1335, commit_id: d799980e8eb170720008b8862609180df08aa42c] virtual merging success
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ⚠️ Medium
审查时间: 2026-03-31 11:42


📊 审查结论

⚠️ 建议修改后合入

  • 严重性: Medium
  • 代码质量: 良好
  • 内存安全: ⚠️ 有风险
  • 安全性: ✅ 无漏洞
  • 测试覆盖: 部分
  • 文档完整性: 部分

代码重构合理,使用模板函数支持两种不同的 keypoint 结构,提升了代码的灵活性和类型安全性。但存在潜在的数据截断风险,建议添加溢出检查。


📋 修改概述

本次 PR 修复了订阅场景下 keypoint 结构处理问题,通过引入模板函数支持两种不同的 keypoint 数据结构(TsProfileKeypoint 和 TsDavidKeypoint)。

  • 修改文件: 4个 (+36行, -15行)
  • 核心变更:
    • analyzer_ts.h: 将 ParseTsKeypointData 改为模板函数
    • data_struct.h: 新增 TsDavidKeypoint 结构体定义
    • analyzer_ffts.cpp: 修复 ExtPmu 模式下的 taskId 计算逻辑
    • analyzer_ts.cpp: 重构 ParseTsKeypointData 实现,支持两种数据结构

🔍 代码质量检查

1. 内存安全 ⚠️

内存泄漏: ✅ 无风险

  • 没有新增动态内存分配
  • 使用栈对象和模板,无泄漏风险

指针操作: ✅ 安全

  • 使用 Utils::ReinterpretCast 替代裸 reinterpret_cast,提升了类型安全性
  • 添加了缓冲区大小检查:remainingLen < sizeof(TsProfileKeypoint)

动态分配: ✅ 合理

  • 无新增动态分配

资源管理: ⚠️ 有风险

  • 问题: opData.taskId = static_cast<uint16_t>(tsData->taskId); (analyzer_ts.cpp:157)
  • 风险: 当 TsDavidKeypoint.taskId (uint32_t) 的值超过 uint16_t 范围 (0-65535) 时,会发生隐式截断
  • 建议: 添加溢出检查:
    if (tsData->taskId > UINT16_MAX) {
        MSPROF_LOGW("taskId %" PRIu32 " exceeds uint16_t range, will be truncated", tsData->taskId);
    }
    opData.taskId = static_cast<uint16_t>(tsData->taskId);
    

2. 安全性 ✅

输入验证: ✅ 完整

  • 检查 tsData->head.bufSize != sizeof(T)
  • 检查 tsData->timestamp == 0
  • 添加了最小缓冲区大小检查

边界检查: ✅ 完整

  • remainingLen < tsHeader->bufSize 检查
  • remainingLen < sizeof(TsProfileKeypoint) 检查

潜在漏洞: ✅ 无

  • 无缓冲区溢出风险
  • 无空指针解引用风险

3. 可读性 ✅

代码清晰度: ✅ 良好

  • 模板函数设计清晰,职责单一
  • 条件分支逻辑清晰(IsExtPmu() 判断)

命名规范: ✅ 符合

  • 变量命名清晰(extTaskId, offsetTask)
  • 函数命名符合项目规范

注释完整性: ⚠️ 部分

  • 有注释说明:// sizeof(TsProfileKeypoint) equal to sizeof(TsDavidKeypoint)
  • 建议: 添加更多注释说明两种结构体的差异和使用场景
  • 建议: 在 TsDavidKeypoint 结构体定义处添加注释说明其用途

4. 逻辑正确性 ⚠️

算法逻辑: ✅ 正确

  • ExtPmu 模式下的 taskId 计算:(taskId << 16) | streamId 逻辑正确
  • 模板函数的类型推导正确

边界条件: ⚠️ 部分处理

  • taskId 截断问题未处理(见内存安全部分)
  • 假设: 注释声称两种结构体大小相同,但未在编译时验证
  • 建议: 添加静态断言:
    static_assert(sizeof(TsProfileKeypoint) == sizeof(TsDavidKeypoint),
                  "TsProfileKeypoint and TsDavidKeypoint must have the same size");
    

影响范围: ✅ 明确

  • 仅影响 Profiling 模块的 keypoint 数据解析
  • ExtPmu 模式的 taskId 计算逻辑修复

💡 改进建议

  1. 数据截断检查: 在 opData.taskId 赋值处添加溢出检查和日志记录,避免静默截断导致数据不一致

  2. 编译时验证: 添加 static_assert 验证两种结构体大小相同,防止未来修改导致的内存问题

  3. 文档完善:

    • TsDavidKeypoint 结构体定义处添加注释说明其与 TsProfileKeypoint 的区别和使用场景
    • 说明 ExtPmu 模式下 taskId 的编码方式(高16位 + 低16位)
  4. 测试建议:

    • 添加边界测试用例:taskId 接近或超过 uint16_t 最大值的情况
    • 验证 ExtPmu 模式下的 taskId 编码/解码正确性

✅ 代码亮点

  • 类型安全提升: 使用模板函数替代 void* 和 reinterpret_cast,提升类型安全性
  • 防御性编程: 添加了缓冲区大小检查和 timestamp 验证
  • 代码复用: 通过模板机制实现代码复用,避免重复逻辑
  • 显式实例化: 在 .cpp 文件末尾显式实例化模板,确保链接正确
  • 命名空间使用: 正确使用命名空间别名简化代码

总体评价:

代码重构合理,解决了订阅场景下 keypoint 结构处理问题。模板化设计提升了代码的灵活性和类型安全性。主要风险在于 uint32_t 到 uint16_t 的隐式截断,建议添加溢出检查。代码质量良好,建议修改后合入。


⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
CANN-robotCANN-robot成员
3月31日 删除了label:ci-pipeline-running
CANN-robotCANN-robot成员
3月31日 添加了label:ci-pipeline-passed
komorebi成员
3月31日 评论:

compile

likedislike
CANN-robotCANN-robot成员
3月31日 删除了label:ci-pipeline-passed
CANN-robotCANN-robot成员
3月31日 添加了label:ci-pipeline-running
CANN-robot
CANN-robot成员
3月31日 评论:

流水线任务触发成功
任务链接 [7474733a412341448070d5118e7095d5][流水线指导]

任务名称状态日志下载链接
codecheck ✅ SUCCESS >>>>>
SCA ✅ SUCCESS >>>>>
anti_virus ✅ SUCCESS >>>>>
Check_Pr ✅ SUCCESS >>>>>
Compile_Ascend_X86 ✅ SUCCESS >>>>> >>>>>
Compile_Ascend_ARM ✅ SUCCESS >>>>> >>>>>
pre_comment ✅ SUCCESS >>>>>
UT_Test_acl ✅ SUCCESS >>>>>
UT_Test_rts ✅ SUCCESS >>>>>
UT_Test_rts_c ✅ SUCCESS >>>>>
UT_Test_platform ✅ SUCCESS >>>>>
UT_Test_qs ✅ SUCCESS >>>>>
UT_Test_aicpusd ✅ SUCCESS >>>>>
UT_Test_tsd ✅ SUCCESS >>>>>
UT_Test_dfx ✅ SUCCESS >>>>>
UT_Test_mmpa ✅ SUCCESS >>>>>

[2026-03-31 14:53:47]    CI执行结束

likedislike
CANN-robotCANN-robot成员
3月31日 删除了label:ci-pipeline-running
CANN-robotCANN-robot成员
3月31日 添加了label:ci-pipeline-passed
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ✅ Low
审查时间: 2026-03-31 14:52


📊 审查结论

✅ 建议合入

  • 严重性: Low
  • 代码质量: 良好
  • 内存安全: ✅ 无风险
  • 安全性: ✅ 无漏洞
  • 测试覆盖: 本地测试
  • 文档完整性: 部分

这是一个修复订阅场景 keypoint 结构问题的 bug 修复 PR。代码整体质量良好,改进了类型安全性,增强了边界检查,使用模板化设计提高了代码复用性。


📋 修改概述

本次 PR 修复了订阅场景下 keypoint 结构解析问题,主要变更:

  • 修改文件: 4个 (+36行, -15行)
  • 核心变更:
    • data_struct.h: 新增 TsDavidKeypoint 结构体,支持扩展 PMU 场景
    • analyzer_ts.h/cpp: 模板化 ParseTsKeypointData 函数,支持两种数据结构
    • analyzer_ffts.cpp: ExtPmu 模式下使用组合 taskId (taskId << 16 | streamId)

🔍 代码质量检查

1. 内存安全 ✅

  • 内存泄漏: 无风险 - 无动态内存分配
  • 指针操作: 安全 - 使用 ReinterpretCast 封装,增加了类型检查
  • 动态分配: 无动态分配
  • 资源管理: 使用栈对象和智能指针

改进点

  • ✅ 增强了边界检查:remainingLen < sizeof(TsProfileKeypoint)
  • ✅ 在解析前验证 bufSize 和 timestamp,避免解析无效数据

2. 安全性 ✅

  • 输入验证: 完整 - 检查 bufSize、timestamp、tagId 等关键字段
  • 边界检查: 完整 - 在解析前检查数据长度是否足够
  • 潜在漏洞: 无

改进点

  • ✅ 使用 Utils::ReinterpretCast 替代裸的 reinterpret_cast,提高类型安全性
  • ✅ 模板函数在编译期进行类型检查

3. 可读性 ✅

  • 代码清晰度: 良好 - 逻辑清晰,结构合理
  • 命名规范: 符合 - TsDavidKeypoint、TsProfileKeypoint 命名清晰
  • 注释完整性: 部分 - 有必要的注释,但可以更详细

建议

  • 可以添加注释说明 TsDavidKeypoint 与 TsProfileKeypoint 的区别和使用场景
  • 可以添加注释说明 ExtPmu 模式的含义

4. 逻辑正确性 ✅

  • 算法逻辑: 正确 - 模板化设计合理,支持两种数据结构
  • 边界条件: 处理完整 - 检查了数据长度、bufSize、timestamp 等
  • 影响范围: 明确 - 仅影响订阅场景的 keypoint 数据解析

注意事项

  • TsDavidKeypoint 和 TsProfileKeypoint 的字段顺序不同,但大小相同(都是 48 字节)
  • 两种结构体的 taskId 类型不同(uint32_t vs uint16_t)

💡 改进建议

  1. 类型转换: 在 analyzer_ts.cpp 第 157 行,将 tsData->taskId(uint32_t)转换为 opData.taskId(uint16_t)时存在截断风险。建议:

    // 当前代码
    opData.taskId = static_cast<uint16_t>(tsData->taskId);
    
    // 建议添加检查或注释说明
    if (tsData->taskId > UINT16_MAX) {
        MSPROF_LOGW("taskId %" PRIu32 " exceeds uint16_t range, truncated", tsData->taskId);
    }
    opData.taskId = static_cast<uint16_t>(tsData->taskId);
    

    或者在注释中说明 ExtPmu 场景下 taskId 不会超过 uint16_t 范围。

  2. 文档完善: 建议在 data_struct.h 中添加注释说明:

    // TsProfileKeypoint: 标准订阅场景的 keypoint 数据结构
    // TsDavidKeypoint: ExtPmu 场景的 keypoint 数据结构,支持更大的 taskId 范围
    
  3. 一致性: analyzer_ffts.cpp 中 ExtPmu 模式使用组合 taskId(taskId << 16 | streamId),但 analyzer_ts.cpp 中没有。建议确认这是否符合预期,或者添加注释说明两者的差异。


✅ 代码亮点

  1. 类型安全改进: 使用模板函数和 ReinterpretCast,提高了类型安全性
  2. 边界检查增强: 在解析数据前增加长度检查,防止缓冲区溢出
  3. 代码复用: 模板化设计避免了代码重复,提高了可维护性
  4. 错误处理完善: 对重复的 keypoint、缺失的 keypoint 等场景都有错误日志记录
  5. 向后兼容: 保留了对原有 TsProfileKeypoint 的支持

总体评价: 这是一个质量良好的 bug 修复 PR,改进了类型安全性和边界检查,代码结构清晰。建议合入。


⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
zhuliangying成员
3月31日 评论:

/approve

likedislike
CANN-robotCANN-robot成员
3月31日 添加了label:approved
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ✅ Low
审查时间: 2026-03-31 15:42


📊 审查结论

✅ 建议合入

  • 严重性: Low
  • 代码质量: 良好
  • 内存安全: ✅ 无风险
  • 安全性: ✅ 无漏洞
  • 测试覆盖: 本地测试
  • 文档完整性: NA

代码质量良好,使用模板函数优雅地支持了两种keypoint结构,类型安全且易于维护。


📋 修改概述

修复订阅场景keypoint结构问题,支持不同PMU模式下的数据结构。

  • 修改文件: 4个 (+36行, -15行)
  • 核心变更:
    • analyzer_ts.h: 将ParseTsKeypointData改为模板函数,支持多种keypoint类型
    • data_struct.h: 新增TsDavidKeypoint结构体,支持扩展PMU场景
    • analyzer_ffts.cpp: 修改taskId计算逻辑,支持extPmu模式
    • analyzer_ts.cpp: 重构keypoint解析逻辑,使用模板和类型转换

🔍 代码质量检查

1. 内存安全 ✅

  • 内存泄漏: 无风险 - 无动态内存分配
  • 指针操作: 安全 - 使用Utils::ReinterpretCast进行类型转换
  • 动态分配: 不适用 - 仅栈对象
  • 资源管理: RAII - 标准C++对象生命周期

2. 安全性 ✅

  • 输入验证: 完整 - 添加了remainingLen < sizeof(TsProfileKeypoint)检查
  • 边界检查: 完整 - 检查bufSize和timestamp有效性
  • 潜在漏洞: 无 - 类型转换安全,无缓冲区操作

3. 可读性 ✅

  • 代码清晰度: 优秀 - 使用模板提高代码复用性
  • 命名规范: 符合 - TsDavidKeypoint命名清晰
  • 注释完整性: 部分 - 添加了关键注释,但可增加更多

4. 逻辑正确性 ✅

  • 算法逻辑: 正确 - 根据IsExtPmu()选择正确的数据结构
  • 边界条件: 处理完整 - 添加了sizeof检查
  • 影响范围: 明确 - 仅影响keypoint解析模块

💡 改进建议

  1. 注释增强: TsDavidKeypoint结构体可添加注释说明各字段含义,特别是与TsProfileKeypoint的差异
  2. 单元测试: 建议补充单元测试覆盖IsExtPmu()的两种场景
  3. 常量定义: offsetTask常量(16)可考虑定义为有意义的宏或常量

✅ 代码亮点

  • 模板函数设计: 使用模板函数避免了代码重复,优雅地支持了两种keypoint结构
  • 类型安全: 使用ReinterpretCast而不是C风格的强制转换,更安全
  • 边界检查增强: 添加了sizeof(TsProfileKeypoint)检查,提高了鲁棒性
  • 扩展性好: 设计易于支持未来更多类型的keypoint结构

总体评价:代码质量良好,设计优雅,使用模板提高了代码复用性和类型安全性。建议合入。


⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ⚠️ Medium
审查时间: 2026-03-31 15:45


📊 审查结论

⚠️ 建议修改后合入

  • 严重性: Medium
  • 代码质量: 良好
  • 内存安全: ⚠️ 有风险
  • 安全性: ✅ 无漏洞
  • 测试覆盖: 本地测试
  • 文档完整性: NA

本次 PR 主要修复订阅场景下 keypoint 结构的兼容性问题,通过模板化设计支持两种不同的 keypoint 数据结构。代码结构清晰,但存在潜在的数据截断风险和类型安全性问题,建议修改后合入。


📋 修改概述

修复订阅场景 keypoint 结构问题,支持两种不同的 keypoint 数据格式(TsProfileKeypoint 和 TsDavidKeypoint)。

  • 修改文件: 4个 (+36行, -15行)
  • 核心变更:
    • analyzer_ts.h: 将 ParseTsKeypointData 改为模板函数
    • data_struct.h: 新增 TsDavidKeypoint 结构体定义
    • analyzer_ts.cpp: 根据 IsExtPmu() 判断使用哪种数据结构
    • analyzer_ffts.cpp: 修改 taskId 处理逻辑,支持 ExtPmu 场景

🔍 代码质量检查

1. 内存安全 ⚠️

  • 内存泄漏: ✅ 无风险(无新增动态内存分配)
  • 指针操作: ⚠️ 有风险(ReinterpretCast 需确保内存对齐)
  • 动态分配: ✅ 合理
  • 资源管理: ✅ RAII

主要问题

  1. taskId 数据截断风险 (analyzer_ts.cpp:154)

    opData.taskId = static_cast<uint16_t>(tsData->taskId);
    
    • TsDavidKeypoint.taskId 是 uint32_t (4字节)
    • KeypointOp.taskId 是 uint16_t (2字节)
    • 直接转换可能导致高16位数据丢失
    • 建议: 添加范围检查或使用更宽的类型
  2. ReinterpretCast 类型安全 (analyzer_ts.cpp:69-72)

    const TsDavidKeypoint *tsData =
        Utils::ReinterpretCast<const TsDavidKeypoint, const TsProfileDataHead>(tsHeader);
    
    • 依赖 TsProfileKeypoint 和 TsDavidKeypoint 大小相同(均为40字节)
    • 需确保内存对齐满足要求
    • 虽然有 bufSize 检查,但仍需谨慎

2. 安全性 ✅

  • 输入验证: ✅ 完整
  • 边界检查: ✅ 完整(新增了 remainingLen < sizeof(TsProfileKeypoint) 检查)
  • 潜在漏洞: ✅ 无

亮点

  1. 改进的边界检查 (analyzer_ts.cpp:55-56):

    // sizeof(TsProfileKeypoint) equal to sizeof(TsDavidKeypoint)
    if (remainingLen < tsHeader->bufSize || remainingLen < sizeof(TsProfileKeypoint)) {
    

    同时检查 bufSize 和结构体大小,更安全。

  2. 运行时验证 (analyzer_ts.cpp:132-133):

    if (tsData->head.bufSize != sizeof(T) || tsData->timestamp == 0) {
    

    检查 bufSize 和模板参数类型是否匹配。

3. 可读性 ✅

  • 代码清晰度: ✅ 良好
  • 命名规范: ✅ 符合
  • 注释完整性: ⚠️ 部分缺失

建议添加注释

  • 在 TsDavidKeypoint 结构体前添加注释说明与 TsProfileKeypoint 的区别
  • 说明为什么需要两种不同的结构体
  • 解释 tagId/streamId/taskId 字段顺序不同的原因

4. 逻辑正确性 ⚠️

  • 算法逻辑: ✅ 正确
  • 边界条件: ✅ 处理完整
  • 影响范围: ✅ 明确(仅影响订阅场景的 keypoint 数据解析)

潜在问题

  1. analyzer_ffts.cpp 中的 taskId 处理 (analyzer_ffts.cpp:102-104):

    constexpr uint32_t offsetTask = 16;
    uint32_t extTaskId = (static_cast<uint32_t>(taskId) << offsetTask | streamId);
    key = std::to_string(extTaskId);
    
    • taskId 是 uint16_t,左移16位后或上 streamId (uint16_t)
    • 这个组合逻辑是否与硬件/固件规范一致?
    • 建议: 添加注释说明这个组合逻辑的来源和用途
  2. IsExtPmu() 的一致性

    • 在解析 ts_track.data 和 ffts 数据时都会调用 IsExtPmu()
    • 需要确保在同一次 profiling 会话中,IsExtPmu() 的返回值保持一致
    • 建议: 在关键位置添加断言或日志,记录 IsExtPmu() 的状态

💡 改进建议

  1. 数据截断防护:

    // 在 analyzer_ts.cpp:154
    if (tsData->taskId > UINT16_MAX) {
        MSPROF_LOGW("taskId truncation: %u exceeds uint16_t range", tsData->taskId);
    }
    opData.taskId = static_cast<uint16_t>(tsData->taskId);
    
  2. 结构体大小断言:

    // 在 data_struct.h 中添加静态断言
    static_assert(sizeof(TsProfileKeypoint) == sizeof(TsDavidKeypoint),
                  "TsProfileKeypoint and TsDavidKeypoint must have same size");
    
  3. 字段顺序注释:

    // TsProfileKeypoint: streamId(2) + taskId(2) + tagId(2) + resv(2)
    // TsDavidKeypoint:   tagId(2) + streamId(2) + taskId(4)
    // 注意: 字段顺序不同,taskId 类型也不同
    
  4. taskId 组合逻辑注释 (analyzer_ffts.cpp):

    // ExtPmu 模式下,taskId 和 streamId 组合成唯一标识
    // 格式: [taskId(16bit)][streamId(16bit)]
    constexpr uint32_t offsetTask = 16;
    uint32_t extTaskId = (static_cast<uint32_t>(taskId) << offsetTask | streamId);
    
  5. 模板实例化验证:

    // 在 analyzer_ts.cpp 末尾,添加编译时检查
    static_assert(std::is_same<decltype(ParseTsKeypointData<TsProfileKeypoint>), 
                                decltype(ParseTsKeypointData<TsDavidKeypoint>)>::value,
                  "Template instantiation types must match");
    

✅ 代码亮点

  1. 模板化设计: 将 ParseTsKeypointData 改为模板函数,提高了代码复用性和类型安全性

  2. 运行时类型检查: 通过 bufSize 和 sizeof(T) 的比较,确保数据结构匹配

  3. 改进的边界检查: 新增了结构体大小的边界检查,更安全

  4. 清晰的条件分支: 使用 if-else 根据 IsExtPmu() 选择不同的数据结构,逻辑清晰

  5. 兼容性设计: 保持向后兼容,原有 TsProfileKeypoint 路径不受影响


🔧 测试建议

  1. 边界测试:

    • 测试 taskId > UINT16_MAX 的情况
    • 测试 bufSize 与实际结构体大小不匹配的情况
  2. 兼容性测试:

    • 分别测试 IsExtPmu() = true 和 false 两种场景
    • 验证两种 keypoint 结构的解析结果正确
  3. 数据完整性测试:

    • 验证 analyzer_ffts.cpp 中 taskId 组合逻辑的正确性
    • 确保组合后的 extTaskId 能正确匹配对应的 op 数据

📝 总体评价

这是一个针对订阅场景 keypoint 结构兼容性问题的修复 PR。代码采用了模板化设计,提高了类型安全性和代码复用性。主要的改进点包括:

  • 支持 TsProfileKeypoint 和 TsDavidKeypoint 两种数据结构
  • 改进了边界检查
  • 清晰的条件分支逻辑

存在的主要问题是 taskId 从 uint32_t 截断到 uint16_t 的风险,以及 ReinterpretCast 的类型安全性。建议添加数据范围检查和更多的注释说明。

总体来说,代码质量良好,修改后可以合入。


⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
zhangjie
zhangjie成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ✅ Low
审查时间: 2026-03-31 16:00


📊 审查结论

✅ 建议合入

代码质量良好,通过模板化改进了类型安全性,修复了订阅场景keypoint结构问题。建议合入。

详见审查报告:https://gitcode.com/cann/runtime/merge_requests/1335

likedislike
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ⚠️ Medium
审查时间: 2026-03-31 15:50


📊 审查结论

⚠️ 建议修改后合入

  • 严重性: Medium
  • 代码质量: 良好
  • 内存安全: ✅ 无风险
  • 安全性: ✅ 无漏洞
  • 测试覆盖: 本地测试
  • 文档完整性: NA

本次 PR 修复了订阅场景下 keypoint 结构问题,支持扩展 PMU 模式。代码重构为模板函数,设计合理,但存在一处潜在的数据截断问题需要确认。


📋 修改概述

本次 PR 主要修改 Profiling 模块的 keypoint 数据解析逻辑,支持扩展 PMU 模式下新的数据结构。

  • 修改文件: 4个 (+36行, -15行)
  • 核心变更:
    • analyzer_ts.h: 将 ParseTsKeypointData 改为模板函数,支持不同数据结构
    • data_struct.h: 新增 TsDavidKeypoint 结构体,用于扩展 PMU 模式
    • analyzer_ffts.cpp: 修改扩展 PMU 模式下的 taskId 组合逻辑
    • analyzer_ts.cpp: 重构 keypoint 解析逻辑,根据 PMU 模式选择数据结构

🔍 代码质量检查

1. 内存安全 ✅

  • 内存泄漏: 无风险
  • 指针操作: 使用 Utils::ReinterpretCast 进行类型转换,安全可控
  • 动态分配: 无动态内存分配
  • 资源管理: 模板函数设计合理,无资源泄漏风险

2. 安全性 ✅

  • 输入验证: 添加了缓冲区大小检查 remainingLen < sizeof(TsProfileKeypoint)
  • 边界检查: 完整,包含 bufSize 和结构体大小的双重验证
  • 潜在漏洞: 无明显安全问题

3. 可读性 ✅

  • 代码清晰度: 良好
  • 命名规范: 符合项目规范
  • 注释完整性: 添加了关键注释 "sizeof(TsProfileKeypoint) equal to sizeof(TsDavidKeypoint)"

4. 逻辑正确性 ⚠️

  • 算法逻辑: 基本正确,但有一处需要确认
  • 边界条件: 处理完整
  • 影响范围: 明确,仅影响 Profiling 模块

⚠️ 潜在问题

问题 1: taskId 数据截断 (Medium)

位置: analyzer_ts.cpp 第 157 行

opData.taskId = static_cast<uint16_t>(tsData->taskId);

问题描述:

  • TsDavidKeypoint.taskIduint32_t 类型
  • KeypointOp.taskIduint16_t 类型
  • 静态转换会截断高 16 位数据

分析:

  1. 在扩展 PMU 模式下,TsDavidKeypoint 的 taskId 可能超过 65535
  2. 虽然 keypoint 查找使用 modelId + indexId 作为 key,不依赖 taskId
  3. 但日志输出和调试时可能显示错误的 taskId 值
  4. 如果未来代码逻辑需要使用 taskId,可能导致匹配失败

对比 analyzer_ffts.cpp:

// 扩展 PMU 模式下,taskId 和 streamId 被组合为 32 位
uint32_t extTaskId = (static_cast<uint32_t>(taskId) << offsetTask | streamId);
key = std::to_string(extTaskId);

建议:

  1. 确认设计意图: 这个截断是否是有意为之?如果是,建议添加注释说明
  2. 如果 taskId 确实可能超过 65535: 建议修改 KeypointOp.taskIduint32_t
  3. 如果是有意截断: 建议添加明确的掩码操作,如 taskId & 0xFFFF,并添加注释说明

影响范围: 中等,影响日志准确性和未来扩展性

问题 2: 结构体字段顺序不一致 (Low)

位置: data_struct.h

TsProfileKeypoint:

uint16_t streamId;
uint16_t taskId;
uint16_t tagId;
uint16_t resv;

TsDavidKeypoint:

uint16_t tagId;
uint16_t streamId;
uint32_t taskId;

问题描述:

  • 两个结构体的字段顺序不同
  • 虽然总大小相同(都有 8 字节尾部字段),但字段位置不同

建议: 添加注释说明这是协议定义的顺序,不可随意调整


💡 改进建议

  1. 确认 taskId 截断:

    • 如果是有意为之,添加注释说明原因
    • 如果不是,修改 KeypointOp.taskId 类型为 uint32_t
  2. 添加单元测试:

    • 为扩展 PMU 模式添加单元测试
    • 覆盖 taskId 超过 65535 的场景
  3. 文档补充:

    • TsDavidKeypoint 结构体添加注释,说明与 TsProfileKeypoint 的区别
    • 说明字段顺序是协议定义,不可调整

✅ 代码亮点

  1. 良好的抽象设计: 使用模板函数支持多种数据结构,代码复用性好
  2. 完整的边界检查: 添加了 sizeof(TsProfileKeypoint) 的缓冲区检查
  3. 显式模板实例化: 在 .cpp 文件末尾显式实例化模板,避免链接错误
  4. 保持向后兼容: 通过 IsExtPmu() 运行时判断,兼容旧版本数据格式
  5. 类型安全: 使用 Utils::ReinterpretCast 替代 C 风格强制转换

📝 审查建议

总体评价: 代码质量良好,设计合理,建议修复 taskId 截断问题后合入。

优先级:

  1. : 确认 taskId 截断是否为设计意图
  2. : 添加相关注释说明
  3. : 补充单元测试

⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ✅ Low
审查时间: 2026-03-31 16:01


📊 审查结论

✅ 建议合入

  • 严重性: Low
  • 代码质量: 良好
  • 内存安全: ✅ 无风险
  • 安全性: ✅ 无漏洞
  • 测试覆盖: 本地测试
  • 文档完整性: 部分

代码质量良好,使用模板方法重构提升了代码的灵活性和可维护性,建议合入。


📋 修改概述

本 PR 修复了订阅场景下 keypoint 结构体解析问题,支持根据不同 PMU 类型使用不同的数据结构进行解析。

  • 修改文件: 4个 (+36行, -14行)
  • 核心变更:
    • analyzer_ts.h: 将 ParseTsKeypointData 方法改为模板方法
    • data_struct.h: 新增 TsDavidKeypoint 结构体定义
    • analyzer_ts.cpp: 重构 keypoint 解析逻辑,支持双结构体类型
    • analyzer_ffts.cpp: 优化 ExtPmu 场景下的 taskId 拼接方式

🔍 代码质量检查

1. 内存安全 ✅

  • 内存泄漏: 无风险 - 无动态内存分配
  • 指针操作: 安全 - 使用模板类型安全转换
  • 动态分配: 不适用 - 无 new/malloc 操作
  • 资源管理: 良好 - 使用栈对象和智能指针

验证要点:

// 添加了结构体大小检查,防止缓冲区不足
if (remainingLen < tsHeader->bufSize || remainingLen < sizeof(TsProfileKeypoint)) {
    break;
}

2. 安全性 ✅

  • 输入验证: 完整 - 检查 bufSize 和 timestamp
  • 边界检查: 完整 - 添加了 remainingLen 检查
  • 潜在漏洞: 无

关键安全检查:

// ParseTsKeypointData 中的验证
if (tsData->head.bufSize != sizeof(T) || tsData->timestamp == 0) {
    MSPROF_LOGE("keypoint op error...");
    return;
}

3. 可读性 ✅

  • 代码清晰度: 良好 - 模板方法设计清晰
  • 命名规范: 符合 - TsDavidKeypoint 命名明确
  • 注释完整性: 部分 - 添加了大小相等的注释

4. 逻辑正确性 ✅

  • 算法逻辑: 正确 - 根据设备类型选择正确的结构体
  • 边界条件: 处理完整 - 添加了长度检查
  • 影响范围: 明确 - 仅影响 TS keypoint 数据解析

💡 改进建议

  1. 类型转换注意: TsDavidKeypoint::taskIduint32_t,而 KeypointOp::taskIduint16_tstatic_cast<uint16_t>(tsData->taskId) 可能截断数据。如果实际 taskId 值不会超过 uint16_t 范围则无问题。

  2. 字段顺序文档化: TsProfileKeypointTsDavidKeypoint 字段顺序不同,建议在 data_struct.h 中添加注释说明设计意图。


✅ 代码亮点

  • 模板方法设计: 使用模板重构 ParseTsKeypointData,避免代码重复,提高类型安全性
  • 显式模板实例化: 明确实例化 TsProfileKeypointTsDavidKeypoint 版本,确保编译时类型检查
  • 边界检查增强: 添加 remainingLen < sizeof(TsProfileKeypoint) 检查,防止缓冲区越界
  • 一致性: analyzer_ffts.cpp 中 ExtPmu 场景使用相同的 taskId 拼接策略

总体评价:代码质量良好,设计清晰,安全检查到位,可以合入。


⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
CANN-robot
CANN-robot成员
3月31日 评论:

🤖 CANN 代码审查报告

PR: #1335 - 【Profiling】【同步9.0.0】修复订阅场景keypoint结构问题
严重性: ⚠️ Medium
审查时间: 2026-03-31 16:12


📊 审查结论

⚠️ 建议修改后合入

  • 严重性: Medium
  • 代码质量: 良好
  • 内存安全: ✅ 无风险
  • 安全性: ⚠️ 有隐患
  • 测试覆盖: 部分(本地测试)
  • 文档完整性: NA

本次修改通过引入模板化设计支持两种不同的 keypoint 数据结构,解决了订阅场景下的结构问题。代码逻辑清晰,但存在 taskId 类型转换的潜在风险,建议确认数据范围或调整存储类型。


📋 修改概述

本次 PR 修复了 Profiling 订阅场景下的 keypoint 结构解析问题,主要变更包括:

  • 修改文件: 4个 (+36行, -15行)
  • 核心变更:
    • data_struct.h: 新增 TsDavidKeypoint 结构体,支持 32 位 taskId
    • analyzer_ts.h: 将 ParseTsKeypointData 改为模板函数
    • analyzer_ts.cpp: 实现 keypoint 数据的模板化解析逻辑
    • analyzer_ffts.cpp: ExtPmu 模式下修改 taskId 的组合方式

🔍 代码质量检查

1. 内存安全 ✅

  • 内存泄漏: ✅ 无风险 - 无动态内存分配
  • 指针操作: ✅ 安全 - 使用类型安全的模板转换
  • 动态分配: ✅ NA - 无动态内存管理
  • 资源管理: ✅ RAII - 使用栈对象和标准容器

详细分析

  • 使用模板函数避免了重复代码,提高了类型安全性
  • Utils::ReinterpretCast 提供了类型安全的指针转换
  • 边界检查充分:remainingLen < tsHeader->bufSize || remainingLen < sizeof(TsProfileKeypoint)

2. 安全性 ⚠️

  • 输入验证: ⚠️ 部分 - 有长度检查,但类型转换有隐患
  • 边界检查: ✅ 完整 - 添加了结构体大小的双重检查
  • 潜在漏洞: ⚠️ 数据截断风险

关键问题:taskId 类型转换

analyzer_ts.cpp 第 157 行:

opData.taskId = static_cast<uint16_t>(tsData->taskId);

问题描述

  • TsDavidKeypoint.taskIduint32_t (4字节,范围 0-4,294,967,295)
  • KeypointOp.taskIduint16_t (2字节,范围 0-65,535)
  • 强制转换会导致高位截断,如果 taskId > 65535 会丢失数据

关联代码分析
analyzer_ffts.cpp 中,ExtPmu 模式下 taskId 的生成逻辑:

uint32_t extTaskId = (static_cast<uint32_t>(taskId) << offsetTask | streamId);

这表明 ExtPmu 模式下 taskId 确实是 32 位组合值(taskId << 16 | streamId)。

潜在影响

  • 如果 extTaskId > 65535,存储到 KeypointOp 时会丢失高 16 位
  • 可能导致后续的 keypoint 匹配失败或数据不一致

建议方案

  1. 方案一:确认数据范围 - 如果实际 taskId 不会超过 65535,添加注释说明
  2. 方案二:修改数据结构 - 将 KeypointOp::taskId 改为 uint32_t
  3. 方案三:分离存储 - ExtPmu 模式下使用不同的存储逻辑或字段

3. 可读性 ✅

  • 代码清晰度: ✅ 优秀 - 逻辑清晰,命名规范
  • 命名规范: ✅ 符合 - 遵循项目命名约定
  • 注释完整性: ⚠️ 部分 - 关键逻辑缺少注释

优点

  • 模板函数设计简洁优雅,避免了代码重复
  • 新增结构体 TsDavidKeypoint 命名清晰,表明了 David 架构的专用数据结构

改进建议

  • 建议在 ParseTsKeypointDatastatic_cast 处添加注释,说明类型转换的原因和范围
  • 建议在 TsDavidKeypoint 结构体定义处添加注释,说明与 TsProfileKeypoint 的区别和使用场景

4. 逻辑正确性 ✅

  • 算法逻辑: ✅ 正确 - 模板化设计合理
  • 边界条件: ✅ 处理完整 - 添加了结构体大小的双重检查
  • 影响范围: ✅ 明确 - 仅影响 ExtPmu 模式的 keypoint 解析

详细分析

  • 修改前:remainingLen < tsHeader->bufSize
  • 修改后:remainingLen < tsHeader->bufSize || remainingLen < sizeof(TsProfileKeypoint)
  • 新增检查确保剩余数据至少能容纳一个完整的 keypoint 结构体

结构体大小验证

  • TsProfileKeypoint: 8 (head) + 8 + 8 + 8 + 2 + 2 + 2 + 2 = 40 bytes
  • TsDavidKeypoint: 8 (head) + 8 + 8 + 8 + 2 + 2 + 4 = 40 bytes
  • 两者大小相同,边界检查逻辑正确

💡 改进建议

  1. 类型转换风险处理(高优先级):

    // 建议在 analyzer_ts.cpp 中添加范围检查
    if (tsData->taskId > UINT16_MAX) {
        MSPROF_LOGW("taskId exceeds uint16_t range: %u, may lose data", tsData->taskId);
    }
    opData.taskId = static_cast<uint16_t>(tsData->taskId);
    
  2. 数据结构优化(中优先级):

    • 评估是否需要将 KeypointOp::taskId 改为 uint32_t
    • 如果修改,需要同步修改所有使用该字段的地方
  3. 代码注释完善(低优先级):

    // ExtPmu mode uses TsDavidKeypoint with 32-bit taskId (taskId << 16 | streamId)
    // Non-ExtPmu mode uses TsProfileKeypoint with 16-bit taskId
    struct TsDavidKeypoint {
        // ... fields
    };
    
  4. 测试建议

    • 添加 taskId 边界值测试(65535, 65536, 0xFFFFFFFF)
    • 添加 ExtPmu 模式的集成测试
    • 验证 keypoint 匹配逻辑在 taskId 截断情况下的行为

✅ 代码亮点

  • 模板化设计:将 ParseTsKeypointData 改为模板函数,避免了代码重复,提高了可维护性
  • 类型安全:使用 Utils::ReinterpretCast 进行类型转换,比 C 风格的强制转换更安全
  • 边界检查增强:添加了结构体大小的双重检查,提高了解析的健壮性
  • 架构分离清晰:通过 IsExtPmu() 明确区分了两种不同的解析路径

总体评价:代码质量良好,设计合理,但 taskId 类型转换存在潜在风险。建议确认数据范围或添加保护逻辑后合入。


⚠️ 声明: 本审查报告由 AI 自动生成,仅供参考。建议人工复核后再做最终决定。

likedislike
chenhao_1209成员
3月31日 评论:

/lgtm

likedislike
CANN-robotCANN-robot成员
3月31日 添加了label:lgtm
CANN-robot
CANN-robot成员
3月31日 评论:

Review Guide

This pull-request passes review.
Committers who wrote a comment of /approve are: zhuliangying.
Reviewers who wrote a comment of /lgtm are: zhuliangying, chenhao_1209.

likedislike
CANN-robotCANN-robot成员
3月31日 合入了pull request