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

Fix string marshaling crash. #91

Closed
wants to merge 1 commit into from

Conversation

TChatzigiannakis
Copy link
Contributor

@TChatzigiannakis TChatzigiannakis commented Feb 22, 2019

This PR fixes a string marshaling issue that causes a crash in #90 (among other cases).

The fix is achieved by:

  • Locating various extern methods that return a string (such as LLVM.GetBasicBlockName).
  • Adding an alternative version that returns an IntPtr (e.g. GetBasicBlockNameAsPtr).
  • Changing the original extern method to call the alternative version and pass the pointer to Marshal.PtrToStringAnsi.

I have moved the offending methods to Overloads.Interop.cs, to keep them separate from the generated ones that don't need this kind of attention.

Some classes in LLVMSharp.API were already performing this fix internally, in order to provide a safer debugging experience (otherwise viewing properties would crash the debugger). This PR simply brings that safety to the bindings as well.


[DllImport(libraryPath, EntryPoint = "LLVMGetBasicBlockName", CallingConvention = CallingConvention.Cdecl)]
public static extern IntPtr GetBasicBlockNameAsPtr(LLVMBasicBlockRef @BB);
public static string GetBasicBlockName(LLVMBasicBlockRef @BB) => Marshal.PtrToStringAnsi(GetBasicBlockNameAsPtr(@BB));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep having to refresh my memory but is there no deallocation requirement?

public static string GetDataLayout(LLVMModuleRef M) => Marshal.PtrToStringAnsi(GetDataLayoutAsPtr(M));

[DllImport(libraryPath, EntryPoint = "LLVMGetTarget", CallingConvention = CallingConvention.Cdecl)]
public static extern IntPtr GetTargetAsPtr(LLVMModuleRef M);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a new addition not previously there? I would keep this private then.

@ghost
Copy link

ghost commented Jun 23, 2019

Any updates on this?

@tannergooding
Copy link
Member

This was probably handled by the ClangSharp rewrite that happened (which in turn fixed up a number of issues in the bindings/etc)...

Could you retry to see if the issue still repros?

@ghost
Copy link

ghost commented Jun 24, 2019

Has the package been re-published? Because I depend on the LLVMSharp 5.0 NuGet package.

@tannergooding
Copy link
Member

No new nuget package has been published yet and the latest bits, which would contain the fix, are using LLVM 8

@ghost
Copy link

ghost commented Jun 25, 2019

No I'm limited to the Nuget package, so 5.0.. I would love to use 8.0 just the Nuget hasn't been released..

@tannergooding
Copy link
Member

A beta of the 8.0.0 packages was published on NuGet. It would be great if you could see if the issue still repros.

@jangofett4
Copy link

jangofett4 commented Aug 31, 2019

@tannergooding I'm unable to get 8.0.0 beta package for my project, nuget won't show anthing above 5.0.0.

Project is a .NET Core 2.1 library and nuget page says it depends on .NET Standard 2.0 (I'm assuming 'standard' and 'core' are same here) and libLLVM 8.0.0, which interestingly I can install.

Does it need exactly 2.0? If so why am I able to install libLLVM 8.0.0 despite it being a .NET Standard 2.0 package too?

Update: I re-downloaded .NET Core development package for Visual Studio 2019 and created a brand new .NET Core 2.2 console project. Nuget package manager still doesn't show LLVMSharp 8.0.0-beta package. Instead I used dotnet CLI to add package:
dotnet add ProjectName package LLVMSharp --version 8.0.0-beta

This one works (I got lost in new LLVM pointer stuff hehe) and adds LLVMSharp 8.0.0 to project but dependencies are not handled (i think) application immediately crashes since libLLVM dll is missing. I
installed that too via .NET CLI, but 8.0.0 package does NOT ship dll (16kb package). So I'm stuck stuck

Update: I know I hi-jack my comment too many times but...
I managed to run project using libLLVM 7.0 dll, and it works (downloaded from nuget and extracted to binary directory, which is not optimal but works). LLVMSharp 8.0 seems to fixed string mashalling. @cloudrex can you check?

@mjsabby
Copy link
Contributor

mjsabby commented Sep 14, 2019

@jangofett4 Did you allow nuget to get dependencies that are preview?

@jangofett4
Copy link

@mjsabby oops, I didn't know that was a thing until you mentioned it (sorry, I'm didn't use nuget that much before). Enabling Preview Packages made it possible for me to download 8.0.0-beta package. But that package depends to libLLVM 8.0.0, which nuget is able to install too. But somehow libLLVM 8.0.0 package does not contain required libLLVM dll to run. Is package incomplete?
broken-package

Package seems to be 16kb which is way to small for libLLVM dll (which was 37mb ish). I can use libLLVM 7.0.0 dll manually, and it would work, but that way clean builds remove dll and whats the point if I need to download dependencies myself. (Also this small code I wrote might work, but bigger projects may break).

Also I can't understand this new LLVM pointer stuff. Everything seems to be returning a pointer now. Are we supposed to use unsafe code or is there a way. Whats the point of using C# if I will use unmanaged code anyway 😉

@tannergooding
Copy link
Member

tannergooding commented Sep 15, 2019

The libLLVM package is a metapackage that will pull down the appropriate real package based on the machine you are building on and/or the runtime identifier you are targeting (otherwise its a 300+ mb nuget package to put all 11 native libLLVM dlls together😄).

For LLVM in particular, the higher level wrapper hasn't been created yet. Basically, the ClangSharp binding generator was updated to generate 1-to-1 bindings with the underlying native code. This ensures that there is a zero overhead option and that it is actually compatible with the underlying API (without needing to guess as to what the input is "meant" to be, etc).

There is then meant to be a higher level (but still minimal) "safe" wrapper over those bindings that takes strings and other "normal" managed types, rather than unsafe code. This makes it easier to use for most users and ensures that the appropriate explicit marshaling occurs, error handling happens, etc.

This was already done for ClangSharp itself (there are three total layers; the raw bindings, a minimal "safe" wrapper, and then a object oriented wrapper that mirrors the Clang C++ API), but something similar still needs to be done for LLVM.

@jangofett4
Copy link

jangofett4 commented Sep 15, 2019

@tannergooding

The libLLVM package is a metapackage that will pull down the appropriate real package based on the machine you are building on and/or the runtime identifier you are targeting

Well thats another new feature of nuget I learned today, thanks! But still, nuget won't pull necessary libraries from server. I tried using different platforms (incase if AnyCPU is failing) but nor x86 or x64 works either. Is .NET Core projects not supported or am I doing something wrong here? Nuget version seems to be fine which is 4.7 (and this is a new installation of VS2019).

This was already done for ClangSharp itself (there are three total layers; the raw bindings, a minimal "safe" wrapper, and then a object oriented wrapper that mirrors the Clang C++ API), but something similar still needs to be done for LLVM.

Thanks for clarifying this 😄. Although some implicit type conversions seems to be working.
For example: LLVMBuilderRef builder = LLVM.CreateBuilder();
LLVM.CreateBuilder seems to be returning LLVMOpaqueBuilder* but C# automatically converts to LLVMBuilderRef somehow. Maybe conversions are partly-implemented?

@jangofett4
Copy link

Probably related to my previous comment: #115

@tannergooding
Copy link
Member

The issue is likely the same as dotnet/ClangSharp#118 (comment), in which case the simple fix for now is to add <RuntimeIdentifier Condition="'$(RuntimeIdentifier)' == '' AND '$(PackAsTool)' != 'true'">$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier> to your project (under a PropertyGroup). Unfortunately because of the way NuGet restore works, we can't just add this to a build/*.targets in the LLVMSharp nuget package.

@yowl
Copy link
Contributor

yowl commented May 30, 2020

FWIW, I took the code in #90
Changed to the new API:

        [Test]
        public unsafe void a()
        {
            LLVMModuleRef mod = LLVMModuleRef.CreateWithName("netscripten");

            LLVMTypeRef[] param_types = { };
            LLVMTypeRef func_type =  LLVMTypeRef.CreateFunction(LLVMTypeRef.Int32, param_types, false);
            LLVMValueRef main = mod.AddFunction("main", func_type);

            LLVMBasicBlockRef entry = main.AppendBasicBlock("entry");

            LLVMBuilderRef builder = LLVM.CreateBuilder();
            builder.PositionAtEnd(entry);
            builder.BuildRet(LLVM.ConstInt(LLVM.Int32Type(), 0ul, 0));

            if (!mod.TryVerify(LLVMVerifierFailureAction.LLVMPrintMessageAction, out var error))
                Console.WriteLine($"Error: {error}");
            byte* res = (byte *)LLVM.GetBasicBlockName(entry);
            Console.WriteLine("Entry block name: {0}", Encoding.ASCII.GetString(res, 5));

            builder.Dispose();
            Console.WriteLine("Program End");
        }

and it works and prints:

Entry block name: entry
Program End

So I would think this can be closed.

@mjsabby mjsabby closed this May 30, 2020
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.

5 participants