Closed
Conversation
tannergooding
requested changes
Nov 11, 2021
Member
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
As per #178 (comment), there is no bug in the LLVMModuleFlagBehavior definition.
This enum matches exactly with the underlying enum defined in the LLVM-C source code.
The bug here is assuming that the C and C++ versions of the enum are identical and are 1-to-1 blittable to eachother.
Comment on lines
+36
to
+66
| [Test(Description = "Tests that LLVMModuleFlagBehavior is properly defined as a 0-based enum for use in AddModuleFlag")] | ||
| /// <summary>Verifies that <see cref="LLVMModuleFlagBehavior"/> is defined as expected by <see cref="AddModuleFlag"/> | ||
| /// so that there isn't unexpected behavior when modules with conflicting flags are linked. | ||
| /// This test is relevant because AddModuleFlag expects a 0-based behavior enum (where 1 defines warning), | ||
| /// while the other method of adding a module flag (AddNamedMetadataOperand) expects a 1-based behavior | ||
| /// (where 1 defines error).</summary> | ||
| public static void TestBehaviorOfLinkingConflictingModules() | ||
| { | ||
| // set up two modules | ||
| LLVMModuleRef module1 = LLVMModuleRef.CreateWithName("module1"); | ||
| LLVMModuleRef module2 = LLVMModuleRef.CreateWithName("module2"); | ||
|
|
||
| // Add conflicting module flags to module1 and module2 using LLVMModuleFlagBehavior.LLVMModuleFlagBehaviorWarning | ||
| // which should only cause a warning, and not an error | ||
| string flagName = "madeUpFlagName"; | ||
| uint value1 = 1; | ||
| uint value2 = 2; | ||
|
|
||
| module1.AddModuleFlag(flagName, LLVMModuleFlagBehavior.LLVMModuleFlagBehaviorWarning, value1); | ||
| module2.AddModuleFlag(flagName, LLVMModuleFlagBehavior.LLVMModuleFlagBehaviorWarning, value2); | ||
|
|
||
| // linking the modules should cause a warning, not an error | ||
| try | ||
| { | ||
| LLVM.LinkModules2(module1, module2); | ||
| } | ||
| catch (AccessViolationException) | ||
| { | ||
| // Fail doesn't actually get called. The test will simply be aborted if we reach this point, but the catch block cleans up the error output | ||
| Assert.Fail("Conflicting module flags that were defined with LLVMModuleFlagBehaviorWarning should not have caused an error."); | ||
| } |
Member
There was a problem hiding this comment.
This test needs to be put in an LLVMOpaqueModuleTests.Manual.cs file. The main file is autogenerated and so this test will get trashed every time bindings get regenerated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the bug where the LLVMModuleFlagBehavior enum values are all off by 1 (#178 (comment)). Also creates 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.