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.

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

C-unwind ABIに切り替える #541

Open
3 tasks done
qryxip opened this issue Jul 14, 2023 · 4 comments
Open
3 tasks done

C-unwind ABIに切り替える #541

qryxip opened this issue Jul 14, 2023 · 4 comments

Comments

@qryxip
Copy link
Member

qryxip commented Jul 14, 2023

内容

C APIのextern "C"を、Rust 1.71で使えるようになったextern "C-unwind"に置き換えます。

C-unwind ABI - Announcing Rust 1.71.0 - Rust Blog

これにより、C APIも #505 のようにできることが期待できます。

Pros 良くなる点

  • C APIでパニックが発生した場合も、プロセスの強制終了とならずにFFIの呼び出し側でキャッチできる? (未調査)
    • というより extern "C"からパニックを送出しようとする行為がそもそもUBらしく、プロセスの強制終了も実は保証されている動作ではなかったらしい?
      (追記) Rustのパニックを出したときの挙動は保証されていて、コールバックからC++の例外とかが発射されたときがUBだったらしい (例えpanic=abortであっても)
      panic runtime ABI panic-unwind Unforced foreign unwind
      panic=unwind "C-unwind" unwind unwind
      panic=unwind "C"-like abort UB
      panic=abort "C-unwind" panic! aborts abort
      panic=abort "C"-like panic! aborts (no unwinding occurs) UB
      2945-c-unwind-abi - The Rust RFC Book

Cons 悪くなる点

実現方法

sed -i 's/extern "C"/extern "C-unwind"/' ./crates/voicevox_core_c_api/src/lib.rs && cargo fmt
diff --git a/.github/workflows/build_and_deploy.yml b/.github/workflows/build_and_deploy.yml
index cbb3ab2..4408fe6 100644
--- a/.github/workflows/build_and_deploy.yml
+++ b/.github/workflows/build_and_deploy.yml
@@ -205,7 +205,6 @@ jobs:
             build > /dev/null 2>&1
           fi
         env:
-          RUSTFLAGS: -C panic=abort
           ORT_USE_CUDA: ${{ matrix.use_cuda }}
       - name: build voicevox_core_python_api
         if: matrix.whl_local_version

VOICEVOXのバージョン

N/A

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

@Hiroshiba
Copy link
Member

issue作成ありがとうございます!

正直C-unwindに切り替えると何ができるようになるのかが分からないなと思いました!
例えばPythonでC APIを使った時に、try~catchできるとかなら便利そうですが、実際そうなのかどうかちょっとよくわからないんですよね。Pythonのctypesの実装による・・・?

なのでまあメリットはぶっちゃけわからんなと思いました!
ただデメリットは(ほぼ)ないと思うので、そういう意味では導入をしても別にいいのかなという気持ちです。

プルリクいただけたらマージすると思います!

@qryxip
Copy link
Member Author

qryxip commented Aug 1, 2023

よく考えたら当たり前だったのですが、RustのパニックはそのままC++の例外とかとしては解釈されません。

なのでvoicevox_on_panic(void (*hook)(const *VoicevoxRustPanicInfo))みたいなAPIを生やし、RustのパニックをC++の例外とかに変換するフックを用意する形になると思います。
(参考: std::panic::set_hook)

この形式がちゃんと動作するかですが、試した人が既におり、ちゃんと動くと言ってよいように思えました。

Rustを経由してC++例外を補足する

@qryxip qryxip mentioned this issue Aug 1, 2023
3 tasks
@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 2, 2023

なるほどです!
この関数を利用するほど固いコードを書く人がいるのか少し疑問ですが、100行くらいの追加で実装できるならまあありかなと感じました。
テストがめちゃくちゃ大変そうですね…!

このissueはcloseして新たにissueを作る感じでしょうか?

@qryxip
Copy link
Member Author

qryxip commented Aug 2, 2023

#561 を作りました。

#561 とは別に#556 (comment)に気付いてしまったので、このissueは残したままになるかと思います。

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

No branches or pull requests

2 participants