#max_field_align(4) on MINIDUMP_EXCEPTION_INFORMATION misbehaves

[tf2spi]

Context

When working on #4474, changing #packed to #max_field_align(4) causes incorrect stores to fields to be emitted and for those stores to be read incorrectly by fmt.printf in turn. Because one of the fields in mdei was a pointer when %v was used, a crash happened shortly after the incorrect read.

• Operating System & Odin Version:
• Please paste odin report output:

        Odin:    dev-2024-11:c4b273958
        OS:      Windows 11 Home Basic (version: 22H2), build 22621.4317
        CPU:     Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz
        RAM:     16286 MiB
        Backend: LLVM 18.1.8

Expected Behavior

Expect no crash and for this to be the output

entering program ...
crash detected, pep: &EXCEPTION_POINTERS{ExceptionRecord = 0xD5B3FFF870, ContextRecord = 0xD5B3FFF380}
opened dumpfile, handle: 0x110
writing dumpfile
        process_handle: 0xFFFFFFFFFFFFFFFF
        process_id: 23156
        mdei: 0 ...
MiniDumpWriteDump() result: true
closing dump file ...

Current Behavior

Odin code is crashing when using fmt.printf on the mdei

If mdei is replaced with 0, it fails in a manner similar to #4407

Failure 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

1. Use git pull on master to ensure you have up-to-date sources
2. Apply the following diff

diff --git a/core/sys/windows/dbghelp.odin b/core/sys/windows/dbghelp.odin
index 336992b4a..e32b4c874 100644
--- a/core/sys/windows/dbghelp.odin
+++ b/core/sys/windows/dbghelp.odin
@@ -15,7 +15,7 @@ MINIDUMP_DIRECTORY :: struct {
        Location:   MINIDUMP_LOCATION_DESCRIPTOR,
 }

-MINIDUMP_EXCEPTION_INFORMATION :: struct {
+MINIDUMP_EXCEPTION_INFORMATION :: struct #max_field_align(4) {
        ThreadId:          DWORD,
        ExceptionPointers: ^EXCEPTION_POINTERS,
        ClientPointers:    BOOL,

3. See Windows SetUnhandledExceptionFilter() and MiniDumpWriteDump() not working #4407 for the rest of the steps on reproducing the issue

Failure Logs

This is a picture of the program crashing when debugging in Visual Studio which contains the information about the access violation.

If mdei is replaced with 0, it does not crash and has similar output to #4407

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 ...

[laytan]

Just a thought, but this may be because this condition needs to be changed to also apply when the struct has max_field_align:

Odin/src/llvm_backend_utility.cpp

Line 1203 in d118f88

if (bt->kind == Type_Struct && bt->Struct.is_packed) {

[tf2spi]

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]

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]

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