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.

Sign up for GitHub

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

Jump to bottom

Unmanaged constructed types tests #31602

Merged
merged 36 commits into from
Jan 11, 2019
Merged

Unmanaged constructed types tests #31602

merged 36 commits into from
Jan 11, 2019

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Dec 7, 2018 •
edited by jcouv
Loading

Related to #31374 and dotnet/csharplang#1744

Seeing an interesting test failure in the new verification test in
7b278f4. Anything come to mind on how to address this @jaredpar @agocke?

System.Exception : Verification failed
---- Roslyn.Test.Utilities.Desktop.RuntimePeVerifyException : 
PeVerify failed for assembly 'C:\Users\rigibson\AppData\Local\Temp\RoslynTests':
[ : MyStruct`1[T]::get_Item][mdToken=0x6000002][offset 0x0000000F][found unmanaged pointer] Expected numeric type on the stack.
[ : C::Main][mdToken=0x6000003][offset 0x00000012][found address of Single] Expected numeric type on the stack.


Stack Trace:
  C:\Users\rigibson\src\roslyn\src\Test\Utilities\Portable\Platform\Desktop\DesktopRuntimeEnvironment.cs(320,0): at Roslyn.Test.Utilities.Desktop.DesktopRuntimeEnvironment.Verify(Verification verification)
  C:\Users\rigibson\src\roslyn\src\Test\Utilities\Portable\CompilationVerifier.cs(160,0): at Microsoft.CodeAnalysis.Test.Utilities.CompilationVerifier.Emit(String expectedOutput, Nullable`1 expectedReturnCode, String[] args, IEnumerable`1 manifestResources, EmitOptions emitOptions, Verification peVerify, SignatureDescription[] expectedSignatures)
  C:\Users\rigibson\src\roslyn\src\Test\Utilities\Portable\CommonTestBase.cs(154,0): at Microsoft.CodeAnalysis.Test.Utilities.CommonTestBase.Emit(Compilation compilation, IEnumerable`1 dependencies, IEnumerable`1 manifestResources, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, Action`1 assemblyValidator, Action`1 symbolValidator, EmitOptions emitOptions, Verification verify)
  C:\Users\rigibson\src\roslyn\src\Test\Utilities\Portable\CommonTestBase.cs(68,0): at Microsoft.CodeAnalysis.Test.Utilities.CommonTestBase.CompileAndVerifyCommon(Compilation compilation, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 assemblyValidator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, EmitOptions emitOptions, Verification verify)
  C:\Users\rigibson\src\roslyn\src\Compilers\Test\Utilities\CSharp\CSharpTestBase.cs(605,0): at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CompileAndVerify(Compilation compilation, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 validator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, EmitOptions emitOptions, Verification verify)
  C:\Users\rigibson\src\roslyn\src\Compilers\Test\Utilities\CSharp\CSharpTestBase.cs(564,0): at Microsoft.CodeAnalysis.CSharp.Test.Utilities.CSharpTestBase.CompileAndVerify(CSharpTestSource source, IEnumerable`1 references, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 assemblyValidator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Nullable`1 expectedReturnCode, String[] args, CSharpCompilationOptions options, CSharpParseOptions parseOptions, EmitOptions emitOptions, TargetFramework targetFramework, Verification verify)
  C:\Users\rigibson\src\roslyn\src\Compilers\CSharp\Test\Semantic\Semantics\GenericConstraintsTests.cs(3659,0): at Microsoft.CodeAnalysis.CSharp.Semantic.UnitTests.Semantics.GenericConstraintsTests.NestedGenericStructContainingPointer()
  ----- Inner Stack Trace -----
  C:\Users\rigibson\src\roslyn\src\Test\Utilities\Portable\Platform\Desktop\RuntimeAssemblyManager.cs(455,0): at Roslyn.Test.Utilities.Desktop.RuntimeAssemblyManager.PeVerifyModules(String[] modulesToVerify, Boolean throwOnError)
     at Roslyn.Test.Utilities.Desktop.RuntimeAssemblyManager.PeVerifyModules(String[] modulesToVerify, Boolean throwOnError)
  C:\Users\rigibson\src\roslyn\src\Test\Utilities\Portable\Platform\Desktop\DesktopRuntimeEnvironment.cs(310,0): at Roslyn.Test.Utilities.Desktop.DesktopRuntimeEnvironment.Verify(Verification verification)

Umbrella issue for unmanaged constructed types: #31374

@RikkiGibson RikkiGibson added this to the 16.0.P2 milestone Dec 7, 2018
@RikkiGibson RikkiGibson requested a review from a team as a code owner December 7, 2018 00:54
@RikkiGibson RikkiGibson changed the title [WIP] Unmanaged construction types tests [WIP] Unmanaged constructed types tests Dec 7, 2018
@jaredpar
Copy link
Member

jaredpar commented Dec 7, 2018

@RikkiGibson

Seeing an interesting test failure in the new verification test in

That is expected. The use of pointers in C# are not guaranteed to be verifiable. There are cases where they can be verified but the overall expectation is that they will fail to verify. For that test I would just disable verification (it's an argument to CompileAndVerify).

@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 7, 2018
@RikkiGibson RikkiGibson force-pushed the unmanaged-constructed-types-testing branch from 1707d58 to 80f47aa Compare December 17, 2018 20:58
@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Dec 17, 2018 •
edited
Loading

Added some commits specifically to address LangVersion 7.3 handling. I pretty much looked at where Error.ManagedAddr is emitted and expanded the logic. Maybe better factoring is possible?

@RikkiGibson RikkiGibson force-pushed the unmanaged-constructed-types-testing branch from 1884415 to 80721b4 Compare December 18, 2018 00:23
@RikkiGibson RikkiGibson removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 19, 2018
@RikkiGibson RikkiGibson changed the title [WIP] Unmanaged constructed types tests Unmanaged constructed types tests Dec 19, 2018
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler please have a look

@RikkiGibson RikkiGibson force-pushed the unmanaged-constructed-types-testing branch from a198a47 to 226d06a Compare January 8, 2019 20:48
@RikkiGibson RikkiGibson force-pushed the unmanaged-constructed-types-testing branch from 226d06a to f0def60 Compare January 8, 2019 20:51
@jaredpar
Copy link
Member

jaredpar commented Jan 9, 2019

Done with review pass (Iteration 34). Looking good. Had one question about a comment I didn't see addressed.

jaredpar
jaredpar approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

:shipit:

@RikkiGibson
Fixes from feedback
71a1f82 
Fixed a bug in constraint checking where the wrong Compilation
would be used.
@RikkiGibson
Add test demonstrating expected behavior around multiple constraint v…
b9e214f 
…iolations
@RikkiGibson
Copy link
Contributor Author

Finished the requested changes @agocke

agocke
agocke approved these changes Jan 10, 2019
View reviewed changes
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@RikkiGibson
Copy link
Contributor Author

🎉 Thanks for bearing with this PR. Now I know to split them apart a bit more in the future.

@RikkiGibson RikkiGibson merged commit eaa0cd8 into dotnet:features/unmanaged-constructed-types Jan 11, 2019
@RikkiGibson RikkiGibson deleted the unmanaged-constructed-types-testing branch January 11, 2019 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agocke agocke agocke approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
16.0.P2
Development

Successfully merging this pull request may close these issues.
4 participants
@RikkiGibson @jaredpar @agocke @jcouv