Clang.jl icon indicating copy to clipboard operation
Clang.jl copied to clipboard

Generators: Add padding between non consecutive fields

Open Pangoraw opened this issue 3 years ago • 7 comments

Hello :wave:

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!

Pangoraw avatar Jan 09 '22 17:01 Pangoraw

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 avatar Jan 10 '22 04:01 Gnimuc

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 https://github.com/JuliaInterop/Clang.jl/issues/317#issuecomment-884837470 and a repo for monitoring the sanity of the generated code.

Gnimuc avatar Jan 10 '22 04:01 Gnimuc

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.

Pangoraw avatar Jan 10 '22 19:01 Pangoraw

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 avatar Jan 11 '22 05:01 Gnimuc

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 avatar Jan 11 '22 05:01 Gnimuc

https://github.com/JuliaInterop/Clang.jl/pull/368#issuecomment-1009602873 has been implemented in #370 and merged to the master.

Gnimuc avatar Jan 22 '22 02:01 Gnimuc

#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 avatar Jan 22 '22 14:01 Pangoraw