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

auto_mutability doesn't take structs into account #490

Open
VarLad opened this issue May 1, 2024 · 7 comments
Open

auto_mutability doesn't take structs into account #490

VarLad opened this issue May 1, 2024 · 7 comments
Labels
bug record struct/union/enum issues

Comments

@VarLad
Copy link

VarLad commented May 1, 2024

There are a few structs which take a pointer to another struct as one of its fields. For example, in a certain generated code:

mutable struct A
  x::Ptr{B}
  y::C
end
struct B
  x::Ptr{B}
  y::D
end

In such cases, wouldn't it make more sense if B was mutable, and for auto_mutability to make B mutable as well?

@Gnimuc
Copy link
Member

Gnimuc commented May 2, 2024

Could you share the C example code?

@VarLad
Copy link
Author

VarLad commented May 2, 2024

typedef struct WGPUChainedStruct {
    struct WGPUChainedStruct const * next;
    WGPUSType sType;
} WGPUChainedStruct WGPU_STRUCTURE_ATTRIBUTE;

typedef struct WGPUBufferDescriptor {
    WGPUChainedStruct const * nextInChain;
    WGPU_NULLABLE char const * label;
    WGPUBufferUsageFlags usage;
    uint64_t size;
    WGPUBool mappedAtCreation;
} WGPUBufferDescriptor WGPU_STRUCTURE_ATTRIBUTE;

from https://github.com/webgpu-native/webgpu-headers/blob/d02fec1b96af29695b9f5659a91067a241f40b04/webgpu.h#L746

@VarLad
Copy link
Author

VarLad commented May 3, 2024

@Gnimuc I believe we've to edit mutability.jl for StructForwardDecl, TypedefFunction and UnionForwardDecl? Does the current code allow this? (I saw it was just checking the nodes, so, can it work if we just edit https://github.com/JuliaInterop/Clang.jl/blob/master/src/generator/mutability.jl#L38 to include the above types to be checked?)

@Gnimuc
Copy link
Member

Gnimuc commented May 3, 2024

"""
TweakMutability <: AbstractPass
In this pass, the mutability of those structs which are not necessary to be immutable
will be reset to `true` according to the following rules:
if this type is not used as a field type in any other types
if this type is in the includelist
then reset
if this type is in the ignore list
then skip
if this type is used as the argument type in some function protos
if all of the argument type are non-pointer-type
then reset
elseif the argument type is pointer-type
if all other argument types in this function are not integer type (to pass a vector to the C function, both pointer and size are needed)
then reset
if this type is not used as the argument type in any functions (but there is an implicit usage)
then reset
"""

This is the current rule which is used to workaround the following case:

Clang.jl/gen/generator.toml

Lines 118 to 125 in 5a1cc29

# if you feel like certain structs should not be generated as mutable struct, please add them in the following list.
# for example, if a C function accepts a `Vector` of some type as its argument like:
# void foo(mutable_type *list, int n);
# when calling this function via `ccall`, passing a `Vector{mutable_type}(undef, n)` to the first
# argument will trigger a crash, the reason is mutable structs are not stored inline within a `Vector`,
# one should use `Ref{NTuple{n,mutable_type}}()` instead.
# this is not convenient and that's where the `auto_mutability_ignorelist` comes in.
auto_mutability_ignorelist = []

You can add more reasonable rules to should_tweak or directly in the pass function.

@Gnimuc
Copy link
Member

Gnimuc commented May 3, 2024

struct B
x::Ptr{B}
y::D
end

wait. does it only happen to StructMutualRef?

@Gnimuc
Copy link
Member

Gnimuc commented May 3, 2024

t isa StructAnonymous || t isa StructDefinition || t isa StructMutualRef || continue

What happens if you remove StructMutualRef in this line?

This actually do check those StructMutualRef types..

@Gnimuc Gnimuc added the bug label May 3, 2024
@Gnimuc
Copy link
Member

Gnimuc commented May 3, 2024

I think we need to add new rules for non-function-proto types.

@Gnimuc Gnimuc added the record struct/union/enum issues label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug record struct/union/enum issues
Projects
None yet
Development

No branches or pull requests

2 participants