Closed Bug 1438212 Opened 2 years ago Closed 2 years ago

UBSan: value is outside the range of representable values of type 'float'

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: tsmith, Assigned: Waldo)

Details

(Keywords: csectype-undefined)

Attachments

(1 file)

This seems to be triggered after a few minutes with regular browsing when built with -fsanitize=float-cast-overflow

changeset: 403479:6d8f470b2579

/mfbt/FloatingPoint.cpp:16:38: runtime error: 1.79769e+308 is outside the range of representable values of type 'float'
    #0 0x46a3fa in mozilla::IsFloat32Representable(double) /mfbt/FloatingPoint.cpp:16:38
    #1 0x7f6c1139e475 in GuessPhiType /js/src/jit/IonAnalysis.cpp:1436:21
    #2 0x7f6c1139e475 in specializePhis /js/src/jit/IonAnalysis.cpp:1538
    #3 0x7f6c1139e475 in analyze /js/src/jit/IonAnalysis.cpp:2002
    #4 0x7f6c1139e475 in js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) /js/src/jit/IonAnalysis.cpp:2016
    #5 0x7f6c11397e6f in js::jit::OptimizeMIR(js::jit::MIRGenerator*) /js/src/jit/Ion.cpp:1560:14
    #6 0x7f6c113a31cf in js::jit::CompileBackEnd(js::jit::MIRGenerator*) /js/src/jit/Ion.cpp:1981:10
    #7 0x7f6c119c52e3 in js::HelperThread::handleIonWorkload(js::AutoLockHelperThreadState&) /js/src/vm/HelperThreads.cpp:1838:39
    #8 0x7f6c119c4929 in js::HelperThread::threadLoop() /js/src/vm/HelperThreads.cpp:2227:13
    #9 0x7f6c119ea265 in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start(void*) /js/src/threading/Thread.h:235:11
    #10 0x7f6c3072f7fb in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x77fb)
    #11 0x7f6c2f75db5e in clone /build/glibc-itYbWN/glibc-2.26/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Attached patch Patch with testsSplinter Review
Note that the existing code doesn't even do what the docs claim, because it mishandles NaN by dint of determining "representable" by an equality test.  Boooooooooooooooooooooooooooooooooooooooooooooooo!

http://eel.is/c++draft/conv.double and http://eel.is/c++draft/conv.fpprom for the relevant spec-text on how these conversions behave.
Attachment #8982977 - Flags: review?(nfroyd)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Basically every build lit up like a Christmas tree when I used is_trivially_copyable, FWIW.  (Some of the chromium code in our tree goes to heroics of toolchain-testing to emulate the trait, but try seems to say we can use the stricter trait that *does* seem to be everywhere, so that seems much better.)
Comment on attachment 8982977 [details] [diff] [review]
Patch with tests

Review of attachment 8982977 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the function uninlined...unless we're uninlining it so we can actually test it in TestFloatingPoint?

::: mfbt/FloatingPoint.cpp
@@ -10,5 @@
> -
> -namespace mozilla {
> -
> -bool
> -IsFloat32Representable(double aFloat32)

I don't understand why we'd uninline this; the new version is actually more code than the original (particularly in DEBUG builds), and I don't think representability checks are so common that we can't afford to have function calls?

::: mfbt/FloatingPoint.h
@@ +574,5 @@
> +  if (!IsFinite(aValue)) {
> +    return true;
> +  }
> +
> +  const double LargestFiniteFloat = 340282346638528859811704183484516925440.0f;

Surely this exists in <limits.h> or std::numeric_limits or something like that?

::: mfbt/tests/TestFloatingPoint.cpp
@@ +699,5 @@
> +
> +    A(IsFloat32Representable(widestPossible));
> +  }
> +
> +  

Nit: some extra whitespace at the end of the line, and extra lines generally.
Attachment #8982977 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3)
> r=me with the function uninlined...unless we're uninlining it so we can
> actually test it in TestFloatingPoint?

I did it this way to be consistent with NumberIsSignedInteger and NumberEqualsSignedInteger.  I don't expect that "inline" definitely always actually inlines, any more than I expect it there, and I figure the compiler/linker will do whatever's sensible.  Given that this function and those functions do similar amounts of stuff, similar treatment seems right to me.

> > +  const double LargestFiniteFloat = 340282346638528859811704183484516925440.0f;
> 
> Surely this exists in <limits.h> or std::numeric_limits or something like
> that?

In the original implementation I had (one involving bitwise math on IEEE-754 low levels), the exact value did somewhat matter, so it was useful to spell it out.  And I was trying to avoid std::numeric_limits<float>::max() because its constexpr-ness has a fair chance of not being dependable yet.

But this algorithm I ended up on doesn't depend on specific values, so I can use FLT_MAX here now.
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1521154cfec
Implement mozilla::IsFloat32Representable using an algorithm that handles NaN correctly and doesn't sometimes invoke undefined behavior.  r=froydnj
https://hg.mozilla.org/mozilla-central/rev/b1521154cfec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.