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

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

Closed
KristofferC opened this issue Dec 14, 2021 · 10 comments · Fixed by #361
Closed

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

KristofferC opened this issue Dec 14, 2021 · 10 comments · Fixed by #361

Comments

@KristofferC
Copy link
Contributor

I am hitting a leaf_ty with JuliaUnknown{Clang.CLInvalid}(CLType (Clang.CLInvalid) ) here

leaf_ty = get_jl_leaf_type(jlty)
which means that it error immediately at
if (hasref && haskey(dag.tags, leaf_ty.sym)) ||
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
Copy link
Member

Gnimuc commented Dec 14, 2021

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

@KristofferC
Copy link
Contributor Author

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

@Gnimuc
Copy link
Member

Gnimuc commented Dec 15, 2021

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

@emmt
Copy link

emmt commented Nov 4, 2022

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
Copy link
Member

Gnimuc commented Nov 4, 2022

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
Copy link
Member

Gnimuc commented Nov 4, 2022

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
Copy link
Member

Gnimuc commented Nov 4, 2022

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
Copy link

emmt commented Nov 5, 2022

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
Copy link
Member

Gnimuc commented Nov 5, 2022

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
Copy link

emmt commented Nov 5, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants