Drop support for generating "safe" code by tannergooding · Pull Request #69 · dotnet/ClangSharp

tannergooding · 2019-06-11T14:41:11Z

As called out here, the current support for generating "safe" code is very error prone and isn't going to be easy to maintain long term.

Namely, almost all of the support is a "best" guess heuristic where the heuristics can vary wildly from header file to header file and we frequently get it wrong, especially when dealing with bool, out, ref, or arrays.

This removes that support and switches to only generating unsafe code. As part of that, it updates the logic of when the unsafe keyword is emitted to ensure it is only emitted when actually required. This also adds NativeTypeName attribute metadata to members where the native type and managed type do not line up (this could do with some tuning in the future, as it is currently overly conservative). The attribute is marked as [Conditional("DEBUG")] so it doesn't bloat the release binaries.

This then regenerates ClangSharp and fixes up the higher level bindings where required.

…lying native type.

tannergooding · 2019-06-11T14:42:00Z

Sorry for the "big" PR. LLVMSharp is going to be just as bad.

tannergooding · 2019-06-11T14:43:07Z

sources/ClangSharp.PInvokeGenerator/Cursors/Cursor.cs

        public TranslationUnit TranslationUnit { get; }

-        public CXToken[] Tokenize(Cursor cursor)
+        public Span<CXToken> Tokenize(Cursor cursor)


Took a dependency on System.Memory to avoid creating arrays and copying overhead where possible.

tannergooding · 2019-06-11T14:44:10Z

sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs

            return EscapeName(name);
        }

+        private string GetAccessSpecifierName(NamedDecl namedDecl)


Also added support for matching the native access specifier.

tannergooding · 2019-06-11T14:46:47Z

sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs

            if (type is ArrayType arrayType)
            {
-                name = GetTypeName(namedDecl, arrayType.ElementType);
+                name = GetTypeName(namedDecl, arrayType.ElementType, out var nativeElementTypeName);


The secondary variables here are only used for debugging purposes. In some future PR, they might be used to drop more NativeTypeNameAttributes when possible (but the logic for treating unsigned int * as equal to uint* or transforming const unsigned int * const * to be const uint * const * is a bit tricky).

tannergooding · 2019-06-11T14:47:08Z

sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs


                switch (name)
                {
-                    case "int8_t":


All of these "just work" now.

tannergooding · 2019-06-11T14:49:56Z

sources/ClangSharp/Extensions/CXCompletionString.cs

-        public CXString GetParent(ref CXCursorKind kind) => clang.getCompletionParent(this, ref kind);
+        public CXString GetParent(out CXCursorKind kind)
+        {
+            fixed (CXCursorKind* pKind = &kind)


The downside (or upside, depending on how you look at it) to having unsafe bindings is you get to explicitly see all the "hidden" overhead that the marshaler was adding (such as implicitly pinning a value).

tannergooding · 2019-06-11T14:52:15Z

sources/ClangSharp/Extensions/CXModuleMapDescriptor.cs

+
+namespace ClangSharp
+{
+    public unsafe partial struct CXModuleMapDescriptor : IDisposable


Had to add a couple more "safe" wrappers so the non-PInvokeGenerator unit tests would pass.

tannergooding · 2019-06-11T14:53:36Z

sources/ClangSharp/Extensions/CXModuleMapDescriptor.cs

+
+        public CXErrorCode SetFrameworkModuleName(string name)
+        {
+            using (var marshaledName = new MarshaledString(name))


This is how I opted to do the string marshaling (and cleanup) currently.

Ideally the "safe" API would take a UTF8String rather than string, but that type doesn't exist yet (and support for treating ReadOnlySpan<byte> as a utf8string is fairly limited in netstandard2.0).

tannergooding · 2019-06-11T14:55:59Z

sources/ClangSharp/Extensions/CXString.cs

+                }
+
+                var span = new ReadOnlySpan<byte>(pCString, int.MaxValue);
+                return span.Slice(0, span.IndexOf((byte)'\0')).AsString();


Marshaling from native to string just utilizes ROSpan<byte> and System.Text.Encoding.UTF8.GetBytes

tannergooding · 2019-06-11T14:58:43Z

sources/ClangSharp/Extensions/CXTranslationUnit.cs

+            using (var marshaledCommandLineArgs = new MarshaledStringArray(commandLineArgs))
+            fixed (CXUnsavedFile* pUnsavedFiles = unsavedFiles)
+            {
+                var pCommandLineArgs = stackalloc sbyte*[marshaledCommandLineArgs.Count];


Marshaling string arrays is currently a bit more complex. I had tried to allocate a sbyte** and then fill it directly (to avoid needing to allocate a sbyte*[]), but that was having problems and I decided to resolve it later.

tannergooding · 2019-06-11T14:59:30Z

sources/ClangSharp/Extensions/CXTranslationUnit.cs

-        public void Dispose() => clang.disposeTranslationUnit(this);
-
-        public void DisposeTokens(CXToken[] tokens) => clang.disposeTokens(this, tokens, (uint)tokens.Length);
+        public void AnnotateTokens(CXToken[] tokens, CXCursor[] cursors)


It would be nice if most of these were updated to take a ROSpan<T> or Span<T>, as appropriate. So you can avoid allocations where appropriate, but I left that to a future PR.

tannergooding · 2019-06-11T15:00:10Z

sources/ClangSharp/Generated.Custom.cs

@@ -1,325 +0,0 @@
-namespace ClangSharp


I was able to get rid of all method/type exclusions and just generate the bindings directly instead.

tannergooding · 2019-06-11T15:01:10Z

sources/ClangSharp/Generated/CXCodeCompleteResults.cs

+    public unsafe partial struct CXCodeCompleteResults
    {
-        public IntPtr Results;
+        [NativeTypeName("CXCompletionResult *")]


Here is an example of where, if we "normalized" the native type name when checking for "matches managed type name", we could drop the attribute.

tannergooding · 2019-06-11T15:02:11Z

sources/ClangSharp/Generated/CXCompletionResult.cs

-    public partial struct CXCompletionResult
+    public unsafe partial struct CXCompletionResult
    {
+        [NativeTypeName("enum CXCursorKind")]


Another example of where "normalizing" the native type name would allow us to drop the attribute.

tannergooding · 2019-06-11T15:03:20Z

sources/ClangSharp/Generated/CXCursor.cs

-        public IntPtr data0; public IntPtr data1; public IntPtr data2;
+
+        [NativeTypeName("const void *[3]")]
+        public _data_e__FixedBuffer data;


I find the attributes to, at a glance, make it much easier to determine the original signature. While still allowing you to use the least expensive managed signature. Here is a good example.

tannergooding · 2019-06-11T15:05:11Z

sources/ClangSharp/Generated/CXIdxObjCInterfaceDeclInfo.cs

+        public CXIdxObjCContainerDeclInfo* containerInfo;
+
+        [NativeTypeName("const CXIdxBaseClassInfo *")]
+        public CXIdxBaseClassInfo* superInfo;


It also allows you to see the "const" info, which can be very useful for exposing the right managed API (and knowing things like: "should this be span or rospan").

tannergooding · 2019-06-11T15:06:11Z

sources/ClangSharp/Internals/NativeTypeNameAttribute.cs

+
+namespace ClangSharp
+{
+    [Conditional("DEBUG")]


Conditional("DEBUG") will cause the attribute to be stripped from release binaries, avoiding bloat.

tannergooding · 2019-06-11T15:06:52Z

sources/ClangSharp/Wrappers/CXCursorSet.cs

+
+        public IntPtr Pointer;
+
+        public static implicit operator CXCursorSet(CXCursorSetImpl* value)


Using implicit operators to convert from the native type to the wrapper type when it is an "exact" match.

tannergooding · 2019-06-11T15:07:11Z

sources/ClangSharp/Wrappers/CXDiagnostic.cs

+
+        public IntPtr Pointer;
+
+        public static explicit operator CXDiagnostic(void* value)


Using explicit operators to convert from the native type to the wrapper type when it is an "ambiguous" match.

…ht places.

tannergooding added 14 commits June 9, 2019 14:06

Changing ClangSharp to no longer support generating "safe" code.

57d1300

Simplify the number of typedefs we need to special-case.

1d1e9bf

Adding support for matching the access specifier given in native code.

a485e95

Adding basic support for emitting an attribute which tracks the under…

82a7a4a

…lying native type.

Moving the wrapper types out of the generated folder.

ff337ac

Adding the NativeTypeNameAttribute file

45d91d1

Removing exclusions from the GenerateClangSharp command line.

d217b7f

Improving the computed native type name.

2442b3f

Ensure the unsafe keyword is used on delegates when required.

aa1a348

Regenerating ClangSharp and updating the extension bindings.

e4ee0b4

Changing artificial fixed sized buffers to have internal fields.

315d66d

Removing exclusions from the GenerateLLVMSharp command line.

273c230

Fixing the System.Memory package version to be centrally managed.

266c0cf

Simplfying getting the native type name.

277d9b1

tannergooding commented Jun 11, 2019

View reviewed changes

Fixing up the tests to expect the NativeTypeName attribute in the rig…

c222d32

…ht places.

tannergooding mentioned this pull request Jun 11, 2019

Regenerate LLVMSharp using unsafe bindings. dotnet/LLVMSharp#105

Merged

mjsabby approved these changes Jun 11, 2019

View reviewed changes

tannergooding merged commit 01e36c8 into dotnet:master Jun 11, 2019

tannergooding deleted the regenerate branch June 12, 2019 02:34


		public IntPtr Pointer;

		public static implicit operator CXCursorSet(CXCursorSetImpl* value)


		public IntPtr Pointer;

		public static explicit operator CXDiagnostic(void* value)

Conversation

tannergooding commented Jun 11, 2019

Uh oh!

tannergooding commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tannergooding Jun 11, 2019 •

edited

Loading