Open tf2spi opened 2 days ago
laytan
commented
2 days ago Just a thought, but this may be because this condition needs to be changed to also apply when the struct has max_field_align: https://github.com/odin-lang/Odin/blob/d118f88cd5e69f9b16de24021c9c0e71f7e9a9bb/src/llvm_backend_utility.cpp#L1203
tf2spi
commented
1 day ago No luck with this dummy diff, unfortunately :(
diff --git a/src/llvm_backend_utility.cpp b/src/llvm_backend_utility.cpp
index a2a0ba4cc..bb72ed7f1 100644
--- a/src/llvm_backend_utility.cpp
+++ b/src/llvm_backend_utility.cpp
@@ -1200,7 +1200,7 @@ gb_internal lbValue lb_emit_struct_ep(lbProcedure *p, lbValue s, i32 index) {
lbValue gep = lb_emit_struct_ep_internal(p, s, index, result_type);
Type *bt = base_type(t);
- if (bt->kind == Type_Struct && bt->Struct.is_packed) {
+ if (bt->kind == Type_Struct && (bt->Struct.is_packed || bt->Struct.custom_max_field_align)) {
lb_set_metadata_custom_u64(p->module, gep.value, ODIN_METADATA_IS_PACKED, 1);
GB_ASSERT(lb_get_metadata_custom_u64(p->module, gep.value, ODIN_METADATA_IS_PACKED) == 1);
}I see it's used to emit unaligned reads but it seems to be a problem that the gep is out-of-sync and doesn't generate the right offsets right now. What I'm guessing will help is if I make a dummy C program that packs the struct or specifies a certain alignment and see what LLVM IR it generates and how we can generate the same IR.
tf2spi
commented
1 day ago This is an example C program demonstrating the behavior we desire.
#include <stdio.h>
struct __attribute__((packed, aligned(4))) XYZ { int x; void *y; int z; char w; };
int main(void) {
struct XYZ xyz = {1, (void *)2, 3};
xyz.y = (void *)4;
printf("Hello %d %p %d %zu %zu\n", xyz.x, xyz.y, xyz.z, sizeof(xyz), _Alignof(struct XYZ));
return 0;
}The declaration of the struct is like this in IR
%struct.XYZ = type <{ i32, ptr, i32, i8, [3 x i8] }>
Normally, it is like this for an unpacked struct
%struct.XYZ = type < i32, ptr, i32, i8 >
And then the IR proceeds to specify the alignment for each store and load similar to what we do right now (which is a tad bit annoying, but it is what it is)
; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32 @main() #0 {
%1 = alloca i32, align 4
%2 = alloca %struct.XYZ, align 4
store i32 0, ptr %1, align 4
call void @llvm.memcpy.p0.p0.i64(ptr align 4 %2, ptr align 4 @__const.main.xyz, i64 20, i1 false)
%3 = getelementptr inbounds %struct.XYZ, ptr %2, i32 0, i32 1
store ptr inttoptr (i64 4 to ptr), ptr %3, align 4
// etc., etc., etc.So it's along the same lines of setting the struct to be packed. Changing the metadata to ODIN_METADATA_MAX_ALIGN from ODIN_METADATA_IS_PACKED would also be helpful in further controlling the alignment in OdinLLVMBuildLoadAligned and friends.
tf2spi
commented
1 day ago Yes, it seems that this diff at least makes the test case succeed again. More work needs to be done to make sure that padding is correct for fields and that loading/storing fields from/to the struct is aligned the way we want, which should (hopefully) be reasonable after seeing how clang does it.
diff --git a/src/llvm_backend_general.cpp b/src/llvm_backend_general.cpp
index 9dc603993..b3cb84d85 100644
--- a/src/llvm_backend_general.cpp
+++ b/src/llvm_backend_general.cpp
@@ -2155,7 +2155,7 @@ gb_internal LLVMTypeRef lb_type_internal(lbModule *m, Type *type) {
GB_ASSERT(fields[i] != nullptr);
}
- LLVMTypeRef struct_type = LLVMStructTypeInContext(ctx, fields.data, cast(unsigned)fields.count, type->Struct.is_packed);
+ LLVMTypeRef struct_type = LLVMStructTypeInContext(ctx, fields.data, cast(unsigned)fields.count, type->Struct.is_packed || type->Struct.custom_max_field_align);
map_set(&m->struct_field_remapping, cast(void *)struct_type, field_remapping);
map_set(&m->struct_field_remapping, cast(void *)type, field_remapping);
#if 0
Context
When working on #4474, changing
#packedto#max_field_align(4)causes incorrect stores to fields to be emitted and for those stores to be read incorrectly byfmt.printfin turn. Because one of the fields inmdeiwas a pointer when%vwas used, a crash happened shortly after the incorrect read.odin reportoutput:Expected Behavior
Expect no crash and for this to be the output
Current Behavior
Odin code is crashing when using
fmt.printfon themdeiIf
mdeiis replaced with0, it fails in a manner similar to #4407Failure Information (for bugs)
Please help provide information about the failure if this is a bug. If it is not a bug, please remove the rest of this template.
Steps to Reproduce
git pullonmasterto ensure you have up-to-date sources-MINIDUMP_EXCEPTION_INFORMATION :: struct { +MINIDUMP_EXCEPTION_INFORMATION :: struct #max_field_align(4) { ThreadId: DWORD, ExceptionPointers: ^EXCEPTION_POINTERS, ClientPointers: BOOL,
entering program ... crash detected, pep: &EXCEPTION_POINTERS{ExceptionRecord = 0xF6C516F7F0, ContextRecord = 0xF6C516F300} opened dumpfile, handle: 0x110 writing dumpfile process_handle: 0xFFFFFFFFFFFFFFFF process_id: 312 mdei: 0 ... MiniDumpWriteDump() result: false MiniDumpWriteDump() error code: 2147943398 error message: Invalid access to memory location.
closing dump file ...