-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[Arm64/Windows] Backport #9500 changes #10190
Changes from 1 commit
a230fe2
15f2e26
5d1922d
e59ab7b
4db5a72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,6 +159,7 @@ | |
RestoreRegMS 26, X26 | ||
RestoreRegMS 27, X27 | ||
RestoreRegMS 28, X28 | ||
RestoreRegMS 29, X29 | ||
|
||
Done | ||
; Its imperative that the return value of HelperMethodFrameRestoreState is zero | ||
|
@@ -991,33 +992,48 @@ UM2MThunk_WrapperHelper_RegArgumentsSetup | |
|
||
; This helper enables us to call into a funclet after restoring Fp register | ||
NESTED_ENTRY CallEHFunclet | ||
|
||
; Using below prolog instead of PROLOG_SAVE_REG_PAIR fp,lr, #-16! | ||
; is intentional. Above statement would also emit instruction to save | ||
; sp in fp. If sp is saved in fp in prolog then it is not expected that fp can change in the body | ||
; of method. However, this method needs to be able to change fp before calling funclet. | ||
; This is required to access locals in funclet. | ||
PROLOG_SAVE_REG_PAIR x19,x20, #-16! | ||
PROLOG_SAVE_REG fp, #0 | ||
PROLOG_SAVE_REG lr, #8 | ||
|
||
; On entry: | ||
; | ||
; X0 = throwable | ||
; X1 = PC to invoke | ||
; X2 = address of X19 register in CONTEXT record; used to restore the non-volatile registers of CrawlFrame | ||
; X3 = address of the location where the SP of funclet's caller (i.e. this helper) should be saved. | ||
; | ||
|
||
; Using below prolog instead of PROLOG_SAVE_REG_PAIR fp,lr, #-16! | ||
; is intentional. Above statement would also emit instruction to save | ||
; sp in fp. If sp is saved in fp in prolog then it is not expected that fp can change in the body | ||
; of method. However, this method needs to be able to change fp before calling funclet. | ||
; This is required to access locals in funclet. | ||
PROLOG_SAVE_REG_PAIR x29,lr, #-96! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Failing test is causing an exception to happen in a finalizer (being executed as part of unwind). At that point OS is not able to unwind correctly out of CallEHFunction. This is because CallEHFunction modifies fp outside of the prolog of the function. CallEHFunction needs to be able to modify fp outside of the prolog to set the correct fp in order to get the locals for the finalizer. In order to make that happen prolog must not assume that fp can be used to unwind out of the function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Using x29 instead of FP was able to trick the unixmacro preprocessor to avoid that unwinding code. I do not see the Windows version of the macros, they must be part of the ksarm64.h header. @rahku Do you want me to push the patch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Windows has another macro called PROLOG_SAVE_REG_PAIR_NO_FP that could be used here instead of doing this trick. @rahku do you think it would make sense to use it instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that works too. I just verified that. I think this macro was not present when I did this work initially. @sdmaclea can you update this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure |
||
|
||
; Spill callee saved registers | ||
PROLOG_SAVE_REG_PAIR x19, x20, 16 | ||
PROLOG_SAVE_REG_PAIR x21, x22, 32 | ||
PROLOG_SAVE_REG_PAIR x23, x24, 48 | ||
PROLOG_SAVE_REG_PAIR x25, x26, 64 | ||
PROLOG_SAVE_REG_PAIR x27, x28, 80 | ||
|
||
; Save the SP of this function. We cannot store SP directly. | ||
mov fp, sp | ||
str fp, [x3] | ||
|
||
ldp x19, x20, [x2, #0] | ||
ldp x21, x22, [x2, #16] | ||
ldp x23, x24, [x2, #32] | ||
ldp x25, x26, [x2, #48] | ||
ldp x27, x28, [x2, #64] | ||
ldr fp, [x2, #80] ; offset of fp in CONTEXT relative to X19 | ||
|
||
; Invoke the funclet | ||
blr x1 | ||
nop | ||
|
||
EPILOG_RESTORE_REG_PAIR x19, x20, 16 | ||
EPILOG_RESTORE_REG_PAIR x21, x22, 32 | ||
EPILOG_RESTORE_REG_PAIR x23, x24, 48 | ||
EPILOG_RESTORE_REG_PAIR x25, x26, 64 | ||
EPILOG_RESTORE_REG_PAIR x27, x28, 80 | ||
EPILOG_RESTORE_REG_PAIR fp, lr, #16! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be EPILOG_RESTORE_REG_PAIR fp, lr, #96! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rahku Yes you are right. Show how dependent I am on compile test workflow.... |
||
EPILOG_RETURN | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x19 is callee saved so it should not be trashed on return from call. Can you explain who/when is x19 getting corrupted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a workaround for unexpected x19 corruption. I had tried to debug this failure by storing the correct value for x19 on the stack above and adding a conditional break, but was unable to do so. The test seemed to fail to make forward progress.
Let me try to reproduce. I may have better luck as the GC is more stable now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removing the equivalent of this line, there are 43 failures on [Arm64/Unix] tip.
The failure signatures are not exactly what I remember. I will attempt to debug a shorter one to see if I can root cause the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the failures are a regression on the tip . When the regression is fixed, looks like I may be able to remove this workaround from the [Arm64/Unix] assembly.