search

TheDan64 / inkwell

It's a New Kind of Wrapper for Exposing LLVM (Safely)
https://thedan64.github.io/inkwell/
Apache License 2.0
2.32k stars 224 forks source link

Wrong thread-safety trait implementing on LLVMString #488

Open GCPanic opened 5 months ago

GCPanic commented 5 months ago

Describe the Bug Currently, inkwell::support::LLVMString implementing !Send, !Sync and Deref<Target = CStr>. This is clearly incorrect, as CStr implementing Send and Sync.

To Reproduce Unrelated

Expected Behavior LLVMString is just an CStr with custom Drop, so it should be safe to send it's reference to other threads.

On the other hands, i've searched some informations about LLVM Messages, and i don't found any documents saying it's not thread-safe. In this case, LLVMString should implement Send as well.

LLVM Version (please complete the following information):

Desktop (please complete the following information):

Additional Context None

TheDan64 commented 5 months ago

I agree that I think LLVMString should be Send (and maybe Sync?), but I think your analysis is slightly off in that it's more like CString, not CStr. &CStr (which is like &str) shouldn't be Send nor Sync when the reference is non static.

GCPanic commented 5 months ago

it's more like CString, not CStr.

Yes, you're right. LLVMString has string's ownership, it more like a CString.

&CStr (which is like &str) shouldn't be Send nor Sync when the reference is non static.

Not exactly, although thread::spawn does ask for values to be 'static, Send and Sync trait has nothing to do with lifetimes. And, infact, CStr does implements Send and Sync(which means &CStr also implements them)

TheDan64 commented 5 months ago

Ah yeah, good point