qryxip commented 1 year ago

内容

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

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

Cons 悪くなる点

218 の効果が若干薄くなる

実現方法

❯ 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の種類/ディストリ/バージョン

[x] Windows
[x] macOS
[x] Linux

その他

Hiroshiba commented 1 year ago

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

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

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

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

qryxip commented 1 year ago

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

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

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

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

Hiroshiba commented 1 year ago

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

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

qryxip commented 1 year ago

561 を作りました。

561 とは別にhttps://github.com/VOICEVOX/voicevox_core/issues/556#issuecomment-1662386617に気付いてしまったので、このissueは残したままになるかと思います。

VOICEVOX / voicevox_core

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

内容

Pros 良くなる点

Cons 悪くなる点

218 の効果が若干薄くなる

実現方法

VOICEVOXのバージョン

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

その他

561 を作りました。

561 とは別にhttps://github.com/VOICEVOX/voicevox_core/issues/556#issuecomment-1662386617に気付いてしまったので、このissueは残したままになるかと思います。