Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[x86/Linux] Revise asmhelper.S using macro #8523

Merged
merged 3 commits into from
Dec 9, 2016
Merged

[x86/Linux] Revise asmhelper.S using macro #8523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d0cdc5
[x86/Linux] Revise asmhelper.S using macro
parjong Dec 8, 2016
9ebc50a
Revise per feedback
parjong Dec 8, 2016
d16baaa
Reduce CFI directives
parjong Dec 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/pal/inc/unixasmmacrosx86.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,31 @@ C_FUNC(\Name\()_End):
.global C_FUNC(\Name\()_End)
LEAF_END \Name, \Section
.endm

.macro PROLOG_BEG
push ebp
mov ebp, esp
.cfi_def_cfa_offset 0
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct. The CFA by definition is the ESP of the caller right before it made the call. You are setting the CFA here to the location where the EBP was pushed.
You need to set the offset to 8 (the pushed EBP plus the return address).

Copy link
Member

Choose a reason for hiding this comment

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

And you need to also define the offset of EBP here

.cfi_rel_offset ebp, 0

.endm

.macro PROLOG_PUSH Reg
push \Reg
.cfi_adjust_cfa_offset -4
.cfi_rel_offset \Reg, 0
.endm

.macro PROLOG_END
.cfi_def_cfa_offset 4
Copy link
Member

Choose a reason for hiding this comment

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

This should be 8, see my comment in the PROLOG_BEG

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for comment. I'll revise those macros as you commented.

.cfi_def_cfa_register ebp
.endm

.macro EPILOG_BEG
.endm

.macro EPILOG_POP Reg
pop \Reg
.endm

.macro EPILOG_END
pop ebp
.endm
66 changes: 45 additions & 21 deletions src/vm/i386/asmhelpers.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,32 +11,39 @@
//
.macro STUB_PROLOG
// push ebp-frame
push ebp
mov ebp, esp
PROLOG_BEG

// save CalleeSavedRegisters
push ebx
push esi
push edi
PROLOG_PUSH ebx
PROLOG_PUSH esi
PROLOG_PUSH edi

// push ArgumentRegisters
push ecx
push edx
PROLOG_PUSH ecx
PROLOG_PUSH edx

// set frame pointer
PROLOG_END
.endm

//
// FramedMethodFrame epilog
//
.macro STUB_EPILOG
// restore stack pointer
EPILOG_BEG

// pop ArgumentRegisters
pop edx
pop ecx
EPILOG_POP edx
EPILOG_POP ecx

// pop CalleeSavedRegisters
pop edi
pop esi
pop ebx
pop ebp
EPILOG_POP edi
EPILOG_POP esi
EPILOG_POP ebx

// pop ebp-frame
EPILOG_END
.endm

//
Expand Down Expand Up @@ -392,8 +399,14 @@ LEAF_END ArrayOpStubTypeMismatchException, _TEXT
// ------------------------------------------------------------------------------
// void STDCALL CallDescrWorkerInternal(CallDescrWorkerParams * pParams)
NESTED_ENTRY CallDescrWorkerInternal, _TEXT, NoHandler
PROLOG_BEG
PROLOG_PUSH eax
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to push EAX, EDX and ECX here. They are not expected to be preserved. I have taken a look at a C function compiled by both clang and gcc and they don't preserve these registers either (and use them)

Copy link
Author

@parjong parjong Dec 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

(As you already mentioned) I found that STDCALL requires only EBX to be preserved. I'll revise the code.

PROLOG_PUSH ebx
PROLOG_PUSH ecx
PROLOG_PUSH edx
PROLOG_END

mov ebx, [esp + 4] // pParams = esp + 4
mov ebx, [esp + ((2 + 4) * 4)]

// copy the stack
mov ecx, [ebx +CallDescrData__numStackSlots]
Expand Down Expand Up @@ -445,6 +458,12 @@ LOCAL_LABEL(ReturnsInt):
mov [ebx + CallDescrData__returnValue + 4], edx

LOCAL_LABEL(Epilog):
EPILOG_BEG
EPILOG_POP edx
EPILOG_POP ecx
EPILOG_POP ebx
EPILOG_POP eax
EPILOG_END
ret 4

LOCAL_LABEL(ReturnsFloat):
Expand Down Expand Up @@ -944,23 +963,28 @@ NESTED_ENTRY VirtualMethodFixupStub, _TEXT, NoHandler
sub eax, 5

// Push ebp frame to get good callstack under debugger
push ebp
mov ebp, esp
PROLOG_BEG

// Preserve argument registers
push ecx
push edx
PROLOG_PUSH ecx
PROLOG_PUSH edx

// Set frame pointer
PROLOG_END

push eax // address of the thunk
push ecx // this ptr
call C_FUNC(VirtualMethodFixupWorker)

// Restore stack pointer
EPILOG_BEG

// Restore argument registers
pop edx
pop ecx
EPILOG_POP edx
EPILOG_POP ecx

// Pop ebp frame
pop ebp
EPILOG_END

PATCH_LABEL VirtualMethodFixupPatchLabel
// Proceed to execute the actual method.
Expand Down