Fix race condition in clang_macro_fallback#3369
Conversation
| use crate::ir::context::BindgenContext; | ||
| use clang_sys::*; | ||
| use std::cmp; | ||
| use std::os::unix::ffi::OsStrExt; |
There was a problem hiding this comment.
This probably needs to be #[cfg]d to build on windows right?
hkalbasi
force-pushed
the
fix-race-condition
branch
from
d1bc925 to
8cd1b34
Compare
| } | ||
|
|
||
| fn path_to_cstring(path: &Path) -> CString { | ||
| CString::new(path.as_os_str().as_bytes()).unwrap() |
There was a problem hiding this comment.
Hmm actually, I think you do need to_string_lossy().as_bytes() or so, as_os_str().as_bytes() would have a bunch of nulls on windows right?
There was a problem hiding this comment.
It is now to_string_lossy.
hkalbasi
force-pushed
the
fix-race-condition
branch
from
8cd1b34 to
8f6fb47
Compare
|
@emilio anything else to change on this PR? |
| pub(crate) struct FallbackTranslationUnit { | ||
| file_path: String, | ||
| pch_path: String, | ||
| temp_dir: TempDir, |
There was a problem hiding this comment.
I think this is minor but the fields of a struct are dropped in declaration order. So I think temp_dir should be last field so it's dropped after disposing translation unit by clang.
There was a problem hiding this comment.
I guess TranslationUnit does not depend on the directory, but I made the change anyway. Better safe than sorry.
hkalbasi
force-pushed
the
fix-race-condition
branch
from
8f6fb47 to
fc93834
Compare
There was a problem hiding this comment.
@ojeda can you confirm this is fine on the kernel side? Or do we need to keep the clang_macro_fallback_build_dir option?
If so this patch needs to become a bit more complicated I suppose.
Looks good to me other than that.
| } | ||
|
|
||
| fn path_to_cstring(path: &Path) -> CString { | ||
| CString::new(path.to_string_lossy().as_bytes()).unwrap() |
There was a problem hiding this comment.
I guess this still doesn't deal correctly with non-utf-8 paths. On unix we could just plumb the bytes, on Windows... well, sucks to suck I guess.
There was a problem hiding this comment.
Is it now good? Is there any non-unix and non-windows os supported by bindgen? That would get the to_string_lossy version.
hkalbasi
force-pushed
the
fix-race-condition
branch
from
fc93834 to
8126d07
Compare
Thanks for the ping Emilio -- we didn't get to use Does this patch create files alongside the source file or just in that Having said that, it is usually a good idea to be able to control where temporary files go, and other compilers (including I hope that helps. |
The version before this patch did this, but this patch ideally fix that too.
You can control it using |
I think that may be fine for most projects -- could we document it then? (At the end of the day the real cost is usually supporting whatever way is decided, not so much the particular mechanism) |
4 participants
Resurrection of #3111
Github does not allow reopening force pushed PRs.