Pull Request已成功合入, 合并人@CANN-robot
(感谢 范其瑞 的贡献)以下是根据您提交的修改文件推荐的Reviewer和Committer序列,需各模块评审通过后方可合入
| Module List | Reviewers | Committers |
|---|---|---|
| sig-ops-nn | fanqirui, lileizheng, liu-wei, FelixTang7, zhang-wu | chenqi317, liubo75, crystalhu, tangweiwei2 |


🔵 source code change are detected, tasks labels is removed in this pull request!


问题/功能描述
本次PR主要解决了两个核心问题:1)在单元测试(UT)场景下,LegacyCommonMgr 类因编译时引入了不必要的源码文件,导致与打桩逻辑冲突,无法进行有效测试。2)在GE的ATC转OM场景下,OM推理时 libophost_comm_legacy.so 共享库被释放到临时目录且可能被删除,导致原有基于环境变量的路径查找方法失效,影响功能正常运行。
修改方案描述
修改方案包含两部分:1)构建配置优化:在CMake脚本中增加条件判断,确保UT场景下仅编译必要的源文件,避免引入冲突代码,为打桩测试创造条件。2)核心逻辑重构:对 LegacyCommonMgr 类进行重构,将其从单例模式改为可构造实例,并彻底重写了动态库路径查找逻辑。新方案摒弃了单一环境变量依赖,转而根据当前SO文件自身路径,通过 GetSoPathForBuiltin、GetSoPathForCustomOp 和 GetSoPathForOm 三个专用函数,分别针对内置算子、自定义算子和OM推理三种场景进行智能路径推断与拼接,从而在多环境下可靠定位目标库。同时,为UT场景提供了独立的桩函数实现,确保测试可行性。


指针与引用安全: 函数GetSoPathForCustomOp的第一个参数parentPath是引用传递,并在函数内部被修改(第137-141行拼接路径时使用了parentPath)。然而,该参数在函数签名中被标记为const std::string&(第112行),但在第130行的函数声明中,parentPath参数类型为std::string&(非const引用)。这导致了不一致的接口设计,并且与GetSoPathForBuiltin函数的参数类型不一致。虽然当前调用GetSoPathForCustomOp时传入的parentPath是GetParentPath返回的变量(第69行),可以被修改,但这种设计降低了代码的可读性和可维护性,容易引起误解。
问题类型: 指针与引用安全
文件路径: common/src/legacy_common_manager.cpp
行号: 130
问题代码:
bool LegacyCommonMgr::GetSoPathForCustomOp(
std::string& parentPath, const std::string& currSoName, std::string& soPath) const
修改建议:
统一函数参数设计。建议将GetSoPathForCustomOp的第一个参数改为const std::string& parentPath,以保持与GetSoPathForBuiltin函数的一致性,并明确表示该函数不会修改传入的路径。如果确实需要修改parentPath(从代码看只是用于拼接,不需要修改),应该使用局部变量。
此评论由代码审查工具自动生成


代码结构与可维护性: GetSoPathForCustomOp函数中使用了硬编码的相对路径字符串(/../../../../../../../../../../opp),这是一个魔法字符串,缺乏解释性且难以维护。这种深层相对路径依赖特定的目录结构,如果目录层级发生变化,代码将无法正确工作。
问题类型: 代码结构与可维护性
文件路径: common/src/legacy_common_manager.cpp
行号: 137
问题代码:
// 自定义算子场景需要先向上找到opp入口
const char* oppPath = "/../../../../../../../../../../opp";
const char* ophostPath = "/built-in/op_impl/ai_core/tbe/op_host/lib/linux/";
修改建议:
将这个魔法字符串定义为有意义的常量,并添加详细注释说明其对应的目录结构。更好的做法是通过配置文件或环境变量来获取这个路径关系,或者使用更可靠的文件系统遍历方法来定位opp目录。
此评论由代码审查工具自动生成


逻辑运算与副作用: 在GetLegacyCommonSoPath函数中,第64行对currSoName的检查使用了逻辑与(&&)运算符,要求currSoName既不是BUILTIN_SO_NAME也不是CUSTOM_SO_NAME时才返回false。然而,根据第69-70行的逻辑,函数会尝试三种不同的路径查找方法:GetSoPathForBuiltin、GetSoPathForCustomOp和GetSoPathForOm。前两个方法内部已经检查了currSoName是否匹配(第115行和第133行),只有匹配时才会真正尝试查找。因此,第64行的检查实际上是多余的,而且可能会错误地拒绝有效的查找场景(例如,如果未来增加了新的so名称,但查找逻辑仍然有效)。
问题类型: 逻辑运算与副作用
文件路径: common/src/legacy_common_manager.cpp
行号: 64
问题代码:
if (currSoName != BUILTIN_SO_NAME && currSoName != CUSTOM_SO_NAME) {
OP_LOGW("LegacyCommonMgr", "Invalid current so name[%s].", currSoName.c_str());
return false;
}
修改建议:
移除第64-67行的检查,让后续的查找逻辑自然地处理不同的so名称。或者,将检查改为更宽松的警告而不是错误返回,因为GetSoPathForOm方法可能仍然有效。
此评论由代码审查工具自动生成


缓冲区溢出风险: 在GetParentPath函数中,使用realpath处理dlInfo.dli_fname,但dlInfo.dli_fname可能超过PATH_MAX长度。虽然realpath函数会检查缓冲区大小,但如果路径过长,realpath可能返回nullptr。代码中仅检查了realpath返回值是否为nullptr,但没有处理ERANGE错误情况。
问题类型: 缓冲区溢出风险
文件路径: common/src/legacy_common_manager.cpp
行号: 95
问题代码:
char soPath[PATH_MAX] = {0};
if (realpath(dlInfo.dli_fname, soPath) == nullptr) {
OP_LOGW("LegacyCommonMgr", "Fail to get realpath of [%s].", dlInfo.dli_fname);
return false;
}
修改建议:
1) 考虑使用realpath的POSIX版本,传递nullptr作为第一个参数让系统分配缓冲区;2) 或者先使用lstat检查路径长度;3) 至少记录errno以区分不同的失败原因。
此评论由代码审查工具自动生成


API误用: GetSoPathForCustomOp函数的参数parentPath被声明为非常量引用,但函数内部并没有修改它。这可能导致调用者误以为该参数会被修改。函数签名应该明确表达意图。
问题类型: API误用
文件路径: common/src/legacy_common_manager.cpp
行号: 130
问题代码:
bool LegacyCommonMgr::GetSoPathForCustomOp(
std::string& parentPath, const std::string& currSoName, std::string& soPath) const
修改建议:
将parentPath参数改为const std::string&,以明确表示该参数是输入参数且不会被修改。
此评论由代码审查工具自动生成


以下是根据您提交的修改文件推荐的Reviewer和Committer序列,需各模块评审通过后方可合入
| Module List | Reviewers | Committers |
|---|---|---|
| sig-ops-nn | wangyongguang, yu-xinjie62, zhajianqing123, zhangyuxiang0119, zhou-qilong | liubo75, crystalhu, tangweiwei2, chenqi317 |


🔵 source code change are detected, tasks labels is removed in this pull request!


compile


🔵 ops-nn pipeline is running. Please wait a moment... (Link 1769)


| Check Name | Build Result | Log Report | Package Link |
|---|---|---|---|
| CI_Ascend_Ops-Nn_Compile | ✅ SUCCESS | #1769 | >>> |
| check-commit-message | ✅ SUCCESS | >>> | |
| download-business-code | ✅ SUCCESS | >>> | |
| Compile_Ascend_X86 | ✅ SUCCESS | >>> | >>> |
| Compile_Ascend_ARM | ✅ SUCCESS | >>> | >>> |
| Compile_Ascend_ARM-single | ✅ SUCCESS | >>> | >>> |
| Check_pr | ✅ SUCCESS | >>> | |
| UT_Test_ophost | ✅ SUCCESS | >>> | |
| UT_Test_opapi | ✅ SUCCESS | >>> | |
| UT_Test_kernel | ✅ SUCCESS | >>> | |
| Smoke_Test_A900 | ✅ SUCCESS | >>> |


/lgtm


Review Guide
This Pull-Request Passes Review.
Committers who writed a comment of /approve are: chenqi317.
Reviewers who writed a comment of /lgtm are: lileizheng, chenqi317.


描述
1、安全整改:使用dladdr替代环境变量
2、支持OM离线推理场景下的so查找
关联的Issue
测试
文档更新
类型标签