Skip to content

Commit

Permalink
FIX: preserve callee-saved registers in callbacks
Browse files Browse the repository at this point in the history 
EBX, EDI, and ESI are callee-saved registers (for both, stdcall and
cdecl), i.e. their content must be preserved across subroutine calls.

For callbacks to work properly, they must adhere to the calling
convention of the function they are called from. As Red/System's code
generator uses callee-saved registers quite heavily, callbacks must save
& restore those registers properly.

Fixes issue #114.
  • Loading branch information
@earl
earl committed Jun 27, 2011
1 parent 0eb5040 commit 89f2533
Showing 1 changed file with 14 additions and 2 deletions.
16 changes: 14 additions & 2 deletions red-system/targets/IA32.r
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ make target-class [
res
]

emit-prolog: func [name [word!] locals [block!] args-size [integer!]][
emit-prolog: func [name [word!] locals [block!] args-size [integer!] /local fspec][
if verbose >= 3 [print [">>>building:" uppercase mold to-word name "prolog"]]

emit #{55} ;-- PUSH ebp
Expand All @@ -817,11 +817,23 @@ make target-class [
emit #{83EC} ;-- SUB esp, args-size
emit to-char align-to args-size 4
]
fspec: select compiler/functions name
if all [block? fspec/4/1 find fspec/4/1 'callback] [
emit #{53} ;-- PUSH ebx
emit #{56} ;-- PUSH esi
emit #{57} ;-- PUSH edi
]
]

emit-epilog: func [name [word!] locals [block!] locals-size [integer!]][
emit-epilog: func [name [word!] locals [block!] locals-size [integer!] /local fspec][
if verbose >= 3 [print [">>>building:" uppercase mold to-word name "epilog"]]

fspec: select compiler/functions name
if all [block? fspec/4/1 find fspec/4/1 'callback] [
emit #{5F} ;-- POP edi
emit #{5E} ;-- POP esi
emit #{5B} ;-- POP ebx
]
emit #{C9} ;-- LEAVE
either any [
zero? locals-size
Expand Down

3 comments on commit 89f2533

@dockimbel
Copy link
Member

Choose a reason for hiding this comment

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

Andreas, this addition should not be required as the IA32 emitter takes great care to use registers only locally for each emitted atomic action (especially to avoid registers saving as much as possible). Only EAX needs to be preserved, as it carries the last value from one atomic action to the next one.

@dockimbel
Copy link
Member

Choose a reason for hiding this comment

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

I took the wrong point of view, it is obvious now that your patch is required to protect caller's registers from changes operated by Red/System.
So ignore my previous comment.

@earl
Copy link
Contributor Author

@earl earl commented on 89f2533 Jun 27, 2011

Choose a reason for hiding this comment

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

A (cdecl/stdcall) C caller calling a Red callback can, per the calling convention, expect the values in EBX/ESI/EDI to be unchanged across the callback call. Red's IA32 emitter uses EBX/ESI heavily as scratch registers, so Red-generated code executed in the callback must take caution to preserve the values of EBX/ESI (and EDI) across the callback's lifetime.

Assume the following call hierarchy:

  1. external ("C") function (zmq_msg_close), calls
  2. Red/System callback function (zmq-free), calls
  3. Red/System native function (print)

After 2. returns to 1., the values of EBX/ESI/EDI must be the same as they where when 1. first called into 2. However, the code generated by Red for 2. and especially 3. more often than not overwrites EBX/ESI/EDI. So the prolog/epilog generated for 1 must save/restore those registers.

Please sign in to comment.