search

dotnet / LLVMSharp

LLVM bindings for .NET Standard written in C# using ClangSharp
MIT License
838 stars 95 forks source link

LLVMModuleFlagBehavior enum values are all off by 1 #178

Closed ryan-moreno closed 2 years ago

ryan-moreno commented 2 years ago

enums in C# start by assigning 0 by default, but the enum within LLVM native is defined as starting at 1. In the documentation for native LLVM, 1 maps to error, 2 maps to warning, etc. Also, in the native LLVM source code, the exposed enum LLVMModuleFlagBehavior is defined using the internal enum ModFlagBehavior which is defined using 1 as error, 2 as warning, etc. So, all of the values in LLVMSharp's LLVMModuleFlagBehavior enum are off by 1. This isn't caught by existing tests because the tests don't actually test the behavior of linking modules with conflictin flags. They just check that the behavior passed into LLVM native is the same as the one returned (and the off by 1 translation happens in both directions, so they do in fact match).

PR #179 fixes the error and also adds a unit test to make sure that the enums aren't off by 1 by checking that LLVM.ModuleFlagBehaviorWarning doesn't cause an Error (which does happen in the existing version of LLVMSharp). There is a comment at the bottom of the unit test describing a way to improve the test if someone else knows how to implement this.

tannergooding commented 2 years ago

You are comparing the C++ enum vs the C# bindings which are over the C version of the enum: https://github.com/llvm/llvm-project/blob/llvmorg-12.0.1/llvm/include/llvm-c/Core.h#L398-L447

See also https://llvm.org/doxygen/group__LLVMCCoreTypes.html#gaa61633f872a8b3cbcbab5f5a96d3ce0f vs https://llvm.org/doxygen/classllvm_1_1Module.html#a0a5c55e12c97b80021330fe82b642293

If there is any error here, it is in the LLVM-C header files and libLLVM source code; not in LLVMSharp.

ryan-moreno commented 2 years ago

You're right, thank you for pointing this out.

There are two different ways to add a module flag in LLVM Native: A) using LLVMAddModuleFlag (which expects a 0-based LLVMModuleFlagBehavior) and B) using LLVMAddNamedMetadataOperand (which expects a 1-based behavior represented by a uint). LLVMSharp does expose LLVMModuleFlagBehavior but doesn't expose LLVMAddModuleFlag (which is the only place the 0-based LLVMModuleFlagBehavior would be used), which is rather confusing [this led me to believe LLVMAddNamedMetadataOperand should be accepting the 0-based LLVMModuleFlagBehavior]. I'm going to close this issue and the associated PR and instead make a PR that exposes LLVMAddModuleFlag to ease this confusion. Let me know if you have any thoughts on this.

tannergooding commented 2 years ago

instead make a PR that exposes LLVMAddModuleFlag to ease this confusion

That sounds reasonable. I'm assuming, however, you mean LLVMModuleRef.AddModuleFlag like the PR has now.

Notably, it does expose LLVM.AddModuleFlag already, it just wasn't visible as a method on the helper LLVMModuleRef struct.

ryan-moreno commented 2 years ago

@tannergooding yes, I meant exposing the functionality of LLVMAddModuleFlag within LLVMModuleRef. Here's the PR I created. Thanks for your feedback!