[project-vvm-async-api] "buffer"をRustの世界で保持し続ける

[qryxip]

内容

BufferManagerに対するリファクタです。

• BufferManagerでLayoutを持つのをやめ、UnsafeCellでBox<[u8]>/CStringを直接保持し続けます。

  Feature/c api cut memcpy #392 (comment)の考えです。

  Rustの世界でowned型として保持しながらCの世界に*mut _で貸し出すわけにはいかないというのもあって選択肢から外してしまったのですが、UnsafeCellで包んでしまえば(多分)大丈夫なことに気づいてしまいました。

  これによりvoicevox_wav_free/voicevox_json_freeはRustの世界でunsafeではなくなります。デストラクトするのにUnsafeCellをただdropするだけでよくなるからです。

• BufferManagerを解体し、SliceOwner<T>とCStringOwnerに分解します。

  [u8]とCStrを別物として扱い始めた時点で、役割は分担可能だったのだからこうすべきでした。

  名前としては"owner"としました。何故なら正真正銘バッファをown(所有)しているからです。C側にはポインタを参照(reference)として貸し出し(lend)ます。

関連 Issue

• Feature/c api cut memcpy #392
  • Fix up #392 #478
  • 間違ったchar*の解放を明示的に拒否する #500
  • [project-vvm-async-api] Fix up #500 #521

その他

正直なところ私自身C FFIへの経験/知識が薄く、手探りでやっている部分が大きいです。今回も #392 の時点でここまでわかっていれば...との思いがあります。

[qryxip]

とりあえずdocを書いたのでdraftを外します。

[Hiroshiba]

ほぼLGTMです！

[qryxip]

すみません、二転三転して申し訳ないのですが、CStringOwnerについては別の形がいいかなと思い始めました。

というのもCStr/CStringのドキュメントを見たところ、「Rustの世界のオブジェクトとして生存したまま可能な可変操作」が一切提供されていないことに気付いてしまいました。つまり"正しく"可変操作するためにはCString::into_raw/CString::from_rawを経由するしかありません。
(標準ライブラリの実装上はCStrは[c_char]に等しいため今のところ問題は考えられないのですが、Unsafe Rustには「正式にドキュメント化されていない内部実装の性質」に頼ってはいけないという鉄則があります)

そこで回避策を考えているうちに思ったのですが、CStringに関してはそもそも"owner"とか"manager"といった枠組みで考えないほうがいいかもしれません。

今owner.rsには"owner"の必要性について長々と書いていると思うのですが、CStringはよく考えたら関係ありません。長さをこちら側で管理する必要は無くポインタ一つで実体を表すことができるので、本来は以下のような形で十分なはずです。

#[no_mangle]
pub extern "C" fn new_string() -> *mut c_char {
    CString::new("Hello!").unwrap().into_raw() // `into_raw`でownershipをC側に移し…
}

/// # Safety
///
/// - `s`は`new_string`で得たものである。
#[no_mangle]
pub unsafe extern "C" fn delete_string(s: *mut c_char) {
    drop(CString::from_raw(s)); // C側から戻ってきたownershipを受け取って`from_raw`
}

それができない理由は我々が #500 をやりたいから、それだけです。であるならユーザーに与える文字列そのものは"own"も"manage"もせずに「間違えた解放に対する拒否」ができればいいはずです。CStringDropCheckerとかVerifierとかAuditorとかWhiteBlackListみたいな名前の構造体にすれば丸く収まり、責務も簡潔になるのではないかと思いました。

(追記) そもそもこの部分が要らず、今となってはvoicevox_error_result_to_messageの文字列だけ弾ければいいかもしれません。考えられる懸念は#370 (comment)くらいのもので、二重解放やvoicevox_coreに関係無いポインタの投入はケアしなくていのかも... そしたら構造体も要らずvoicevox_json_free内でベタ書きできるんじゃないかと。

voicevox_core/crates/voicevox_core_c_api/src/helpers.rs

Lines 279 to 282 in 0a0cf99

	panic!(
	"解放しようとしたポインタはvoicevox_coreの管理下にありません。\
	誤ったポインタであるか、二重解放になっていることが考えられます",
	);

[Hiroshiba]

@qryxip なるほどです！　CStringOwnerを不要にできるのは大きいですね！
CStringDropCheckerにするの、良い気がします。
owner.rsが不要になってSliceOwnerだけになるのでドキュメントも簡単になりますし。

voicevox_error_result_to_messageのCStringをブラックリストとして持っておく手もなるほどと思いました。
ただ新しくそういうCStringが現れた時におそらく追加し忘れるオチだろうな～と思いました。
ブラックリストを保持するクラスを作るのと、CStringDropCheckerを作るのはそこまで手間変わらなそうですし、こっちのが良さそうかも。

[qryxip]

CStringDropCheckerにしました。

[qryxip]

あくまでチェックということで、MaybeUninitへの書き込みとCString::{from,into}_rawをここから外に出しました。

メソッド名および概念は"whitelist"(動詞兼名詞)と"blacklist"(動詞兼名詞)としてみました。

[Hiroshiba]

将来なにか関数増えたときにcheck通すの忘れそうですね。。
仕組みで解決できないので、まあ思い出すしか無さそう。

[qryxip]

ですね... ベタ書きしたものをCの世界に向けて発射できる以上、どうしようもない気がします。

[Hiroshiba]

LGTM！！！！

見通しが上がったように感じます、ありがとうございます！！