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

Rework the package -- Take 1 #278

Merged
merged 52 commits into from
Apr 16, 2021
Merged

Rework the package -- Take 1 #278

merged 52 commits into from
Apr 16, 2021

Conversation

Gnimuc
Copy link
Member

@Gnimuc Gnimuc commented Jan 13, 2021

Well, I ended up reworking the whole package.

Features added by this PR:

In short, the goal is to make the wrapper generator easier to extend for new features.

As mentioned above, this PR is breaking. But I guess it's not that hard to upgrade the old scripts. I'm going to manually submit upgrading PRs to the downstream repos, which can also serve as a validation of the correctness of this PR.

TODO list:

Other bugfixes: fix #280, fix #279, fix #136, fix #126

@Gnimuc Gnimuc changed the title Refactor the Julia code emitting pass Rework the package Feb 23, 2021
@ihnorton
Copy link
Collaborator

ihnorton commented Mar 9, 2021

I'm trying this out, and it looks amazing ❤️

The only thing I've run in to so far is handling of {u}int{32,64}_t arguments -- they seem to be translated as bare uint32_t identifier, which doesn't exist (adding const uint32_t = UInt32 etc. fixed it). I'm happy to open an issue if you prefer.

edit: specifically, looks like that only happens for return types, function args work fine.

@Gnimuc
Copy link
Member Author

Gnimuc commented Mar 10, 2021

hi @ihnorton, could you share the C code that triggers the issue? I simplified the rule for translating function return types, but this change may be wrong for some corner cases.

@ihnorton
Copy link
Collaborator

ihnorton commented Mar 10, 2021

The signatures are here. The driver is exactly as you posted here but changed the names and lib path.

Looking closer, I see what is going on (overlooked the warning comment 🙃 ): these are all classified as CXType_FunctionNoProto. In the current master I believe this situation is handled here:

Clang.jl/src/type.jl

Lines 208 to 222 in 01bf3e8

"""
resolve_type(t::CLType) -> CLType
This function attempts to work around some limitations of the current libclang API.
"""
function resolve_type(t::CLType)::CLType
if kind(t) == CXType_Unexposed
# try to resolve Unexposed type to cursor definition.
cursor = typedecl(t)
if !isnull(cursor) && kind(cursor) != CXCursor_NoDeclFound
return type(cursor)
end
end
# otherwise, this will either be a builtin or unexposed client needs to sort out.
return t
end
(I had tried the same header a while back with an earlier master, and the return type was translated correctly as UInt64 etc.)

@Gnimuc
Copy link
Member Author

Gnimuc commented Mar 10, 2021

Thanks for the hint. Should now be fixed by df52021.

@ihnorton
Copy link
Collaborator

Works perfectly now, thanks!

@vrzh
Copy link

vrzh commented Apr 14, 2021

This rework effort is looking great!

Is it on purpose that in test/test.toml, there is no "prologue" stuff included? otherwise Ctime_t will not be defined in the output:

prologue_file_path = "../gen/prologue.jl"

Also, I have been unlucky that the C library I am trying to generate bindings for has a function which has a parameter named Ptr, which conflicts with the Julia Ptr. Generating the bindings succeeds, but when trying them I was getting:

ERROR: LoadError: could not evaluate ccall argument type (it might depend on a local variable)

I got around it by:

import Clang.Generators.RESERVED_WORDS
# ...
push!(RESERVED_WORDS, "Ptr")

Does this warrant enough of a use-case to expose a function/macro to add other words which may conflict? Similar to @add_def which helps to add extra definitions?

@Gnimuc
Copy link
Member Author

Gnimuc commented Apr 14, 2021

Is it on purpose that in test/test.toml, there is no "prologue" stuff included? otherwise Ctime_t will not be defined in the output:

I'm mainly running https://github.com/Gnimuc/GeneratorScripts for meaningful testing of the generated files, so here only the generator's public API is tested for now.

Does this warrant enough of a use-case to expose a function/macro to add other words which may conflict? Similar to @add_def which helps to add extra definitions?

@vrzh, thanks for the feedback! Without adding new features, a better fix would be writing a rewriter for that specific function like this one.

If you'd like to have this feature in Clang.jl, I'd suggest that we just add a new entry in the toml file like:

[codegen]
function_argument_conflict_symbols = ["Ptr"]  # should be empty by default

and prefix the argument names with "_" according to this list in the codegen.jl.

@Gnimuc
Copy link
Member Author

Gnimuc commented Apr 14, 2021

@vrzh, OK, I just implemented that in the latest commit, could you give it a try?

@vrzh
Copy link

vrzh commented Apr 14, 2021

@vrzh, OK, I just implemented that in the latest commit, could you give it a try?

Doesn't seem to work for me. I am new to Julia so most likely my fault. I tried the following in a clean Julia environment.

test.jl:

using Pkg
Pkg.add(PackageSpec(name="Clang", rev="4fb6690"))

using Clang.Generators
using Clang.LibClang.Clang_jll

headers = [ "test.h" ]
options = load_options(joinpath(@__DIR__, "test.toml"))
args = [ "-I." ]

# import Clang.Generators.RESERVED_WORDS
# push!(RESERVED_WORDS, "Ptr")

ctx = create_context(headers, args, options)

build!(ctx)

test.toml:

[general]
library_name = "libtest"

[codegen]
function_argument_conflict_symbols = ["Ptr"] 

test.h:

void f(int * x, int Ptr);

Output:

function f(x, Ptr)
    ccall((:f, libtest), Cvoid, (Ptr{Cint}, Cint), x, Ptr)
end

I agree with you that a rewriter is the best way to go (as you could also have C function names, C struct names, etc. which also clash), but an entry in the toml file as you suggest is the easiest for new users.

@Gnimuc
Copy link
Member Author

Gnimuc commented Apr 14, 2021

@vrzh Oops! Should be fixed now.

I agree with you that a rewriter is the best way to go (as you could also have C function names, C struct names, etc. which also clash), but an entry in the toml file as you suggest is the easiest for new users.

As C doesn't have any name mangling stuff, a common C library usually has its own manually mangled prefixes, so generally speaking, function/struct name collisions rarely happen in practice.

@vrzh
Copy link

vrzh commented Apr 14, 2021

@vrzh Oops! Should be fixed now.

Perfect! Works great, thanks a lot.

@Gnimuc Gnimuc changed the title Rework the package Rework the package -- Take 1 Apr 16, 2021
@Gnimuc
Copy link
Member Author

Gnimuc commented Apr 16, 2021

This PR is too long, so let's merge this to master.

@Gnimuc Gnimuc merged commit 27a7af7 into master Apr 16, 2021
@Gnimuc Gnimuc deleted the dag branch April 16, 2021 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment