Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

codegen/llvm: fix wrong field offset in packed structs #16657

Closed
wants to merge 2 commits into from

Conversation

xxxbxxx
Copy link
Contributor

@xxxbxxx xxxbxxx commented Aug 2, 2023

when a field was 'byte_aligned', the offset was applied once to the pointer
and again when dereferencing it (because it's a ptr with a 'packed_offset')

resolves #16615
and maybe #16621 ?

@xxxbxxx
Copy link
Contributor Author

xxxbxxx commented Aug 2, 2023

TODO:

  • [ x] the x86_64 back end also seems to adds the byte offset for the .lo field , but I can't figure out how the code works and where the offset is added...
  • [ x] the change regresses the test "@intFromPtr on a packed struct field".

I'm confused, I'm not sure how it's supposed to work.
the behaviour from packed_offset pointers in llvm (and generally in the zig compiler) seem to be that the pointer of a field points to the address of the struct, and the offset of the field is encoded in the ptr type. which is not what the test assumes. and what is the ptr of a field inside a packed struct when the field is non byte aligned?

Maybe the offset should be applied when casting, for example from *align(1:16:u123)u8 to *u8 ?

@xxxbxxx xxxbxxx force-pushed the packed-struct branch 3 times, most recently from c1bc6fc to e444e49 Compare August 9, 2023 08:13
@xxxbxxx
Copy link
Contributor Author

xxxbxxx commented Aug 9, 2023

https://ziglang.org/documentation/0.11.0/#packed-struct explains that a pointer to byte aligned field is always promoted to a simple "classic" pointer.
it is unclear what happens when the field is not full bytes. But the current behaviour is that

        var a:u4 = 0;
        var b:packed struct { x:u4} = .{.x=0};
        std.debug.print("{} {}\n", .{@TypeOf(&a), @TypeOf(&b.x)});

a  ->   *u4
b ->   *align(1:0:1) u4

so I tried to reflect that in the code for pointer/offsets computations.

note:
Writing this I realize now, the fix could (should?) have been the other way around (promote to *u4).
It is easy to try, but it doesn't work:

the llvmir generated seem correct:

  %14 = load i4, ptr inttoptr (i64 add (i64 ptrtoint (ptr @repro.test_0.S2.s to i64), i64 1) to ptr), align 1, !dbg !3066
  %15 = icmp ne i4 %14, 2, !dbg !3066
  br i1 %15, label %Then2, label %Else3, !dbg !3066

but the machine code that llvm generates from this doesn't apply a 0x0F mask, and the test fails:

     mov    al, BYTE PTR [rip+0x92ef3]
     sub    al, 0x2
     jne    0x2245b0

@xxxbxxx xxxbxxx marked this pull request as ready for review August 9, 2023 08:25
@xxxbxxx
Copy link
Contributor Author

xxxbxxx commented Aug 18, 2023

Writing this I realize now, the fix could (should?) have been the other way around (promote to *u4).

Out of curiosity (I don't have an actual use-case affecting me with that bug...) I investigated a bit further.
using #16605 gets the loads working.
Then I added similar load+mask+set+write code to store(), and new issues appeared:

  • the code for compares would also need fixing (*)
  • what about atomic access? load+...+write doesn't look very atomic to me...
  • what about volatile? not sure the additional unwanted load is expected.

(* currently, when using non-byte sized int types, it works because the adjacent extra bits are stomped with zeros https://godbolt.org/z/rq1dWh65s - which should not be done in a packed struct)

But then. at least the two last points seem problematic even outside of packed struct. maybe volatile/atomic access shouldn't be allowed with partial int types?

and I don't known where are the rules about the padding bits? I guess there are at least two possibilities:

  • if we want to give transparent access to packed non-byte-sized ints as if they were not packed, the rules must be that partial ints never modify the padding bits.
  • or we prefer keeping easy and fast access by saying the padding bits are undefined or zeroed - but then we can never promote non-byte-sized packed ints from *align(1:0:1) u4 to *u4.

Fields smaller that the abisize, even when aligned to a byte boundary, are accessed via a packed_offset decorated ptr.
This was not always considered by the code, and the offset was applied once to the pointer (as if it would be used as an undecorated ptr),
and again when dereferencing it (by the packed_offset decoration)

resolves ziglang#16615
and probaly ziglang#16621
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Packed structs: failure in assigning to struct fields
1 participant