Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Buffer floating point write signaling NaNs #4648

Closed
trevnorris opened this issue Jan 23, 2013 · 8 comments
Closed

Buffer floating point write signaling NaNs #4648

trevnorris opened this issue Jan 23, 2013 · 8 comments

Comments

@trevnorris
Copy link

When Node is compiled for ia32 with clang 3.2, writeFloatLE|BE and writeDoubleLE|BE output signaling NaNs not quiet NaNs.

Investigating why this is happening.

@trevnorris
Copy link
Author

Small update: I think ->NumberValue() is returning a floating point signaling NaN. Excuse my gimp cc skills, but used the following to print out each character of the returned double value:

  double tmparg = args[0]->NumberValue();
  char* tmpchar = reinterpret_cast<char*>(&tmparg);

  return Number::New(tmpchar[7]);

The above returns 0x...00 f8 ff, while when compiled for x64 it returns 0x...00 f8 7f.

I'm going to start digging into the v8 source and see if I can identify what's going on.

@trevnorris
Copy link
Author

cc/ @bnoordhuis (any insights appreciated)

@trevnorris
Copy link
Author

Here is the full chain of execution v8 uses to evaluate an argument with NumberValue():

args[0]->NumberValue();

// from Value::NumberValue()
i::Handle<i::Object> num = i::Execution::ToNumber(...);
    RETURN_NATIVE_CALL(...)
      return Call(...);
        return Invoke(...);
          return Handle<Object>(value->ToObjectUnchecked(), isolate);

return num->Number();
  reinterpret_cast<HeapNumber*>(this)->value();

// from HeapNumber::value()
return READ_DOUBLE_FIELD(this, kValueOffset);
  #define READ_DOUBLE_FIELD(p, offset) \
    (*reinterpret_cast<double*>(FIELD_ADDR(p, offset)))
      #define FIELD_ADDR(p, offset) \
        (reinterpret_cast<byte*>(p) + offset - kHeapObjectTag)

// so the call laid out looks like so:
(*reinterpret_cast<double*>(reinterpret_cast<byte*>(this) + kValueOffset - kHeapObjectTag)

So the whole thing seems to boil down to a few reinterpret_cast calls to memory. Then I'd assume it has something to do with how the value is being stored in memory in the first place. But it's almost 3am so I'll backtrace that later.

@trevnorris
Copy link
Author

@bnoordhuis This issue also occurs in node's Typed Arrays implementation:

var buf = new Buffer(8);
var fview = new Float64Array(buf);
fview[0] = NaN;
// <Buffer 00 00 00 00 00 00 f8 ff>

And in d8's as well:

var buf = new ArrayBuffer(8);
var fview = new Float64Array(buf);
var uiview = new Uint8Array(buf);
uiview[6].toString(16) + ' ' + uiview[7].toString(16)
// f8 ff

So imho reverting back to the old way of writing buffer floats doesn't make a lot of sense in terms of the previous code complexity and performance hit, when all current implementations have the same problem.

@trevnorris
Copy link
Author

hopped into llvm irc and told me to file a bug: http://llvm.org/bugs/show_bug.cgi?id=15054

@bnoordhuis
Copy link
Member

Bummer, I guess you're right. Okay, I'll leave the changes in.

You might want to mention on that llvm bug that it appears to be some kind of weak interaction between clang and the system headers because the builtins return the right values.

@trevnorris
Copy link
Author

Will do. And thanks for figuring out what was going on. Would've taken me a week to see that.

@trevnorris
Copy link
Author

@bnoordhuis well that didn't take long.

Benjamin Kramer 2013-01-25 11:02:58 CST
Fixed in r173459.

I'll build and test it out, but close the bug for now.

And figured I'd throw in the diff for the fix:

     *losesInfo = lostFraction != lfExactlyZero || X86SpecialNan;
+
+    // For x87 extended precision, we want to make a NaN, not a special NaN if
+    // the input wasn't special either.
+    if (!X86SpecialNan && semantics == &APFloat::x87DoubleExtended)
+      APInt::tcSetBit(significandParts(), semantics->precision - 1);
+
     // gcc forces the Quiet bit on, which means (float)(double)(float_sNan)

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

No branches or pull requests

2 participants