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.

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

Added UnwindInfo as an optional printout #2043

Merged
merged 8 commits into from
Jun 24, 2020

Conversation

edkazcarlson
Copy link
Contributor

Here I tried to add unwindinfo to the print out, as well as adding an option to turn it on or off.
Before the change it would look like:

; Void System.IO.BinaryReader.Dispose()
; Prolog
000000000083FC50 55                   push      rbp
000000000083FC51 4883EC20             sub       rsp,20h
000000000083FC55 488D6C2420           lea       rbp,[rsp+20h]
000000000083FC5A 48894D10             mov       [rbp+10h],rcx
; IL_0000
000000000083FC5E 90                   nop
; IL_0001
000000000083FC5F 488B4D10             mov       rcx,[rbp+10h]
000000000083FC63 BA01000000           mov       edx,1
000000000083FC68 488B4510             mov       rax,[rbp+10h]
000000000083FC6C 488B00               mov       rax,[rax]
000000000083FC6F 488B4048             mov       rax,[rax+48h]
; IL_0003
000000000083FC73 FF5028               call      qword [rax+28h]
; IL_0008
000000000083FC76 90                   nop
; IL_0009
000000000083FC77 90                   nop
; Epilog
000000000083FC78 488D6500             lea       rsp,[rbp]
000000000083FC7C 5D                   pop       rbp
000000000083FC7D C3                   ret

After turning the option on under View->Options -> ReadyToRun it would look like:

; Void System.IO.BinaryReader.Dispose()
; UnwindInfo:
; Version:            1
; Flags:              0x03 EHANDLER UHANDLER
; FrameRegister:      none
; System.Collections.Generic.Dictionary`2[System.UInt64,ILCompiler.Reflection.ReadyToRun.Amd64.UnwindCode]
; Prolog
000000000083FC50 55                   push      rbp
000000000083FC51 4883EC20             sub       rsp,20h ; UnwindCode: OpCode: UWOP_PUSH_NONVOL Op: RBP(5)
000000000083FC55 488D6C2420           lea       rbp,[rsp+20h] ; UnwindCode: OpCode: UWOP_ALLOC_SMALL Op: 32
000000000083FC5A 48894D10             mov       [rbp+10h],rcx
; IL_0000
000000000083FC5E 90                   nop
; IL_0001
000000000083FC5F 488B4D10             mov       rcx,[rbp+10h]
000000000083FC63 BA01000000           mov       edx,1
000000000083FC68 488B4510             mov       rax,[rbp+10h]
000000000083FC6C 488B00               mov       rax,[rax]
000000000083FC6F 488B4048             mov       rax,[rax+48h]
; IL_0003
000000000083FC73 FF5028               call      qword [rax+28h]
; IL_0008
000000000083FC76 90                   nop
; IL_0009
000000000083FC77 90                   nop
; Epilog
000000000083FC78 488D6500             lea       rsp,[rbp]
000000000083FC7C 5D                   pop       rbp
000000000083FC7D C3                   ret

@christophwille
Copy link
Member

@cshung for this and future PRs from Ed in R2R - please add a comment when you reviewed & ok'd it (can't add you as a formal reviewers to the PR because GH doesn't allow me to pick arbitrary people)

@cshung
Copy link
Contributor

cshung commented Jun 20, 2020

Congrats @edkazcarlson for your first PR in ILSpy. Thanks @christophwille for the heads up.

@edkazcarlson edkazcarlson marked this pull request as ready for review June 20, 2020 22:43
@edkazcarlson edkazcarlson marked this pull request as draft June 20, 2020 22:46
@edkazcarlson edkazcarlson marked this pull request as ready for review June 23, 2020 00:26
@cshung
Copy link
Contributor

cshung commented Jun 23, 2020

Please hold this change, I think I found a bug, I will look into it more.

WriteCommentLine(output, $"Flags: 0x{amd64UnwindInfo.Flags:X2}{parsedFlags}");
WriteCommentLine(output, $"FrameRegister: {((amd64UnwindInfo.FrameRegister == 0) ? "none" : amd64UnwindInfo.FrameRegister.ToString())}");
for (int unwindCodeIndex = 0; unwindCodeIndex < amd64UnwindInfo.CountOfUnwindCodes; unwindCodeIndex++) {
unwindCodes.Add(amd64UnwindInfo.UnwindCodeArray[unwindCodeIndex].CodeOffset, amd64UnwindInfo.UnwindCodeArray[unwindCodeIndex]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The CodeOffset value here corresponds to its offset from the beginning of the i (i.e. The RuntimeFunction object). The name RuntimeFunction is really unfortunate, we cannot rename it as it is part of the Windows API, but it doesn't really mean what you think it is. In particular, it is not the whole method. A ReadyToRunMethod could have multiple RuntimeRunction objects that correspond to different portions of the method's code that has its own unwind data.

In practice, this is used for funclets. A funclet is a runtime construct used to support exception filtering and exception handling. A when block or a catch block could potentially access the local variables of the method, therefore a catch block is not an independent function. An implementation trick to accomplish this is to set the frame pointer to be the same the parent frame and access the local variables that way, but keep the stack pointer unchanged (in case of filter) to make sure the original stack stay intact or set the stack pointer to the frame's stack pointer (in case of catch) to represent a complete unwind. In both case, and exception or a GC could happen while the filter/catch is in progress and therefore we still need to be able to unwind there, and we will need special provision to deal with such tricky frames. This is why funclets are their own RuntimeFunction.

The code here is buggy because if we happen to have two runtime functions, and both of them have an unwind opcode at code offset 0, we will be adding duplicated dictionary entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I have fixed this by calling WriteUnwindInfo once per RunTimeFunction instead of once per ReadyToRunMethod, and so a code offset 0 for one RunTimeFunction will not overlap with the code offset 0 of another RunTimeFunction.

Copy link
Contributor

@cshung cshung left a comment

Choose a reason for hiding this comment

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

With the change I suggested, the code should work on methods with catch blocks.

ILSpy.ReadyToRun/ReadyToRunLanguage.cs Outdated Show resolved Hide resolved
@siegfriedpammer
Copy link
Member

Can this be merged?

@cshung
Copy link
Contributor

cshung commented Jun 24, 2020

Can this be merged?

I think the code is ready to be merged.

@siegfriedpammer siegfriedpammer merged commit c0ce62e into icsharpcode:master Jun 24, 2020
@siegfriedpammer
Copy link
Member

Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants