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

%r11 clobbered by Lswitch in Windows AMD64 native-code compilation #5319

Closed
vicuna opened this Issue Jul 22, 2011 · 4 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Jul 22, 2011

Original bug ID: 5319
Reporter: @alainfrisch
Assigned to: @xavierleroy
Status: closed (set by @xavierleroy on 2012-09-25T18:07:21Z)
Resolution: fixed
Priority: normal
Severity: crash
Fixed in version: 3.12.1+dev
Category: ~DO NOT USE (was: OCaml general)
Related to: #4424

Bug description

This issue is the little sister of #4424. We've encountered a case where the argument of Lswitch happens to be in %r11, and emit_nt.mlp does not like it.

Since the fix for #4424, the support for Lswitch has changed in the Unix case. I don't know if one should use the fix for #4424:

http://caml.inria.fr/cgi-bin/viewcvs.cgi/ocaml/version/3.10/asmcomp/amd64/emit.mlp?rev=8439&r1=8425&r2=8439

or rather the solution implemented in the latest change for Lswitch:

http://caml.inria.fr/cgi-bin/viewcvs.cgi/ocaml/version/3.11/asmcomp/amd64/emit.mlp?rev=9342&r1=9333&r2=9342

(and related change in proc.ml)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 22, 2011

Comment author: @alainfrisch

Suggested fix:

--- ocaml/asmcomp/amd64/emit_nt.mlp (revision 36936)
+++ ocaml/asmcomp/amd64/emit_nt.mlp (working copy)
@@ -600,8 +600,17 @@
| Lswitch jumptbl ->
let lbl = new_label() in
if !pic_code then begin

  •      if i.arg.(0).loc = Reg 9 then begin
    
  •        `  sal     r11, 3\n`;
    
  •        `  push    r11\n`;
    
  •        `  lea     r11, {emit_label lbl}\n`;
    
  •        `  add     r11, QWORD PTR [rsp]\n`;
    
  •        `  add     rsp, 8\n`;
    
  •        `  jmp     QWORD PTR [r11]\n`;
    
  •      end else begin
         `    lea     r11, {emit_label lbl}\n`;
         `    jmp     QWORD PTR [r11+{emit_reg i.arg.(0)}*8]\n`
    
  •      end
       end else begin
         `    jmp     QWORD PTR [{emit_reg i.arg.(0)}*8 + {emit_label lbl}]\n`
       end;
    
@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 23, 2011

Comment author: @xavierleroy

Well spotted. I would prefer to use the same fix as in emit.mlp/proc.ml (namely, two temp regs that are destroyed across Lswitch) than to use your suggested fix, for two reasons: 1- trying to keep the two emitters in sync, and 2- avoiding pushes and pops if at all possible. I can do the backporting of the fix; can you do the testing?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 23, 2011

Comment author: @alainfrisch

Of course we can do some testing! But if one changes the register selection, the bug would probably not appear at the same place anyway, so it will be difficult to draw any conclusion. (We have a large code base compiled and running with Win64, and the bug only appeared once after several months.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Sep 18, 2011

Comment author: @xavierleroy

Tentative fix committed in 3.12 branch and in trunk. Still not tested yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.