Fix cycle removal bugs

[JamesWrigley]

The i index in the outermost loop over the nodes would become shadowed by the i index in the inner loop over the cycle indices, so the check to exit early would be done against the wrong variable.

I don't have an example of this failing, I was just reading the code and figured that it was probably a bug. Unfortunately I'm not familiar enough with the code to write a test for it.

[Gnimuc]

Thanks for fixing this. Did it solve #440 as well?

[JamesWrigley]

No unfortunately, I'm still looking into that. My current suspicion is that there's something going wrong with the cycle detection 🤔 Just for my own information, am I correct in thinking that StructMutualRef and the RemoveCircularReference pass is only for removing cycles like these?

typedef struct Bar;

struct Foo {
    Bar* bar;
};

struct Bar {
    Foo foo; // Could also be a pointer
}

[Gnimuc]

Yes. I'm now trying to reduce the header to an MWE.

[Gnimuc]

MWE:

typedef struct A A;
typedef struct B B;
typedef struct C C;

struct B;
struct C;

struct B
{
    B* x;
};

struct C
{
    B y[10];
};

struct A
{
    C* z;
};

[Gnimuc]

With this PR:

julia> using Clang.Generators
[ Info: Precompiling Clang [40e3b903-d033-50b4-a0cc-940c62c95e31]

julia> args = get_default_args()
6-element Vector{String}:
 "-isystem/Users/gnimuc/.julia/ar" ⋯ 61 bytes ⋯ "64-apple-darwin14/4.8.5/include"
 "-isystem/Users/gnimuc/.julia/ar" ⋯ 67 bytes ⋯ "le-darwin14/4.8.5/include-fixed"
 "-isystem/Users/gnimuc/.julia/ar" ⋯ 47 bytes ⋯ "9/x86_64-apple-darwin14/include"
 "-isystem/Users/gnimuc/.julia/ar" ⋯ 60 bytes ⋯ "e-darwin14/sys-root/usr/include"
 "-isystem/Users/gnimuc/.julia/ar" ⋯ 74 bytes ⋯ "-root/System/Library/Frameworks"
 "--target=x86_64-apple-darwin14"

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

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 A
    z::Ptr{Cvoid} # z::Ptr{C}
end

function Base.getproperty(x::A, f::Symbol)
    f === :z && return Ptr{C}(getfield(x, f))
    return getfield(x, f)
end

struct B
    x::Ptr{B}
end

struct C
    y::NTuple{10, B}
end

[ Info: Done!
Context(...)

[JamesWrigley]

The simplified MWE works, but it's not equivalent to your original MWE which still fails for me. I think this is an equivalent MWE:

typedef struct A A;
typedef struct B B;
typedef struct C C;

struct B;
struct C;

struct B
{
    B* x;
};

struct C
{
    B y[10];
};

typedef struct vector_C
{
    C* c;
} vector_C;

typedef struct pool_C
{
    vector_C vc;
} pool_C;

struct A
{
    pool_C pc;
    C* c;
};

[JamesWrigley]

Ok, I stepped through detect_cycle!() and couldn't find any issues, so now I'm thinking that the cycle removal is too aggressive 🤔 Currently it's looping through each each parent/child in the whole cycle path and (after checking some conditions) converts each to a StructMutualRef. But I think it's only necessary to convert the leaf parent/child since that's the actual cycle?

That would explain the output:

[ Info: Building the DAG...
[ Info: [RemoveCircularReference]: removed ImPlotAxis's dependency ImPlotAxis
[ Info: [RemoveCircularReference]: removed ImPlotPlot's dependency ImPlotAxis
[ Info: [RemoveCircularReference]: removed ImVector_ImPlotPlot's dependency ImPlotPlot

It's considering ImPlotAxis a mutual reference with itself, but then it's continuing up the cycle and converting the parents ImPlotPlot and ImVector_ImPlotPlot too.

[JamesWrigley]

62c9917 fixes the issue for me 😅

[JamesWrigley]

Ok, I think this is ready for review. I improved the original error message in 78bcaf8 and added a test for the cycle detection in 761b9e7.

(BTW if you approve it I can squash the fixup commits before you merge)