transpile: Various fixes to type handling for builtins - #1860
Conversation
6ac8aad to
7a907b6
Compare
6ec15cf to
d7fcb1c
Compare
d7fcb1c to
aaabe91
Compare
fw-immunant
left a comment
There was a problem hiding this comment.
LGTM overall, but would appreciate a comment where mentioned inline.
aaabe91 to
04f75f7
Compare
fw-immunant
left a comment
There was a problem hiding this comment.
NAK on the comment change - now the name of bubble_expr_types and its comment disagree on its primary purpose. It exists to homogeneize types between Clang versions by bubbling up types from child AST nodes to parent ones (and these types are not necessarily ones in PULLBACK_KINDS, e.g. in the case of bit shifts), and the other things it does are unrelated to this function. They should really be done separately, so we can remove the function when we no longer support Clang <16. Translating arbitrary C operations producing one type to Rust ones that producing a different type (based on the operation's identity rather than its children) seems best done in expr translation rather than by altering types here.
We don't necessarily need to change where this logic happens in this PR, but I meant to ask for a comment that documents the tech debt here rather than making it sound like the current confusing arrangement is by design.
Iterating through the whole AST is somewhat expensive, so I figure it's more efficient to fit it all together like this.
Perhaps, but the difficulty here is that the "bubbling" is done bottom-up while translation operates top-down. For example, if you have an expression like |
Squashed from immunant#1860 (5 commits). Threads expected_type_id/result_type_id through convert_builtin so its result is cast to the caller's expected type, fixing several builtins whose translated Rust return type didn't match the call site's expected type (e.g. __builtin_ffs/__builtin_clz/__builtin_ctz/__builtin_popcount returning u32 instead of the expected c_int, __builtin_alloca's pointer cast, __builtin_object_size's result type). Also refactors convert_builtin's internal match to return a value cast once at the end instead of wrapping every arm in its own Ok(...). Upstream: immunant#1860
04f75f7 to
5732ea8
Compare
5732ea8 to
cd535ed
Compare
|
I've now renamed |
5138d8a to
801bcd2
Compare
| @@ -0,0 +1,4 @@ | |||
| void test_builtin_object_size(void) { | |||
| int x = 0; | |||
| unsigned long n = __builtin_object_size(&x, 1); | |||
There was a problem hiding this comment.
Could use a few more tests for the other builtins.
There was a problem hiding this comment.
I don't really know how the builtins are used or what their use cases are, so I wouldn't know what would be a good test for them.
There was a problem hiding this comment.
See my other comment below for a single line change here.
For other builtins, here's something you could add that should confirm that these builtins return an int that's the same as int32_t:
#include <stdint.h>
void test_builtin_expected_type(double d, int y) {
int32_t ffs = __builtin_ffs(y);
int32_t clz = __builtin_clz((unsigned)y);
int32_t isnan = __builtin_isnan(d);
int32_t isinf_sign = __builtin_isinf_sign(d);
int32_t constant_p = __builtin_constant_p(y);
int32_t cond = y ? __builtin_ffs(y) : 0;
}
…ter reflect purpose
801bcd2 to
08d2a59
Compare
CTypeKind::Boolis always present)size_tfor__builtin_object_sizecast tounsigned long, causing Rust type mismatch #1707