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

Broken float constant in native code, ppc64 #6963

Closed
vicuna opened this issue Aug 18, 2015 · 8 comments
Closed

Broken float constant in native code, ppc64 #6963

vicuna opened this issue Aug 18, 2015 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Aug 18, 2015

Original bug ID: 6963
Reporter: @mjambon
Status: closed (set by @xavierleroy on 2015-08-20T09:53:38Z)
Resolution: not a bug
Priority: normal
Severity: minor
Platform: ppc64
OS: Linux
Version: 4.02.0
Category: back end (clambda to assembly)
Monitored by: "Richard Jones"

Bug description

The last two bytes of a float constant are incorrectly interpreted (ocamlopt on ppc64). The runtime representation of the float 1.00146962706651288 should be 0x3ff0060504030201 but instead we get 0x3ff0060504033f5a.

open Printf

let read_bits_from_constant () =
  let expected = "3ff0060504030201" in
  let result =
    sprintf "%0Lx"
      (Int64.bits_of_float 1.00146962706651288)
  in
  printf "read_bits_from_constant: %s %s\n"
    result (if result = expected then "OK" else "ERROR")

prints:

read_bits_from_constant: 3ff0060504033f5a ERROR

See conversation at ocaml-community/biniou#13 (comment) and attached file.

The bug was originally found in the biniou library by Rafael Fonseca with the help of Richard W Jones.

Steps to reproduce

See attached file for full program.

File attachments

@vicuna
Copy link
Author

vicuna commented Aug 18, 2015

Comment author: rdossant

The ocaml version I tested against is actually 4.02.0. I tested on both PPC64 big endian and little endian.

@vicuna
Copy link
Author

vicuna commented Aug 18, 2015

Comment author: Richard Jones

This could be a bug in our downstream ppc64 backend ( https://git.fedorahosted.org/cgit/fedora-ocaml.git/tree/asmcomp/power64?h=fedora-24-4.02 )

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: @xavierleroy

As Richard wrote, PPC64 is not supported by "upstream" OCaml 4.02.0, so the reporter must be using RedHat's "downstream" back-end.

An "upstream" PPC64 port is in progress (see #225). For what it's worth, the test case works fine with this port.

Did you run the test using the bytecode compiler? There is a very small chance that the problem is in the strtod() C library function, in which case the problem would manifest both in bytecode and in native-code.

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: @xavierleroy

I see from the "biniou" discussion that the problem doesn't occur in bytecode. So, the problem seems to come from the "downstream" PPC64 native-code generator.

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: Richard Jones

Hi Xavier, as you say I am also sure this is a bug in our downstream backend and not in OCaml. Still studying it on my local VM.

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: Richard Jones

[Bleah, Mantis ate the long comment I wrote here.]

There's obviously a bug in the downstream ppc64 & ppc64le code generators. They currently generate this assembler:

%%
camlFloat_bits__6:
.double 0d1.00146962707
%%

were the literal is obviously truncated.

The patch to fix this is something like:

%%
diff --git a/asmcomp/power64/emit.mlp b/asmcomp/power64/emit.mlp
index 4e0317a..de6a525 100644
--- a/asmcomp/power64/emit.mlp
+++ b/asmcomp/power64/emit.mlp
@@ -906,11 +906,9 @@ let emit_item = function
| Cint n ->
.quad {emit_nativeint n}\n
| Csingle f ->

  •  let s = string_of_float f in
    
  •  `        .float  0d{emit_string s}\n`
    
  •  emit_float32_directive ".long" (Int32.bits_of_float f)
    
    | Cdouble f ->
  •  let s = string_of_float f in
    
  •  `        .double 0d{emit_string s}\n`
    
  •  emit_float64_directive ".quad" (Int64.bits_of_float f)
    
    | Csymbol_address s ->
    .quad {emit_symbol s}\n
    | Clabel_address lbl ->
    %%

(similarly for the other backend).

I'm still testing the above.

Very keen to switch to the new combined-ppc backend as soon as possible so we don't have to maintain our own!

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: Richard Jones

Red Hat BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1255135

@vicuna
Copy link
Author

vicuna commented Aug 19, 2015

Comment author: Richard Jones

The attached patch fixes the issue in Red Hat's ppc64be/le backends. Since this doesn't apply to the upstream OCaml, you can just close this bug.

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

No branches or pull requests

1 participant