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

Generators: Add padding between non consecutive fields #368

Closed
wants to merge 1 commit into from

Conversation

Pangoraw
Copy link
Contributor

@Pangoraw Pangoraw commented Jan 9, 2022

Hello 👋

I had an issue with Clang.jl where the created Julia struct would not match the C representation of the same struct because of field alignments issues.

Consider the following struct:

struct B {
  uint64_t ba;
  uint64_t bb;
};

union union_B {
  struct B b;
};

struct A {
  char a;
  union union_B b;
};

/**/
sizeof(struct A) // 24
sizeof(struct B) // 16
sizeof(struct A) != sizeof(char) + sizeof(struct B) // true

The generated Julia struct is:

struct B
  ba::UInt64
  bb::UInt64
end

struct union_B
    data::NTuple{16, UInt8}
end

struct A
  a::Cchar
  b::union_B
end

#==#
sizeof(A) # 17
sizeof(union_B) # 16
sizeof(A) != sizeof(Cchar) + sizeof(union_B) # false

The C and Julia version of field b::B are not aligned (C is +8 whereas Julia is only +1). This PR tries to address this problem by adding new fields on the Julia side and adding a custom constructor to the struct:

struct B
  ba::UInt64
  bb::UInt64
end

struct union_B
    data::NTuple{16, UInt8}
end

struct A
    a::Cchar
    var"##pad0#274"::NTuple{7, UInt8}
    b::union_B
    A(a, b) = new(a, Tuple((zero(UInt8) for _ = 1:7)), b)
end

#==#
sizeof(A) # 24
sizeof(union_B) # 16
sizeof(A) != sizeof(Cchar) + sizeof(union_B) # true

I have noticed that there are only a few tests for Clang.Generators so I would be interested on having other codebase to test this code on. For example when generating the Julia for clang-c into LibClang some struct are added padding fields. Also, should I add test for this ? It would eval the generated julia code and check the different sizeof.

Anyway, thanks for making this project!

@Gnimuc
Copy link
Member

Gnimuc commented Jan 10, 2022

Could you share some system info? I can't reproduce the wrong padding result on the Julia side.

julia> using Clang.Generators

julia> args = get_default_args("x86_64-linux-gnu")
5-element Vector{String}:
 "-isystem/Users/gnimuc/.julia/ar"  56 bytes  "/x86_64-linux-gnu/4.8.5/include"
 "-isystem/Users/gnimuc/.julia/ar"  62 bytes  "4-linux-gnu/4.8.5/include-fixed"
 "-isystem/Users/gnimuc/.julia/ar"  42 bytes  "e3d045/x86_64-linux-gnu/include"
 "-isystem/Users/gnimuc/.julia/ar"  55 bytes  "-linux-gnu/sys-root/usr/include"
 "--target=x86_64-unknown-linux-gnu"

shell> cat test.h
#include <stdint.h>

struct A {
  char a;
  struct B {
    uint64_t ba;
    uint64_t bb;
  } b;
};

julia> ctx = create_context("test.h", args);
[ Info: Parsing headers...

julia> build!(ctx)
[ Info: Processing header: test.h
[ Info: Building the DAG...
┌ Warning: default libname: ":libxxx" is being used, did you forget to set `library_name` in the toml file?
└ @ Clang.Generators ~/.julia/dev/Clang/src/generator/audit.jl:16
[ Info: Emit Julia expressions...
struct B
    ba::UInt64
    bb::UInt64
end

struct A
    a::Cchar
    b::B
end

[ Info: Done!
Context(...)

julia> struct B
           ba::UInt64
           bb::UInt64
       end

julia> struct A
           a::Cchar
           b::B
       end

julia> sizeof(A)
24

julia> sizeof(B)
16

@Gnimuc
Copy link
Member

Gnimuc commented Jan 10, 2022

I have noticed that there are only a few tests for Clang.Generators so I would be interested on having other codebase to test this code on.

It would be great if you could contribute a set of test cases. There were some simple tests as mentioned in #317 (comment) and a repo for monitoring the sanity of the generated code.

@Pangoraw
Copy link
Contributor Author

oh I simplified my example a bit too much. It only happens when the re-aligned field is an union because in Julia they are represented as NTuple{X, UInt8} which does not require a different alignment. I updated the example as such.

@Gnimuc Gnimuc added bug record struct/union/enum issues labels Jan 11, 2022
Comment on lines +485 to +502

current_offset = 0
pads = []
for (i, field_cursor) in enumerate(field_cursors)
field_sym = make_symbol_safe(name(field_cursor))
field_ty = getCursorType(field_cursor)
field_offset = getOffsetOfField(field_cursor) ÷ 8
field_size = getSizeOf(field_ty)
@assert field_offset >= current_offset

if field_offset != current_offset
pad_size = field_offset - current_offset
pad_name = Symbol("pad", length(pads)) |> gensym
push!(block.args, Expr(:(::), pad_name, :(NTuple{$pad_size, UInt8})))
push!(pads, (;narg = i + length(pads), size = pad_size))
end

current_offset += field_size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix only handles generic cases, the same fix should also apply for StructMutualRef.

@Gnimuc
Copy link
Member

Gnimuc commented Jan 11, 2022

An alternative fix would be marking these kinda structs as StructLayout in the preprocessing pass like what we do for nested_anonymous_types. In this way, we redirect its codegen pass to here, so don't need to change anything in the CodeGen pass.

@Gnimuc
Copy link
Member

Gnimuc commented Jan 11, 2022

struct A
    a::Cchar
    var"##pad0#274"::NTuple{7, UInt8}
    b::union_B
    A(a, b) = new(a, Tuple((zero(UInt8) for _ = 1:7)), b)
end

I think this solution is also good. We can add an option in the .toml file to let users choose what they'd like to generate.

You could implement this by adding a new type like StructLayout and writing a new codegen pass for it.

@Gnimuc Gnimuc added enhancement and removed bug record struct/union/enum issues labels Jan 22, 2022
@Gnimuc
Copy link
Member

Gnimuc commented Jan 22, 2022

#368 (comment) has been implemented in #370 and merged to the master.

@Pangoraw
Copy link
Contributor Author

#368 (comment) has been implemented in #370 and merged to the master.

Awesome! Sorry i didn't had time to update on this. I don't know whether or not it's worth adding the different codegen with paddings now that it uses a StructLayout. What do you think ?

@Pangoraw Pangoraw closed this Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants