`#max_field_align(4)` on `MINIDUMP_EXCEPTION_INFORMATION` misbehaves #4491

tf2spi · 2024-11-17T01:41:01Z

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

Use git pull on master to ensure you have up-to-date sources
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,

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

The text was updated successfully, but these errors were encountered:

laytan · 2024-11-17T21:26:25Z

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 · 2024-11-18T04:02:48Z

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 · 2024-11-18T04:29:24Z

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 · 2024-11-18T04:53:23Z

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

tf2spi changed the title ~~#max_align(4) on MINIDUMP_EXCEPTION_INFORMATION~~ #max_field_align(4) on MINIDUMP_EXCEPTION_INFORMATION misbehaves Nov 17, 2024

tf2spi linked a pull request Dec 22, 2024 that will close this issue

Add packing + aligned access w/ field_align #4611

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`#max_field_align(4)` on `MINIDUMP_EXCEPTION_INFORMATION` misbehaves #4491

`#max_field_align(4)` on `MINIDUMP_EXCEPTION_INFORMATION` misbehaves #4491

tf2spi commented Nov 17, 2024

laytan commented Nov 17, 2024

tf2spi commented Nov 18, 2024

tf2spi commented Nov 18, 2024

tf2spi commented Nov 18, 2024

#max_field_align(4) on MINIDUMP_EXCEPTION_INFORMATION misbehaves #4491

#max_field_align(4) on MINIDUMP_EXCEPTION_INFORMATION misbehaves #4491

Comments

tf2spi commented Nov 17, 2024

Context

Expected Behavior

Current Behavior

Failure Information (for bugs)

Steps to Reproduce

Failure Logs

laytan commented Nov 17, 2024

tf2spi commented Nov 18, 2024

tf2spi commented Nov 18, 2024

tf2spi commented Nov 18, 2024

`#max_field_align(4)` on `MINIDUMP_EXCEPTION_INFORMATION` misbehaves #4491

`#max_field_align(4)` on `MINIDUMP_EXCEPTION_INFORMATION` misbehaves #4491