Refactoring IShiftOperators to take a `TOther` by tannergooding · Pull Request #71405 · dotnet/runtime

tannergooding · 2022-06-28T21:20:16Z

This is failing locally, checking if CI provides any additional details.

ghost · 2022-06-28T21:20:26Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-06-28T21:20:31Z

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

This is failing locally, checking if CI provides any additional details.

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Numerics`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2022-06-29T15:18:10Z

@davidwrighton, @lambdageek seeing:

Assert failure(PID 27404 [0x00006b0c], Thread: 7640 [0x1dd8]): pMT != NULL

CORECLR! CoreLibBinder::GetExistingClass + 0x3E (0x00007ff9`d12f59ae)
CORECLR! CoreLibBinder::GetElementType + 0x1A (0x00007ff9`d131f90a)
CORECLR! SigPointer::GetTypeHandleThrowing + 0x937 (0x00007ff9`d1664dc7)
CORECLR! SigPointer::GetTypeHandleThrowing + 0x19B0 (0x00007ff9`d1665e40)
CORECLR! ClassLoader::LoadTypeDefOrRefOrSpecThrowing + 0x9B5 (0x00007ff9`d13fc4c5)
CORECLR! MethodTableBuilder::LoadExactInterfaceMap + 0x5D9 (0x00007ff9`d192d469)
CORECLR! ClassLoader::LoadExactParentAndInterfacesTransitively + 0x654 (0x00007ff9`d13cc344)
CORECLR! ClassLoader::LoadExactParents + 0x431 (0x00007ff9`d13cc7d1)
CORECLR! ClassLoader::DoIncrementalLoad + 0x78D (0x00007ff9`d13ee7fd)
CORECLR! ClassLoader::LoadTypeHandleForTypeKey_Body + 0xDDC (0x00007ff9`d140059c)
    File: C:\Users\tagoo\source\repos\runtime\src\coreclr\vm\binder.h Line: 393
    Image: C:\Users\tagoo\source\repos\runtime\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\corerun.exe

I'm unsure what's "special" here that's causing the error. it seems to occur just by adding a generic type parameter to IShiftOperators

tannergooding · 2022-06-29T15:25:36Z

Stack trace here is the following with it seemingly occurring just as part of startup:

> coreclr.dll!DbgAssertDialog(const char * szFile, int iLine, const char * szExpr) Line 599	C++
  coreclr.dll!CoreLibBinder::GetExistingClass(BinderClassID id) Line 393	C++
  coreclr.dll!CoreLibBinder::GetElementType(CorElementType type) Line 218	C++
  coreclr.dll!SigPointer::GetTypeHandleThrowing(Module * pModule, const SigTypeContext * pTypeContext, ClassLoader::LoadTypesFlag fLoadTypes, ClassLoadLevel level, int dropGenericArgumentLevel, const Substitution * pSubst, const ZapSig::Context * pZapSigContext, MethodTable * pMTInterfaceMapOwner) Line 1091	C++
  coreclr.dll!SigPointer::GetTypeHandleThrowing(Module * pModule, const SigTypeContext * pTypeContext, ClassLoader::LoadTypesFlag fLoadTypes, ClassLoadLevel level, int dropGenericArgumentLevel, const Substitution * pSubst, const ZapSig::Context * pZapSigContext, MethodTable * pMTInterfaceMapOwner) Line 1388	C++
  coreclr.dll!ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module * pModule, unsigned int typeDefOrRefOrSpec, const SigTypeContext * pTypeContext, ClassLoader::NotFoundAction fNotFoundAction, ClassLoader::PermitUninstantiatedFlag fUninstantiated, ClassLoader::LoadTypesFlag fLoadTypes, ClassLoadLevel level, int dropGenericArgumentLevel, const Substitution * pSubst, MethodTable * pMTInterfaceMapOwner) Line 2035	C++
  coreclr.dll!MethodTableBuilder::LoadExactInterfaceMap(MethodTable * pMT) Line 9208	C++
  coreclr.dll!ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable * pMT) Line 908	C++
  coreclr.dll!ClassLoader::LoadExactParents(MethodTable * pMT) Line 941	C++
  coreclr.dll!ClassLoader::DoIncrementalLoad(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel currentLevel) Line 2871	C++
  coreclr.dll!ClassLoader::LoadTypeHandleForTypeKey_Body(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel targetLevel) Line 3516	C++
  coreclr.dll!ClassLoader::LoadTypeHandleForTypeKey(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel targetLevel, const InstantiationContext * pInstContext) Line 3235	C++
  coreclr.dll!ClassLoader::LoadTypeDefThrowing(Module * pModule, unsigned int typeDef, ClassLoader::NotFoundAction fNotFoundAction, ClassLoader::PermitUninstantiatedFlag fUninstantiated, unsigned int tokenNotToLoad, ClassLoadLevel level, Instantiation * pTargetInstantiation) Line 2216	C++
  coreclr.dll!SigPointer::GetTypeHandleThrowing(Module * pModule, const SigTypeContext * pTypeContext, ClassLoader::LoadTypesFlag fLoadTypes, ClassLoadLevel level, int dropGenericArgumentLevel, const Substitution * pSubst, const ZapSig::Context * pZapSigContext, MethodTable * pMTInterfaceMapOwner) Line 1422	C++
  coreclr.dll!ClassLoader::LoadTypeDefOrRefOrSpecThrowing(Module * pModule, unsigned int typeDefOrRefOrSpec, const SigTypeContext * pTypeContext, ClassLoader::NotFoundAction fNotFoundAction, ClassLoader::PermitUninstantiatedFlag fUninstantiated, ClassLoader::LoadTypesFlag fLoadTypes, ClassLoadLevel level, int dropGenericArgumentLevel, const Substitution * pSubst, MethodTable * pMTInterfaceMapOwner) Line 2035	C++
  coreclr.dll!MethodTableBuilder::LoadExactInterfaceMap(MethodTable * pMT) Line 9208	C++
  coreclr.dll!ClassLoader::LoadExactParentAndInterfacesTransitively(MethodTable * pMT) Line 908	C++
  coreclr.dll!ClassLoader::LoadExactParents(MethodTable * pMT) Line 941	C++
  coreclr.dll!ClassLoader::DoIncrementalLoad(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel currentLevel) Line 2871	C++
  coreclr.dll!ClassLoader::LoadTypeHandleForTypeKey_Body(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel targetLevel) Line 3516	C++
  coreclr.dll!ClassLoader::LoadTypeHandleForTypeKey(TypeKey * pTypeKey, TypeHandle typeHnd, ClassLoadLevel targetLevel, const InstantiationContext * pInstContext) Line 3235	C++
  coreclr.dll!ClassLoader::EnsureLoaded(TypeHandle typeHnd, ClassLoadLevel level) Line 1021	C++
  coreclr.dll!CoreLibBinder::LoadPrimitiveType(CorElementType et) Line 1194	C++
  coreclr.dll!SystemDomain::LoadBaseSystemClasses() Line 1378	C++
  coreclr.dll!SystemDomain::Init() Line 1203	C++
  coreclr.dll!EEStartupHelper() Line 935	C++
  coreclr.dll!`EEStartup'::`9'::__Body::Run(void * p) Line 1073	C++
  coreclr.dll!EEStartup() Line 1075	C++
  coreclr.dll!EnsureEEStarted() Line 311	C++
  coreclr.dll!CorHost2::Start() Line 101	C++

Potentially related to the specializations that the built-in types have to reduce the cost of the interfaces?

tannergooding · 2022-06-29T16:18:38Z

Should probably tag @trylek as well

tannergooding · 2022-06-29T16:28:29Z

Logged #71441

dakersnar

Looks good. All of our types here use int as TOther, right? Is the intent to allow users implementing the interface to use different types as the shiftAmount?

dakersnar · 2022-07-05T17:19:53Z

src/coreclr/vm/appdomain.cpp

    CoreLibBinder::GetModule()->AllocateRegularStaticHandles(DefaultDomain());

+    // Int32 has to be loaded first to break cycle in IShiftOperators
+    CoreLibBinder::LoadPrimitiveType(ELEMENT_TYPE_I4);


What's this change?

A change from Jan that fixes a VM bug that was preventing this change from working.

Refactoring IShiftOperators to take a TOther

9dd82af

ghost added area-System.Numerics new-api-needs-documentation labels Jun 28, 2022

ghost assigned tannergooding Jun 28, 2022

tannergooding added this to the 7.0.0 milestone Jun 29, 2022

tannergooding mentioned this pull request Jun 29, 2022

CoreLibBinder asserts with certain static abstract in interface declarations #71441

Closed

tannergooding added the blocked Issue/PR is blocked on something - see comments label Jun 29, 2022

Break IShiftOperators typeloading cycle

fefbf58

jkotas removed the blocked Issue/PR is blocked on something - see comments label Jun 30, 2022

tannergooding marked this pull request as ready for review June 30, 2022 20:45

tannergooding requested a review from dakersnar July 5, 2022 16:57

dakersnar approved these changes Jul 5, 2022

View reviewed changes

tannergooding merged commit 6b0d373 into dotnet:main Jul 5, 2022

karelz mentioned this pull request Jul 21, 2022

[WinHttp] Manual_CertificateOnlySentWhenValid_Success fails with WINHTTP_CALLBACK_STATUS_REQUEST_ERROR in rolling CI #69238

Open

ghost locked as resolved and limited conversation to collaborators Aug 4, 2022

tannergooding deleted the generic-math-shift branch November 11, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring IShiftOperators to take a `TOther`#71405

Refactoring IShiftOperators to take a `TOther`#71405
tannergooding merged 2 commits intodotnet:mainfrom
tannergooding:generic-math-shift

tannergooding commented Jun 28, 2022

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

tannergooding commented Jun 29, 2022

Uh oh!

tannergooding commented Jun 29, 2022 •

edited

Loading

Uh oh!

tannergooding commented Jun 29, 2022

Uh oh!

tannergooding commented Jun 29, 2022

Uh oh!

dakersnar left a comment •

edited

Loading

Uh oh!

dakersnar Jul 5, 2022

Uh oh!

tannergooding Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tannergooding commented Jun 28, 2022

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

ghost commented Jun 28, 2022

Uh oh!

tannergooding commented Jun 29, 2022

Uh oh!

tannergooding commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Jun 29, 2022

Uh oh!

tannergooding commented Jun 29, 2022

Uh oh!

dakersnar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakersnar Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

tannergooding Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tannergooding commented Jun 29, 2022 •

edited

Loading

dakersnar left a comment •

edited

Loading