Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

添加其他平台的编译支持 #4

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

js2xxx
Copy link

@js2xxx js2xxx commented Mar 10, 2023

这样可以使得添加了该项目的crates在编译平台(比如x86)上运行不依赖SBI的单元测试,而不需要额外编写其他代码。

@duskmoon314 duskmoon314 requested a review from YdrMaster March 10, 2023 12:20
@duskmoon314
Copy link
Member

duskmoon314 commented Mar 10, 2023

在 x86 上跑测试的意义是啥?测试应该要看能否在 rv 上用吧


我不清楚其他平台提供一个 unimplemented、其他平台直接 cfg 屏蔽这些函数与当前的做法哪个比较好?

@YdrMaster 看看?

@js2xxx
Copy link
Author

js2xxx commented Mar 10, 2023

有一些平台无关的模块需要编写单元测试,但是RISC-V还不支持标准库,所以在编译平台上运行是一个比较好的选择。
如果所有测试都直接写在实际运行代码里,就需要额外的编译选项。

@YdrMaster
Copy link
Member

YdrMaster commented Mar 11, 2023

不理解,这个项目哪有平台无关模块……


这个项目的定位就是 rv 上低于 machine 的特权态调用 sbi 的简单封装,避免反复写汇编;另外提供 SbiRet 的定义。如果有复杂到需要测试的功能应该拆分到其他项目。

而且 legacy 里还有一把汇编啊……

@js2xxx
Copy link
Author

js2xxx commented Mar 11, 2023

只是从易用性上考虑。如果不加编译支持的话,所有添加了这个项目的crate就都运行不了单元测试了;或者说要把测试代码单独分到其他地方。这样增加复杂性从该项目的使用者的角度来说感觉不太方便。

@YdrMaster
Copy link
Member

YdrMaster commented Mar 11, 2023

所以是依赖这个项目的其他项目的问题,那为什么不考虑在调用处设置 cfg 呢?看起来是一样的,说不定要设置的还更少(如果实际使用到的调用更少的话)。按照这个标准设置的话,岂不是在要求所有 crate 都能在任何平台上编译?毕竟上游是没法干涉下游怎么使用的。


试试用这个依赖:

[target.'cfg(any(target_arch = "riscv32", target_arch = "riscv64"))'.dependencies]

@@ -28,7 +28,7 @@ default = []
# Implement sbi-rt traits for integer types
# By using this feature, parameter types of sbi-rt functions fall back to integers,
# static type checks are disabled so this library won't detect parameters in incorrect orders.
# Although some people may find it userul in prototyping sbi-rt implementations,
# Although some people may find it useful in prototyping sbi-rt implementations,
Copy link
Member

Choose a reason for hiding this comment

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

有道理,可以单独改一下

@js2xxx
Copy link
Author

js2xxx commented Mar 11, 2023

也不是说所有crate都能跨平台编译吧,至少我在crates.io上嵌入式板块里看到的主流crate都支持在编译平台上编译,可能为的也是通用性吧。比如:

[target.'cfg(any(target_arch = "riscv32", target_arch = "riscv64"))'.dependencies]

我也在考虑如此操作,不过最终还是避免不了在代码里设置大量的cfg

@YdrMaster
Copy link
Member

我们讨论了一下,对这个修改的意见基本上是负面的。不过作为参考,也许可以试试这些方案:

  1. [target.'cfg(any(target_arch = "riscv32", target_arch = "riscv64"))'.dependencies]。理论上说,平台 cfg 应该包含尽量多的内容。不太可能只有 sbi 调用是平台相关的,需要调用的上下文、以及返回值处理等逻辑一定也是。应该平台相关依赖,并且为所有平台相关逻辑设置 cfg。
  2. 尝试 sbi-spec。这是 sbi-rt 的依赖,提供 sbi 规范定义的常量和结构体。我们假定所有需要 sbi 相关信息,但不需要在 RISC-V 上运行的项目(例如在其他平台模拟 RISC-V,或解析 RISC-V 文件)会使用这个 crate。
  3. 依赖你的 fork,并保留这个 pr。也许我们以后会有不同的理解。
  4. 如果方便的话,可以提供你实际使用的代码。也许具体场景能帮助我们理解这么做的必要性。

BTY,以下是我的个人观点:

riscv 不是一个设计良好的 crate,很多方面,所以不建议拿他当参考。

@js2xxx
Copy link
Author

js2xxx commented Mar 11, 2023

好的,我暂时先采用第三种方案了,之后如果有新的想法会继续在这里发布。感谢review。

@YdrMaster YdrMaster marked this pull request as draft March 13, 2023 00:54
@YdrMaster YdrMaster added the wontfix This will not be worked on label Mar 13, 2023
@luojia65 luojia65 force-pushed the main branch 3 times, most recently from cb56b4b to 4a90a5f Compare December 8, 2023 12:59
@luojia65
Copy link
Member

@js2xxx 您好:由于本项目已经被合并到rustsbi/rustsbi仓库统一管理,若后续有贡献计划,请移交到rustsbi/rustsbi仓库。谢谢您的理解!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants