Rewrite the ClangSharpPInvokeGenerator to allow better reuse and to be more explicit #34

tannergooding · 2019-04-25T17:45:32Z

I imagine there will be some feedback and/or questions that need to be addressed, so feel free 😄

This rewrites the ClangSharpPInvokeGenerator code. For the most part, the rewrite tries to produce code that is nearly identical to the previous codegen.

An overview is:

The visitation logic is centralized in CursorVisitor and extension points BeginHandle and EndHandle are exposed, to make it easy for someone to handle or skip particular nodes and to execute their own code.
The file generation logic is centralized in CursorWriter. It takes a Func<CXCursor, bool> predicate that allows a user to indicate if they want to process or skip a given node. The visitation logic should account for nested types and for nodes that always need to be traversed
Both the traversal and generation logic try to be explicit as possible. This means there are a lot of switch statements and the fallback behavior is generally to log a message and break the debugger. This was done to make it very easy to know what has and hasn't been considered. It also makes it easier to break out individual logic or paths where needed.

The notable changes are:

The visitation logic now considers Location.IsInMainFile, this ultimately means the order is slightly different and that we don't visit #includes. This changes the output order slightly but also means we don't need to keep track of already visited nodes.
We return uint for uint rather than int
We no longer prefix @ to everything, and instead only prefix it to reserved C# keywords
We better follow typedefs and resolve the correct type to output. For example, we will now write out public CXCompletionString CompletionString rather than public IntPtr @CompletionString. The latter (IntPtr) will still be done if there is a double indirection.
We no longer explicitly declarate an enum as : int, since that is the default
We output IntPtr rather than ulong for size_t, to make the code produced more portable

Potential Future changes:

The writer should ideally be extended to allow writing a type per file, this makes it easier to parse and compare deltas
The writer should ideally be extended to allow writing "unsafe" code, rather than trying to create wrapper types or use managed types that require marshaling
- Wrapper types don't always work, they sometimes break functionality based on the underlying calling convention or ABI
- Modern .NET has a lot of helper types, like Span<T>, that make it much easier to work with unsafe types
- Purely unsafe/blittable code has less overhead, since there is no marshaling required
Allow more user customization of the output. This would allow options like the above (single vs multi-file generation and safe vs unsafe generation) and others like AnyCPU vs Platform Specific generation or Windows vs Unix generation (for handling the long is IntPtr vs int problem)

ClangSharp/ClangSharp.csproj

tannergooding · 2019-04-25T17:51:36Z

ClangSharpPInvokeGenerator/CXCursorExtensions.cs

