JuliaInterop / Clang.jl

C binding generator and Julia interface to libclang
https://juliainterop.github.io/Clang.jl/
MIT License
225 stars 68 forks source link

ERROR: LoadError: type JuliaUnknown has no field sym #354

Closed KristofferC closed 2 years ago

KristofferC commented 2 years ago

I am hitting a leaf_ty with JuliaUnknown{Clang.CLInvalid}(CLType (Clang.CLInvalid) ) here https://github.com/JuliaInterop/Clang.jl/blob/2c9230dc28a7d6d3bb33025b8c99a302f7ce5170/src/generator/system_deps.jl#L17 which means that it error immediately at https://github.com/JuliaInterop/Clang.jl/blob/2c9230dc28a7d6d3bb33025b8c99a302f7ce5170/src/generator/system_deps.jl#L23 because a JuliaUnknown does not have a sym field:

ERROR: LoadError: type JuliaUnknown has no field sym
Stacktrace:
 [1] getproperty
   @ ./Base.jl:42 [inlined]
 [2] collect_dependent_system_nodes!(dag::ExprDAG, node::ExprNode{Clang.Generators.FunctionProto, Clang.CLFunctionDecl}, system_nodes::Dict{ExprNode, Int64})
   @ Clang.Generators ~/gen/dev/Clang/src/generator/system_deps.jl:24
 [3] (::CollectDependentSystemNode)(dag::ExprDAG, options::Dict{String, Any})
   @ Clang.Generators ~/gen/dev/Clang/src/generator/passes.jl:69
 [4] build!(ctx::Context, stage::Clang.Generators.BuildStage)
   @ Clang.Generators ~/gen/dev/Clang/src/generator/context.jl:170
 [5] build!
   @ ~/gen/dev/Clang/src/generator/context.jl:160 [inlined]

I guess getting that object there is not ideal but the code should probably explicitly handle it somehow.

Gnimuc commented 2 years ago

Would be great if you could share those headers. Are there any errors when parsing the code?

KristofferC commented 2 years ago

These are the headers https://github.com/dankamongmen/notcurses/tree/master/include/notcurses. The error is the same as in #355 though.

Gnimuc commented 2 years ago

I noticed there are many inline functions in those headers, so you may also hit #267.

emmt commented 1 year ago

I apologize if I should not have re-open this issue but I had the very same error which was not solved by #361, it was due to some atomic member in a structure. Removing the _Atomic specification worked but the corresponding member in Julia equivalent structure has lost it atomicity. I replaced types like _Atomic long by atomic_long and get a Julia Cint as a result whatever the kind of integer e.g. atomic_llong, atomic_fast_int64_t etc. all yield Cint where I would expect Threads.Atomic{Cllong}, Threads.Atomic{Int64}, etc.

I managed to get what I wanted by hacking the C header files with a macro that rename atomic variables with an __Atomic__ suffix and remove the atomic specification when some macro was defined, say -DJULIA_HACKS, and then filter the Julia code produced by Clang to replace expressions like x__Atomic__::T by x::Threads.Atomic{T} whatever x and T.

Is there a better solution?

I forgot to mention that I am using Clang v0.16.6.

Gnimuc commented 1 year ago

Could you share the generator script and the output info?

get a Julia Cint as a result whatever the kind of integer e.g. atomic_llong, atomic_fast_int64_t etc. all yield Cint where I would expect Threads.Atomic{Cllong}, Threads.Atomic{Int64}, etc.

I suspect that your header is not successfully parsed. For any type that Clang fails to recognize, it will return a Cint as a fallback value.

_Atomic is currently not supported by Clang.jl because I can't find any info in the Julia docs on how to map it on the Julia side.

If Threads.Atomic is the right choice, then I'm happy to add support in Clang.jl.

Gnimuc commented 1 year ago

A quick check shows that mapping _Atomic to Threads.Atomic might be wrong because they don't have the same size.

Wait. If Threads.Atomic is compatible with _Atomic/std::atomic<T>, then it's OK.

julia> Threads.Atomic{Int} |> dump
Base.Threads.Atomic{Int64} <: Any
  value::Int64

julia> struct foo 
         x::Threads.Atomic{Cint}
       end

julia> struct bar
         x::Cint
       end

julia> Core.sizeof(foo)
8

julia> Core.sizeof(bar)
4
Gnimuc commented 1 year ago

According to

https://github.com/JuliaLang/julia/blob/88fcf44c1e52cf0e0bd32747b0cb2b77fb9c0f3f/src/julia_atomics.h#L36

https://github.com/JuliaLang/julia/blob/7a21d52d4847a3ebed944888eae98b8c69472861/base/atomics.jl#L75-L79

I don't think these two types are compatible with each other.

Well. After looking into the implementation of std::atomic<>, they do seem to be compatible with each other...

  /// Base class for atomic integrals.
  //
  // For each of the integral types, define atomic_[integral type] struct
  //
  // atomic_bool     bool
  // atomic_char     char
  // atomic_schar    signed char
  // atomic_uchar    unsigned char
  // atomic_short    short
  // atomic_ushort   unsigned short
  // atomic_int      int
  // atomic_uint     unsigned int
  // atomic_long     long
  // atomic_ulong    unsigned long
  // atomic_llong    long long
  // atomic_ullong   unsigned long long
  // atomic_char16_t char16_t
  // atomic_char32_t char32_t
  // atomic_wchar_t  wchar_t
  //
  // NB: Assuming _ITp is an integral scalar type that is 1, 2, 4, or
  // 8 bytes, since that is what GCC built-in functions for atomic
  // memory access expect.
  template<typename _ITp>
    struct __atomic_base
    {
    private:
      typedef _ITp  __int_type;

      __int_type    _M_i;

    public:
      __atomic_base() noexcept = default;
      ~__atomic_base() noexcept = default;
      __atomic_base(const __atomic_base&) = delete;
      __atomic_base& operator=(const __atomic_base&) = delete;
      __atomic_base& operator=(const __atomic_base&) volatile = delete;

      // Requires __int_type convertible to _M_i.
      constexpr __atomic_base(__int_type __i) noexcept : _M_i (__i) { }
emmt commented 1 year ago

Thank you for your responsiveness.

In case you want to have a look, the build sript is here, the C headers are from this project and there is a dependency here. When building Julia code with Clang, I define -DJULIA_HACKS so that atomic members declared with the tao_atomic macro become non-atomic but their name changed to have suffix __Atomic__, then at the end of build.jl I rewrite the produced code with a few substitution rules to convert the type of these variables.

Gnimuc commented 1 year ago

This workaround looks good to me.

I noticed there are only 3 structs that are using tao_atomic in the project. If you don't want to use JULIA_HACKS to add poison to your headers, then it's also a good choice to manually wrap these 3 structs and put them in an epilogue patch file.

emmt commented 1 year ago

Manually wrapping the structures was something I had done in the past (before I knew about Clang). Having the structures automatically (and correctly) defined is really handy when the C code evolves (which occurred quite often for this project). So hacking a bit the C headers to be able to automatically generate correct Julia bindings seems a light price to pay. I admit that this hack is hardly conceivable for all projects, but TAO has been designed from the beginning to be used with other languages than C, especially with Julia.