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

[project-vvm-async-api] output_系引数がunalignedであることを許す #534

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jul 1, 2023

内容

C APIにおけるoutput_系のポインタがunalignedであることを許可します。

Rustの参照 (reference)有効 (valid)であると同時にアラインメントに沿って (aligned)いなければなりません。そうでなければRustにおける未定義動作 (undefined behavior)となります。そのためC側から渡される生ポインタ (raw pointer)を参照として解釈するときは、アラインメントにも注意する必要があります。

Cのconst VoicevoxSynthesizer*&VoicevoxSynthesizerとして、VoicevoxSynthesizer*Box<VoicevoxSynthesizer>として解釈するのにはアラインメント的な問題はありません。何故ならVoicevoxSynthesizerを作れるのはVOICEVOX COREだけであり、ユーザーはVoicevoxSynthesizerの実体に触れないようになっているため、「有効なVoicevoxSynthesizerのポインタ」がRustの世界で生まれたalignedなものであると言えるからです。またconst char*&CStrとして読むときもアラインメントを気にする必要はありません。c_charは1バイトだからです。

しかしオブジェクトを"new"したり"create"したりする先のT**/*mut *mut Tは別です。ドキュメントを通じてユーザーにお願いしない限り、(T*)*としてのアラインメントに沿わない (unaligned)ポインタが渡ってくる可能性があります。

このPRではC APIのoutput_系の引数をalignedだと仮定せず、&mutを経由しないように注意して<*mut T>::write_unalignedで"new"/"create"対象を書き込むようにします。

最初「unalignedなポインタをよこすな」というドキュメントを書けばいいと思っていたのですが、実際に #532 で書こうとしたら我々とユーザーの両方に面倒を生じさせることに気づいてしまいました。アラインメントを気にする必要があるのはoutput_系の引数だけなので、write_unalignedする方が良いと思いました。

関連 Issue

その他

Copy link
Member Author

@qryxip qryxip Jul 1, 2023

Choose a reason for hiding this comment

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

APIとしてのドキュメントの変更についてはすべて #532 に回すことにします(現段階ではそもそもsafetyについて正しいことが書かれていないため)。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

うーんなるほど!!奥が深いですね・・・。

@Hiroshiba Hiroshiba merged commit b8b8484 into VOICEVOX:project-vvm-async-api Jul 1, 2023
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Jul 1, 2023
Hiroshiba pushed a commit that referenced this pull request Jul 2, 2023
qryxip added a commit to qryxip/voicevox_core that referenced this pull request Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants