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

ocamlc: more robust implementation of Misc.no_overflow_mul #1520

Merged
merged 3 commits into from Dec 20, 2017

Conversation

Projects
None yet
4 participants
@murmour
Copy link
Contributor

murmour commented Dec 9, 2017

The old version of Misc.no_overflow_mul fails on a = min_int, b = -1 and on b = 0, which may (or may not) be the cause of incorrect code transformations in Cmmgen. I suggest replacing this function with a more robust version, which is based on a similar function from Batteries.

@murmour

This comment has been minimized.

Copy link
Contributor Author

murmour commented Dec 9, 2017

Shall I add a test for this?
Also, I'm not sure if it deserves an entry in Changes.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 9, 2017

Yes, you should add tests for it, and yes, it deserves a Changes entry. Thanks!

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 10, 2017

Yes, you should add tests for it

I'm sorry to disagree but so far we don't use unit tests for compiler functions, we only do integration tests for the whole compiler (= check that some input program is correctly compiled), and I don't see how to test this function via integration tests.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 10, 2017

We have unit tests for some Misc functions in testsuite/tests/utils.

P.S.: I think it would be fairly reasonable to require that new functions in Misc or the standard library come with unit tests.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 10, 2017

This implementation looks like table 2.2 from chapter 2 of Hacker's Delight. This is good because it's almost certainly correct, like all of Hacker's Delight. But: please add a reference in a comment.

Also: a few lines above, Hacker's Delight proposes the following test for x * y multiplication overflow:

   let z = x * y in
   (y < 0 && x = min_int) || (y <> 0 && z / y <> x)

To me this looks simpler than your code.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 10, 2017

I have mixed feelings about unit tests. They take a lot of time to write and rarely find bugs. Committing ourselves to writing unit tests for (parts of) the compiler functions is attaching a ball and chain to our ankles.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Dec 11, 2017

I must say, I share similar feelings to @xavierleroy 's about unit tests, although I do think they have clear value at times. I would have thought this was such a case.

murmour added some commits Dec 8, 2017

ocamlc: more robust implementation of Misc.no_overflow_mul
The old one fails on [a = min_int, b = -1] and [b = 0].

@murmour murmour force-pushed the murmour:Misc.no_overflow_mul-fix branch from 6c01a3b to fd5084f Dec 13, 2017

@murmour

This comment has been minimized.

Copy link
Contributor Author

murmour commented Dec 14, 2017

@xavierleroy

This implementation looks like table 2.2 from chapter 2 of Hacker's Delight. This is good because it's almost certainly correct, like all of Hacker's Delight. But: please add a reference in a comment.

Also: a few lines above, Hacker's Delight proposes the following test for x * y multiplication overflow:

   let z = x * y in
   (y < 0 && x = min_int) || (y <> 0 && z / y <> x)

To me this looks simpler than your code.

The proposed test is from CERT C Coding Standard, and is notable for avoiding an overflow while checking for overflow. However, since overflows are perfectly defined in OCaml (palmface), we have the luxury of relying on a particular behaviour; so I've updated the patch with the shorter test, which looks simpler to me as well.

I've added the Changes entry.

As for extending the testsuite, I share the sentiment that unit testing is rarely of any value, especially so in compilers. My rule of thumb is: the only things that are worth testing are those things that can break, and I don't see this trivial one-line function breaking (now that I've audited and tested it carefully).

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 14, 2017

My idea was that if you keep track, in the testsuite, of the sanity checks that you performed on your new definition, then later if you decide to change the implementation you can quickly check that it still passes them. (For example this could have saved you time when you decided to simplify the implementation.)

It also serves as a way to store the knowledge of which edge cases have been considered already and of the fact that the function's behavior in this case is intentional.

@murmour

This comment has been minimized.

Copy link
Contributor Author

murmour commented Dec 14, 2017

It also serves as a way to store the knowledge of which edge cases have been considered already and of the fact that the function's behavior in this case is intentional.

All of it is encoded in the source of the formula that is pointed in the comment (Hacker's Delight, chapter "Overflow Detection").

But of course we can be more explicit, yes. It's not much of a hassle. Will add tests a bit later.

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

LGTM

@murmour

This comment has been minimized.

Copy link
Contributor Author

murmour commented Dec 17, 2017

A test is added. It also covers Misc.no_overflow_add and Misc.no_overflow_sub.

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Looks good to me (again). Merging!

@xavierleroy xavierleroy merged commit 7c6b2ed into ocaml:trunk Dec 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 20, 2017

This is a very minor comment but when you use "Squash and merge", the weird stuff that is proposed in the awkwardly small textarea is actually the commit message -- the default value is a concatenation of the messages of each commit. It is often appropriate to delete or reformulate the lines corresponding to the minor commits that you decided to squash.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Dec 20, 2017

Yes, that is my understanding too, and I often edit the small textarea intensively. Here I edited lightly because the default commit message looked pretty good already. Don't you agree?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 20, 2017

I think it's okay, I just wasn't sure you were aware of the feature -- many people just ignore the textarea. Sorry for the noise.

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.