+
+            switch (name)
+            {
+                case "size_t":


Hopefully a better can be solved to handle this and other similar types... Potentially could check for both regular and -m32 mode and if it differs chooise IntPtr, that way it is more general....

tannergooding · 2019-04-25T17:53:01Z

ClangSharpPInvokeGenerator/CXTypeExtensions.cs

+
+namespace ClangSharpPInvokeGenerator
+{
+    internal static class CXTypeExtensions


If the writer is extended to support user-configuration, these might need to either take additional parameters or become instance methods on the writer... The later might be better since they are more specific to the writer than to the actual interop types.

tannergooding · 2019-04-25T17:54:49Z

ClangSharpPInvokeGenerator/CXTypeExtensions.cs

+
+                case CXTypeKind.CXType_Char_S:
+                {
+                    return "MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(StringMarshaler))";


This could be LPUTF8Str if we said net48, netstandard2.1, and netcoreapp10 are the baseline

tannergooding · 2019-04-25T17:55:18Z

ClangSharpPInvokeGenerator/CXTypeExtensions.cs

+
+                        case CXCursorKind.CXCursor_ParmDecl:
+                        {
+                            if (cursor.Type.GetParmModifier(cursor).Equals("out"))


There is probably a better way to handle this combination...

tannergooding · 2019-04-25T17:56:09Z

ClangSharpPInvokeGenerator/CXTypeExtensions.cs

+            }
+        }
+
+        public static string GetParmModifierForPointeeType(this CXType type, CXCursor cursor, CXType pointeeType)


Could also support in and ref for "safe" code. Part of the problem, however, is that if attributes don't exist we are just guessing and will often get it wrong....

tannergooding · 2019-04-25T17:57:25Z

ClangSharpPInvokeGenerator/Program.cs

@@ -158,31 +158,38 @@ public static void Main(string[] args)
                sw.WriteLine("    using System.Runtime.InteropServices;");
                sw.WriteLine();

-                var structVisitor = new StructVisitor(sw);
+                var structDeclWriter = new CursorWriter(sw, indentation: 1, (cursor) => cursor.Kind == CXCursorKind.CXCursor_StructDecl);


Could potentially expose a method that takes the predicate, rather than the constructor, so you can reuse the instance across multiple filters...

tannergooding · 2019-04-25T17:58:04Z

ClangSharpPInvokeGenerator/Program.cs

+                    sw.WriteLine($"        private const string libraryPath = \"{libraryPath}\";");
+                    sw.WriteLine();
+
+                    functionDeclWriter.PrefixStrip = prefixStrip;


Ideally there would be a better way to pass this, and other options, in. Potentially a CursorWriterOptions type....

ClangSharpPInvokeGenerator/Properties/launchSettings.json

tannergooding · 2019-04-25T23:31:35Z

I'll get a fix up that resolves the couple issues I called out in an hour or two.

mjsabby · 2019-04-25T23:50:50Z

@tannergooding Sounds good. Now that it's SDK style, I'm thinking we can make it all nice like other dotnet repos with Azure DevOps and analyzers! I did want to ask you about if we were to make these signed nuget packages what secret sauce the dotnet repos have. We can have that conversation offline if need be.

… launchSettings to be machine independent

tannergooding · 2019-04-26T00:49:51Z

Dependency on S.R.CompilerServices.Unsafe was removed.

Fixed the launchSettings.json to include a Unix profile and to not be machine dependent. It still makes an assumption about the location of LLVM and assumes it is running from the current output directory (which is how VS runs it for F5).

Now that it's SDK style, I'm thinking we can make it all nice like other dotnet repos with Azure DevOps and analyzers!

That would be nice. It could potentially be use dotnet/arcade for the infrastructure (which would give it everything other repos like Roslyn or CoreFX have availalbe) or it could go with something smaller and more targeted... NuGet signing I'm not as sure about, but I know the right people to talk to.

mjsabby · 2019-04-26T00:56:56Z

The F5 is important so that's good. I'm wondering if we could do a download of LLVM-* and have it be there as part of the build. It's cached for most scenarios, but it'll allow us the latest and greatest.

I'm now eyeing how we can improve LLVMSharp with this rewrite. I've been bogged down with issues in the generator and now at least somethings seem better that it could be 100% generated without modifications.

I did have a question. It is my understanding that a single IntPtr in a struct for marshalling can't be ABI incompatobe. But you said it is, are there examples?

tannergooding · 2019-04-26T01:08:38Z

I'm wondering if we could do a download of LLVM-* and have it be there as part of the build. It's cached for most scenarios, but it'll allow us the latest and greatest.

It might be possible, but it's also a 350 MB tarball which might be more than some people expect (especially if they already have it locally). Probably worth some further thought to determine the right behavior...

It is my understanding that a single IntPtr in a struct for marshalling can't be ABI incompatobe. But you said it is, are there examples?

Struct wrappers are generally not a problem for most ABIs. However, __thiscall in particular can be problematic (there may be others as well, but I don't know of any specifics) since it defines different rules for returning a struct and returning a primitive. Full Framework and .NET Core prior to 3.0 had a bug that allowed this to "just work", but that has been fixed for netcoreapp3.0 and will now do the correct thing in P/Invoke scenarios (but preserves the old bug for the COM scenario).

tannergooding · 2019-04-26T01:09:37Z

For reference on the __thiscall scenario in particular, you can see dotnet/coreclr#23974 and related PRs/issues

tannergooding · 2019-04-26T01:15:59Z

I'm now eyeing how we can improve LLVMSharp with this rewrite. I've been bogged down with issues in the generator and now at least somethings seem better that it could be 100% generated without modifications.

That's Great!

I'm going to try out a few more libraries after this is merged and try to ensure more scenarios are covered as well as there are likely plenty of cursor types that still need to be handled.

mjsabby · 2019-04-26T05:36:19Z

Are you waiting for me to merge? Or is there is more to be done here?

tannergooding · 2019-04-26T11:21:16Z

Nothing more to be done here. Was waiting to ensure nothing else came up from the conversation above.

tannergooding added 4 commits April 24, 2019 16:47

Performing some cleanup of the OOP methods, primarily to fix debugging

3e709fd

Adding a launchSettings.json so that F5 regenerates ClangSharp

50c00d3

Rewriting ClangSharpPInvokeGenerator

8e78e3c

Regenerating ClangSharp and updating OOP extensions

e7d5d8b

tannergooding commented Apr 25, 2019

View reviewed changes

ClangSharp/ClangSharp.csproj Outdated Show resolved Hide resolved

tannergooding commented Apr 25, 2019

View reviewed changes

ClangSharpPInvokeGenerator/Properties/launchSettings.json Outdated Show resolved Hide resolved

mjsabby approved these changes Apr 25, 2019

View reviewed changes

Removing the dependency on S.R.CompilerServices.Unsafe and fixing the…

e501ed7

… launchSettings to be machine independent

tannergooding merged commit 4021b87 into dotnet:master Apr 26, 2019

tannergooding deleted the rewrite branch April 26, 2019 14:07

This was referenced Apr 26, 2019

Setup a CI system for ClangSharp #36

Closed

Adding some additional fixups allow LLVMSharp to be regenerated #37

Merged

Regenerate LLVMSharp from the 6.0.0 sources, based on the ClangSharp rewrite dotnet/LLVMSharp#93

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the ClangSharpPInvokeGenerator to allow better reuse and to be more explicit #34

Rewrite the ClangSharpPInvokeGenerator to allow better reuse and to be more explicit #34

tannergooding commented Apr 25, 2019

tannergooding Apr 25, 2019

tannergooding Apr 25, 2019

tannergooding Apr 25, 2019

tannergooding Apr 25, 2019

tannergooding Apr 25, 2019

tannergooding Apr 25, 2019 •

edited

Loading

tannergooding Apr 25, 2019

tannergooding commented Apr 25, 2019

mjsabby commented Apr 25, 2019

tannergooding commented Apr 26, 2019

mjsabby commented Apr 26, 2019

tannergooding commented Apr 26, 2019

tannergooding commented Apr 26, 2019

tannergooding commented Apr 26, 2019

mjsabby commented Apr 26, 2019

tannergooding commented Apr 26, 2019

Rewrite the ClangSharpPInvokeGenerator to allow better reuse and to be more explicit #34

Rewrite the ClangSharpPInvokeGenerator to allow better reuse and to be more explicit #34

Conversation

tannergooding commented Apr 25, 2019

tannergooding Apr 25, 2019

Choose a reason for hiding this comment

tannergooding Apr 25, 2019

Choose a reason for hiding this comment

tannergooding Apr 25, 2019

Choose a reason for hiding this comment

tannergooding Apr 25, 2019

Choose a reason for hiding this comment

tannergooding Apr 25, 2019

Choose a reason for hiding this comment

tannergooding Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

tannergooding Apr 25, 2019

Choose a reason for hiding this comment

tannergooding commented Apr 25, 2019

mjsabby commented Apr 25, 2019

tannergooding commented Apr 26, 2019

mjsabby commented Apr 26, 2019

tannergooding commented Apr 26, 2019

tannergooding commented Apr 26, 2019

tannergooding commented Apr 26, 2019

mjsabby commented Apr 26, 2019

tannergooding commented Apr 26, 2019

tannergooding Apr 25, 2019 •

edited

Loading