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

Convert AssemblyNative::Load to QCall #1929

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Convert AssemblyNative::Load to QCall #1929

merged 4 commits into from
Jan 28, 2020

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Jan 20, 2020

  • FCall uses libunwind to find Method description.
  • Get rid of the libunwind overhead in AssemblyNative::Load
    by converting to QCall

@clamp03
Copy link
Member Author

clamp03 commented Jan 20, 2020

This is a PR to fix an issue in #1603
It is the first time to implement QCall. I think there are many mistakes in it.
Could you please review?
Thank you.

@clamp03 clamp03 mentioned this pull request Jan 20, 2020
Closed
@jkotas
Copy link
Member

jkotas commented Jan 20, 2020

I have done some related cleanup in #1936. Hope it won't conflict with your change too much.

@clamp03
Copy link
Member Author

clamp03 commented Jan 21, 2020

@jkotas Fixed all. Thank you.

@jkotas
Copy link
Member

jkotas commented Jan 21, 2020

Tests are failing with:

Assert failure(PID 5392 [0x00001510], Thread: 4832 [0x12e0]): Thread::IsObjRefValid(this)

CORECLR! GetCLRRuntimeHost + 0x1F14E1 (0x72869401)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x720d34fc)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x7225b75b)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x7224ff25)
```

@clamp03
Convert AssemblyNative::Load to QCall
728af91 
- FCall uses libunwind to find Method description.
- Get rid of the libunwind overhead in AssemblyNative::Load
  by converting to QCall
@jkotas
Copy link
Member

jkotas commented Jan 22, 2020

Several tests fails on Libraries Test Run checked CoreCLR Windows_NT x86 Release with:

  Starting:    System.Reflection.Tests (parallel test collections = on, max threads = 2)

Assert failure(PID 3728 [0x00000e90], Thread: 3904 [0x0f40]): !"SetFrame frame == stopFrame"

CORECLR! GetCLRRuntimeHost + 0xCA5F4 (0x724b2514)
CORECLR! GetCLRRuntimeHost + 0xBC00B (0x724a3f2b)
CORECLR! GetCLRRuntimeHost + 0x48E5CA (0x728764ea)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x71e434e8)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x71fcc387)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x71fc0a53)

I do not see any obvious problem in the delta that would be causing this. If you are not able to figure out what's wrong, let me know and I will help you debug it.

One thing you can try is to move the GC_PROTECT to be inside InitializeSpec, and change the InitializeSpec argument to be ASSEMBLYREF.

@clamp03
Copy link
Member Author

clamp03 commented Jan 23, 2020

@jkotas Could you please help to debug?
Actually, I cannot build "checked CoreCLR Windows_NT x86 Release" because of network/security issues in my company.
Thank you.

@jkotas
Copy link
Member

jkotas commented Jan 24, 2020

I am not able to reproduce the crash locally, and the dump does not have enough clues about what went wrong. I will keep looking...

@jkotas
Copy link
Member

jkotas commented Jan 24, 2020

SetFrame frame == stopFrame"

This is a bug in exception handling on Windows x86.

The problem is in the Windows x86 path here: https://github.com/dotnet/coreclr/pull/24199/files#diff-5eb922d9071dbbd2c84a9fa5f4468ea0R1880

We are missing a check like this: https://github.com/dotnet/coreclr/pull/24199/files#diff-d5e4cb27da3539140a16dc576906b925R1857

It results into unwinding too many Frames that sometime leads to asserts about corrupted Frame chain and crashes.

@jkotas
Copy link
Member

jkotas commented Jan 24, 2020

cc @janvorli

@jkotas jkotas added the blocked Issue/PR is blocked on something - see comments label Jan 24, 2020
@janvorli
Copy link
Member

I'll fix that.

@clamp03
Copy link
Member Author

clamp03 commented Jan 24, 2020

@jkotas @janvorli Thak you.

@janvorli janvorli mentioned this pull request Jan 27, 2020
Merged
@jkotas jkotas mentioned this pull request Jan 27, 2020
Closed
@jkotas jkotas merged commit 4011572 into dotnet:master Jan 28, 2020
@jkotas
Copy link
Member

jkotas commented Jan 28, 2020

@clamp03 Thank you!

@clamp03
Copy link
Member Author

clamp03 commented Jan 28, 2020

@jkotas
I found that arm_phdr_cb is called in "BindToMethodInfo in System Delegate.CoreCLR.cs" and "CreateDynamicAssembly in System.Reflection.Emit AssemblyBuilder.cs" too.
If you don't mind, I want to convert the functions to QCall after the #2215 issue is fixed.
Thank you.

@jkotas
Copy link
Member

jkotas commented Jan 28, 2020 •
edited
Loading

@clamp03 Yes, it is fine to convert any FCalls with HELPER_METHOD_FRAME to QCalls. I do not think you need to block on #2215 . You just got unlucky that you hit this problem in this PR. Other similar conversions should not hit it.

BindToMethodInfo

Where do you see it called in BindToMethodInfo? I would not expect it to be called under BindToMethodInfo. This method does not take StackCrawlMark argument.

@clamp03
Copy link
Member Author

clamp03 commented Jan 29, 2020

Thanks. Below is a backtrace when arm_find_proc_info is called.

 * frame #0: 0xef1b5394 libcoreclr.so`_ULarm_find_proc_info(as=0xef3d4cb0, ip=4009840052, pi=0xfffdac14, need_unwind_info=1, arg=0xfffde9f0) at Gex_tables.c:530
    frame #1: 0xef1b4a1e libcoreclr.so`_ULarm_step [inlined] _ULarm_arm_exidx_step(c=0xfffda9f0) at Gstep.c:56
    frame #2: 0xef1b49e2 libcoreclr.so`_ULarm_step(cursor=0xfffda9f0) at Gstep.c:120
    frame #3: 0xef1892f0 libcoreclr.so`::PAL_VirtualUnwind(context=0xfffdeab0, contextPointers=0xfffdea6c) at seh-unwind.cpp:308
    frame #4: 0xeefdf640 libcoreclr.so`LazyMachState::unwindLazyState(baseState=<unavailable>, unwoundstate=0xfffdec80, threadId=<unavailable>, funCallDepth=1, hostCallPreference=AllowHostCalls) at stubs.cpp:549
    frame #5: 0xeeecf2fe libcoreclr.so`HelperMethodFrame::InsureInit(this=0xfffdf8d4, initialInit=<unavailable>, unwindState=<unavailable>, hostCallPreference=<unavailable>) at frames.cpp:0
    frame #6: 0xeeecf29a libcoreclr.so`HelperMethodFrame::GetFunction(this=0xfffdf8d4) at frames.cpp:1691
    frame #7: 0xeef1640c libcoreclr.so`StackFrameIterator::ProcessCurrentFrame(this=0xfffdedd8) at stackwalk.cpp:3003
    frame #8: 0xeef17022 libcoreclr.so`StackFrameIterator::NextRaw(this=0xfffdedd8) at stackwalk.cpp:2754
    frame #9: 0xeef16c16 libcoreclr.so`StackFrameIterator::Filter(this=0xfffdedd8) at stackwalk.cpp:2286
    frame #10: 0xeef160aa libcoreclr.so`StackFrameIterator::Init(this=0xfffdedd8, pThread=0x0178d348, pFrame=<unavailable>, pRegDisp=0xfffdf0d0, flags=33) at stackwalk.cpp:1271
    frame #11: 0xeef15ed6 libcoreclr.so`Thread::StackWalkFramesEx(this=0x0178d348, pRD=0xfffdf0d0, pCallback=(libcoreclr.so`SystemDomain::CallersMethodCallbackWithStackMark(CrawlFrame*, void*) + 1 at appdomain.cpp:2498), pData=0xfffdf6a8, flags=33, pStartFrame=<unavailable>)(CrawlFrame*, void*), void*, unsigned int, Frame*) at stackwalk.cpp:956
    frame #12: 0xeef161a0 libcoreclr.so`Thread::StackWalkFrames(this=0x0178d348, pCallback=(libcoreclr.so`SystemDomain::CallersMethodCallbackWithStackMark(CrawlFrame*, void*) + 1 at appdomain.cpp:2498), pData=0xfffdf6a8, flags=33, pStartFrame=<unavailable>)(CrawlFrame*, void*), void*, unsigned int, Frame*) at stackwalk.cpp:1043
    frame #13: 0xeefe8734 libcoreclr.so`SystemDomain::GetCallersMethod(stackMark=0x00000000, ppAppDomain=0xfffdf810) at appdomain.cpp:2411
    frame #14: 0xef062fd6 libcoreclr.so`RefSecContext::GetCallerMethod() [inlined] RefSecContext::FindCaller(this=<unavailable>) at invokeutil.cpp:1328
    frame #15: 0xef062fcc libcoreclr.so`RefSecContext::GetCallerMethod(this=<unavailable>) at invokeutil.cpp:1349
    frame #16: 0xef063092 libcoreclr.so`RefSecContext::IsCalledFromInterop(this=<unavailable>) at invokeutil.cpp:1398
    frame #17: 0xef012cf4 libcoreclr.so`ClassLoader::CheckAccessMember(pContext=0xfffdf804, pTargetMT=0xe42dabd4, pTargetAssembly=0x017bf3f8, dwMemberAccess=<unavailable>, pOptionalTargetMethod=0xe42dacac, pOptionalTargetField=<unavailable>, accessCheckOptions=0xfffdf740) at clsload.cpp:5261
    frame #18: 0xef012bc4 libcoreclr.so`ClassLoader::CanAccess(pContext=0xfffdf804, pTargetMT=0xe42dabd4, pTargetAssembly=0x017bf3f8, dwMemberAccess=147, pOptionalTargetMethod=<unavailable>, pOptionalTargetField=0x00000000, accessCheckOptions=0xfffdf7c4) at clsload.cpp:5142
    frame #19: 0xef0633a6 libcoreclr.so`InvokeUtil::CheckAccess(pCtx=0xfffdf804, pTargetMT=0xe42dabd4, pInstanceMT=0x00000000, pTargetMethod=0xe42dacac, pTargetField=0x00000000, accessCheckOptions=0xfffdf7c4) at invokeutil.cpp:1639
    frame #20: 0xef063224 libcoreclr.so`InvokeUtil::CheckAccessMethod(pCtx=<unavailable>, pTargetMT=<unavailable>, pInstanceMT=<unavailable>, pTargetMethod=<unavailable>) at invokeutil.cpp:1545
    frame #21: 0xef0149aa libcoreclr.so`COMDelegate::BindToMethod(pRefThis=0xfffdf8b0, pRefFirstArg=0xfffdf8b4, pTargetMethod=0xe42dacac, pExactMethodType=0xe42dabd4, fIsOpenDelegate=<unavailable>, fCheckSecurity=YES) at comdelegate.cpp:916
    frame #22: 0xef014f4e libcoreclr.so`COMDelegate::BindToMethodInfo(refThisUNSAFE=<unavailable>, targetUNSAFE=<unavailable>, pMethodUNSAFE=<unavailable>, pMethodTypeUNSAFE=<unavailable>, flags=<unavailable>) at comdelegate.cpp:847 <= FCall
    frame #23: 0xeb3d86f2
    frame #24: 0xeb3851d8
    frame #25: 0xeb38517e
    frame #26: 0xe973372a
    frame #27: 0xe61237a8
    frame #28: 0xebcf6f1a
    frame #29: 0xe6126882
    frame #30: 0xe6126960
    frame #31: 0xe61274c8
    frame #32: 0xe6127a3c
    frame #33: 0xebcf43cc
    frame #34: 0xe6125000
    frame #35: 0xe60dcab2
    frame #36: 0xe60dcade
    frame #37: 0xe60bcf86
    frame #38: 0xe60bce66
    frame #39: 0xe70198fa
    frame #40: 0xe7018932
    frame #41: 0xe7018780
    frame #42: 0xeefe4bd6 libcoreclr.so`CallDescrWorkerInternal at unixasmmacrosarm.inc:666

@clamp03 clamp03 deleted the convert branch January 29, 2020 08:29
@jkotas
Copy link
Member

jkotas commented Jan 29, 2020

I think this should be just deleted. Submitted WIP PR #2348

@jkotas jkotas mentioned this pull request Jun 25, 2020
Open
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@vitek-karas vitek-karas vitek-karas left review comments

@elinor-fung elinor-fung Awaiting requested review from elinor-fung

Assignees
No one assigned
Labels
area-AssemblyLoader-coreclr blocked Issue/PR is blocked on something - see comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@clamp03 @jkotas @janvorli @vitek-karas