Skip to content
This repository has been archived by the owner on May 21, 2019. It is now read-only.

Remove UB from int-to-float conversion for INT_MIN. #24

Conversation

pnkfelix
Copy link
Member

The fixed->float conversion intrinsics were carefully written to handle the case where the input is INT_MIN; e.g. avoiding sign extension via an intermediate cast to an unsigned int.

Unfortunately it wasn't quite careful enough; I am seeing evidence in rust-lang/rust#36518 that one cannot rely on -a to behave in the same manner as ~(unsigned)a+1, because it is simply undefined behavior.

Anyway, the fix I've chosen is simple enough: cast to unsigned int earlier in the computation and use ~a + 1 to do the negation.

@pnkfelix
Copy link
Member Author

Oh wait, during more review, it looks like the code in https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/floatsitf.c is doing the right thing. So it would probably be better form for me to adapt that code rather than introducing my own variant..

@alexcrichton
Copy link
Member

Oh if this is fixed upstream maybe we should just update to upstream?

@pnkfelix
Copy link
Member Author

@alexcrichton Sorry, my previous commit was super unclear.

Its not fixed upstream.

floatsitf.c handles converting single-word fixed-point to quad-word floating-point

It is a more recent file and I guess its author took care to avoid the UB here.

In any case, I figured the best thing for me to do would be to adopt the coding style used by the author of floatsitf.c, because I figure that has a much better chance of being accepted upstream, since I can point to the precedent.

(the end effect in terms of code execution is the same as what I wrote; its just a matter of taste in terms of whether it is easier to miss bugs where one writes a when they should have written aAbs in this new version).

This change was already incorporated into floatsitf.c and floatditf.c
(As far as I can tell they were correct when they were first commited;
perhaps their author did not realized this bug lay dormant in the
other routines.)
@pnkfelix
Copy link
Member Author

(In case its unclear: commit b2676d8 is the one that has adapted the style used in floatsitf.c, so it can be reviewed and hopefully merged.)

@alexcrichton
Copy link
Member

Ah ok, sounds good to me!

@alexcrichton alexcrichton merged commit f03ba5a into rust-lang:rust-llvm-2016-07-18 Sep 28, 2016
TimNN pushed a commit that referenced this pull request Apr 23, 2017
Remove UB from int-to-float conversion for INT_MIN.
TimNN added a commit that referenced this pull request Jul 20, 2017
Remove UB from int-to-float conversion for INT_MIN.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants