Translate c pointer alignment #2318
Conversation
|
Just saw this, I don't mind at all - this way I get to compare our solutions =) |
|
Your implementation addresses the first two questions I had posted on my PR:
If the alignment is not known and/or the returned alignment is 0, should the alignCasts be skipped? This is the case for void *. I think alignCast requires a power of 2 alignment. Regarding dividing the bit alignment by 8 - is it guaranteed to be a multiple of 8 or could it consider an alignment of 17 bits to not be smaller than one of 18 bits because the division truncates the remainder? I'll make a comment on the commit |
src/translate_c.cpp
Outdated
| const clang::PointerType *ptr_ty = reinterpret_cast<const clang::PointerType*>(ty); | ||
| ZigClangQualType base_qt = bitcast(ptr_ty->getPointeeType()); | ||
| // Convert from bits to bytes | ||
| return ZigClangASTContext_getTypeAlign(c->ctx, base_qt) / 8; |
There was a problem hiding this comment.
is the type alignment guaranteed to be multiple of 8? Could integer div interfere with ptr alignment comparison?
test/translate_c.zig
Outdated
| \\ var p: ?*c_void = undefined; | ||
| \\ { | ||
| \\ var to_char: [*c]u8 = @ptrCast([*c]u8, p); | ||
| \\ var to_short: [*c]c_short = @ptrCast([*c]c_short, @alignCast(2, p)); |
There was a problem hiding this comment.
maybe this is easier said than done but this would be better translated as @alignCast(@alignOf(c_short), p) rather than hard coding 2.
Common wisdom is that it is indeed safe and that's also what llvm does in
I've left the hard-coded number since it also serves as a sanity check to catch types with mismatched alignment. No idea about the test failure, as always it's osx that's failing. |
I think it's actually problematic, because, for example, the alignment of
Don't worry about that one - it's one of the things I'm addressing with the coroutine rewriting. |
LemonBoy
force-pushed
the
translate-c-pointer-alignment
branch
2 times, most recently
from
15027ae to
e4fda63
Compare
test/translate_c.zig
Outdated
| @@ -1334,7 +1334,7 @@ pub fn addCases(cases: *tests.TranslateCContext) void { | |||
| \\} | |||
| , | |||
| \\fn ptrcast(a: [*c]c_int) [*c]f32 { | |||
| \\ return @ptrCast([*c]f32, a); | |||
| \\ return @ptrCast([*c]f32, @alignCast(@alignOf([*c]f32), a)); | |||
There was a problem hiding this comment.
This is not quite right, it should be @alignOf(f32).
There was a problem hiding this comment.
Duh, I must've missed some type unwrapping in the last rewrite. Sorry for the noise, the patch should be ready to go now.
Avoid producing Zig code that doesn't compile due to mismatched alignments between pointers. Always emit a @Alignof instead of hardcoding the alignment value returned by LLVM for portability sake of the generated code.
LemonBoy
force-pushed
the
translate-c-pointer-alignment
branch
from
e4fda63 to
7c41146
Compare
I hope @raulgrell doesn't mind but I ended up needing this for some translate-c magic to work so I went ahead and wrote this PR.
Closes #2313