Precise macro value type #3323
Precise macro value type #3323
Conversation
dingxiangfei2009
force-pushed
the
precise-macro-value-type
branch
from
4d459ec to
a10ba8a
Compare
|
Apparently the clang fallback has race condition. If I set thread number to 1, we don't have truncated output anymore. |
|
I have a theory about the test setup leading to the racing. The test setup uses the fallback translation unit backed by a file, which is located in Update: yes, the two tests are racing on writing into the precompiled header. |
dingxiangfei2009
force-pushed
the
precise-macro-value-type
branch
4 times, most recently
from
e03d7e7 to
4d04b3a
Compare
Upon activating the `--macro-const-use-ctypes` flag, `bindgen` should generate bindings with precise type resolution to the exact C types. This flag implies `--clang-macro-fallback` as its pre-requisite. Signed-off-by: Xiangfei Ding <[email protected]>
dingxiangfei2009
force-pushed
the
precise-macro-value-type
branch
from
4d04b3a to
2de8281
Compare
|
I am holding off |
|
r? @emilio 🙏 |
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Looks good, some (optional) nits.
| let mut header_names_to_compile = Vec::new(); | ||
| let mut header_paths = Vec::new(); | ||
| let mut header_includes = Vec::new(); | ||
| let single_header = self.options().input_headers.last().cloned()?; |
There was a problem hiding this comment.
This change seems drive-by, right? Seems fine tho.
| /// Use C platform-specific types for generated macro constants. | ||
| /// | ||
| /// This implies `clang_macro_fallback` and takes precedence over | ||
| /// `default_macro_constant_type`. |
There was a problem hiding this comment.
Would be good to have this be a little less weird, but for now it seems fine.
| // Safety: we are only copying the content, not assuming a borrow. | ||
| // TODO(@dingxiangfei2009): LLVM Libclang does not return the true size | ||
| // of a string literal, which could be truncated due to a null character | ||
| // '\0' in the middle. |
There was a problem hiding this comment.
Yeah, hopefully this is rare, but maybe file an upstream issue and reference it here?
|
|
||
| let source = fs::File::open(header)?; | ||
| let reader = BufReader::new(source); | ||
| // Safety: assume that headers in our test suite have distinct names. |
There was a problem hiding this comment.
There's no unsafe here tho, right?
This seems fine but might be worth a comment like "this is to prevent tests running in parallel from clobbering each other's results".
2 participants
Fix #923.
Fix #3200. This is due to a fallout from how
libclangtype would be processed with this patch.This patch introduce a new generator flag
--macro-const-use-ctypes, which resolves the macro definition expression to a precise type advised by Clang if activated.A macro expansion result adapter is introduced to maintain compatibility with
cexpr, which has a limited capacity to resolve to a precise C type.There is a change to the testing code because up to this point our tests, which can be executed concurrently, use the same path to the precomputed headers. Since this patch introduces a second test that relies on
libclangmacro fallback, the precompiled headers from the two different tests would overwrite each other. The new addition is that the backing files are now temporarily stored at different directories named after the header files under tests.