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

ocamlopt returns n for (n mod 1) instead of 0 #6749

Closed
vicuna opened this Issue Jan 13, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented Jan 13, 2015

Original bug ID: 6749
Reporter: ysk
Assigned to: @mshinwell
Status: closed (set by @xavierleroy on 2017-02-16T14:14:14Z)
Resolution: fixed
Priority: normal
Severity: minor
Platform: Darwin
OS: OS X
OS Version: 14.0
Version: 4.02.1
Fixed in version: 4.02.2+dev / +rc1
Category: middle end (typedtree to clambda)
Tags: patch
Related to: #6042 #6879
Monitored by: @gasche @yallop @hcarty

Bug description

The code is "let () = for i = 1 to 3 do Printf.printf "%d\n" (i mod 1) done".

The results of the code with ocamlopt are:
1
2
3

but it should be:
0
0
0

Additional information

I think that mod_int in asmcomp/cmmgen.ml may have a bug.

File attachments

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 13, 2015

Comment author: @gasche

Indeed this is a bug in cmmgen.ml:mod_int which believes that (c1 mod 1 = c1) instead of (c1 mod 1 = 0). This was introduced by commit trunk@14303 between 4.01 and 4.02 (3bb1612), and it seems to be a copy-pasting mistake from div_int's definition.

The whole function should be reviewed (it also seems to believe that (1 mod n = 1)), and it would also be important to understand why the testsuite that was added for #6042 did not catch this bug.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 13, 2015

Comment author: pcouderc

"List.iter (fun x -> Printf.printf "%d\n" (x mod 1)) [1; 2; 3]"

produces the same result, however unrolling it, i.e.

"Printf.printf "%d\n" (1 mod 1);
Printf.printf "%d\n" (2 mod 1);
Printf.printf "%d\n" (3 mod 1)"

returns a correct result.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 13, 2015

Comment author: @gasche

Indeed (nice catch!), this comes from the fact that (constant mod constant) operations are fully-evaluated earlier in the compilation pipeline, in closure.ml.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 14, 2015

Comment author: @mshinwell

Jeremie and I are looking at this

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jan 14, 2015

Comment author: @mshinwell

Proposed patch attached; comments welcome.

  1. The case for x mod 1 with x non-constant was wrong.
  2. We have added cases for x mod y when y is constant negative 1.
  3. Gabriel's first comment, second paragraph doesn't seem correct: 1 mod n does equal 1 so long as n is not 0 or 1, and those cases have already been ruled out at the corresponding point in the code.

We couldn't see anything else wrong.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Feb 8, 2015

Comment author: @gasche

Merged in trunk and 4.02, thanks!

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.