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

Be explicit about arrays #2

Merged
merged 2 commits into from
May 17, 2018

Conversation

qmfrederik
Copy link
Contributor

I had apparently random access violation errors when running code that uses Core.Clang in Release mode.
I was able to reproduce the same behavior by running the Playground project on the netcoreapp2.0 runtime:

  • The program worked in Debug mode
  • An AccessViolationException occurred in Release mode.

It turns out that the culprit was the declaration of fixed size arrays. Whereas this worked:

internal struct CXCursor
{
    public CXCursorKind kind;
    public int xdata;
    public ulong data_0;
    public ulong data_1;
    public ulong data_2;
}

the code would consistently fail when CXCursor was declared like this:

internal struct CXCursor
{
    public CXCursorKind kind;
    public int xdata;
    public fixed ulong data[3];
}

So, this PR updates the code generator to always make "unroll" arrays as member fields.

I also updated the generator code to generate types and methods for the Documentation functionality in Clang.

@GordenOu
Copy link
Owner

Hi thanks, I got some questions:

  1. Why use the "CopyClang" method instead of running the "Native.LibClang.Restore" project? The embedded zip file also contains some header files which will be looked up by Clang.

  2. Would you mind keeping only basic support for documentation parsing like "GetText"? Since those like "HTMLStartTag" do not really belong to C/C++.

@qmfrederik
Copy link
Contributor Author

qmfrederik commented May 17, 2018

  1. Thanks, I removed the CopyClang method for now.
  2. Well, I added clang/Documentation.h to the list of headers being parsed so all methods in there are being generated automatically. It seems you can somehow include HTML comments in C source code? The methods are being generated in an internal class, so users of the library won't see them. Would you prefer to explicitly filter out those method?

PS: For deploying the native libraries, have you considered putting the clang.dll file in a runtimes\win7-x64 folder of the NuGet package? With the runtimes folder, .NET Core will automatically locate clang.dll (even from the NuGet package folder) without the need to manually copy it to the application folder. https://natemcmaster.com/blog/2016/05/19/nuget3-rid-graph/ has some additional documentation.

@GordenOu
Copy link
Owner

Importing the native methods is fine, I will try exposing some of them...

I saw the /runtimes folder in packages like Libuv as well. However, the size of the Clang binary is large, so it may cause trouble if more platforms are to be added. I noticed there are packages like this: https://www.nuget.org/packages/runtime.win7-x64.runtime.native.System.IO.Compression/ but not sure how it works. Anyway, I will do some experiments on it.

@GordenOu GordenOu merged commit 38ef66d into GordenOu:master May 17, 2018
@qmfrederik
Copy link
Contributor Author

To include native files such as libclang.dll in a NuGet package, you need something like this:

<Content Include="libclang.dll">
  <PackagePath>runtimes/win7-x64/native/libclang.dll</PackagePath>
  <Pack>true</Pack>
</Content>

Depending on how large the library is, and how many platforms you want to target, you can put the native files for all platforms in a single NuGet package or you can create one NuGet package for each platform.

See
https://github.com/libimobiledevice-win32/imobiledevice-net/blob/master/iMobileDevice-net/iMobileDevice-net.csproj#L46 for an example.

Hope it helps and thanks for all the great work with Core.Clang!

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 this pull request may close these issues.

2 participants