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

Drop support for generating "safe" code #69

Merged
merged 15 commits into from Jun 11, 2019

Conversation

Projects
None yet
2 participants
@tannergooding
Copy link
Member

commented Jun 11, 2019

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.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

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

@@ -82,10 +83,9 @@ protected Cursor(CXCursor handle, Cursor parent)

public TranslationUnit TranslationUnit { get; }

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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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

@@ -422,6 +463,54 @@ private string EscapeAndStripName(string name)
return EscapeName(name);
}

private string GetAccessSpecifierName(NamedDecl namedDecl)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

Also added support for matching the native access specifier.


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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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).


switch (name)
{
case "int8_t":

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

All of these "just work" now.

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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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).


namespace ClangSharp
{
public unsafe partial struct CXModuleMapDescriptor : IDisposable

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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


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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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).

}

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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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.

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)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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.

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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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

{
public IntPtr Results;
[NativeTypeName("CXCompletionResult *")]

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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.

{
[NativeTypeName("enum CXCursorKind")]

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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

public IntPtr data0; public IntPtr data1; public IntPtr data2;

[NativeTypeName("const void *[3]")]
public _data_e__FixedBuffer data;

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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.

public CXIdxObjCContainerDeclInfo* containerInfo;

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

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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").


namespace ClangSharp
{
[Conditional("DEBUG")]

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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


public IntPtr Pointer;

public static implicit operator CXCursorSet(CXCursorSetImpl* value)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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


public IntPtr Pointer;

public static explicit operator CXDiagnostic(void* value)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Jun 11, 2019

Author Member

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

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

10 checks passed

license/cla All CLA requirements met.
Details
microsoft.ClangSharp Build #20190611.2 succeeded
Details
microsoft.ClangSharp (macos_1014_debug_x64) macos_1014_debug_x64 succeeded
Details
microsoft.ClangSharp (macos_1014_release_x64) macos_1014_release_x64 succeeded
Details
microsoft.ClangSharp (ubuntu_1604_debug_x64) ubuntu_1604_debug_x64 succeeded
Details
microsoft.ClangSharp (ubuntu_1604_release_x64) ubuntu_1604_release_x64 succeeded
Details
microsoft.ClangSharp (windows_debug_x64) windows_debug_x64 succeeded
Details
microsoft.ClangSharp (windows_debug_x86) windows_debug_x86 succeeded
Details
microsoft.ClangSharp (windows_release_x64) windows_release_x64 succeeded
Details
microsoft.ClangSharp (windows_release_x86) windows_release_x86 succeeded
Details

@tannergooding tannergooding deleted the tannergooding:regenerate branch Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.