Skip to content

Commit

Permalink
Fix gcStableHashHermesValue() for NaN
Browse files Browse the repository at this point in the history
Summary: NaNs with different sign bits should get the same hash.

Reviewed By: neildhar

Differential Revision: D68584614

fbshipit-source-id: aa9fd7f0ee822f5a68241dcd4cbf15772c03d848
  • Loading branch information
lavenzg authored and facebook-github-bot committed Jan 24, 2025
1 parent 871de01 commit c908a05
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 5 deletions.
14 changes: 9 additions & 5 deletions lib/VM/Runtime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1974,13 +1974,17 @@ uint64_t Runtime::gcStableHashHermesValue(Handle<HermesValue> value) {
}
default:
assert(!value->isPointer() && "Unhandled pointer type");
if (value->isNumber() && value->getNumber() == 0) {
if (value->isNumber()) {
// We need to check for NaNs because they may differ in the sign bit,
// but they should have the same hash value.
if (LLVM_UNLIKELY(value->isNaN()))
return llvh::hash_value(HermesValue::encodeNaNValue().getRaw());
// To normalize -0 to 0.
return 0;
} else {
// For everything else, we just take advantage of HermesValue.
return llvh::hash_value(value->getRaw());
if (value->getNumber() == 0)
return 0;
}
// For everything else, we just take advantage of HermesValue.
return llvh::hash_value(value->getRaw());
}
}

Expand Down
32 changes: 32 additions & 0 deletions test/hermes/regress-hash-nan.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

// RUN: %hermes %s | %FileCheck --match-full-lines %s
// RUN: %shermes -exec %s | %FileCheck --match-full-lines %s

// Test that different NaN representations get the same hash.
globalThis.zero = 0;
var m = new Map([[NaN, 42]]);
var dynamicNaN = 0 / globalThis.zero;
print(m.get(dynamicNaN));
// CHECK: 42
print(m.has(dynamicNaN));
// CHECK: true
print(m.get(NaN));
// CHECK: 42
print(m.has(NaN));
// CHECK: true

m = new Map([[dynamicNaN, 24]]);
print(m.get(dynamicNaN));
// CHECK: 24
print(m.has(dynamicNaN));
// CHECK: true
print(m.get(NaN));
// CHECK: 24
print(m.has(NaN));
// CHECK: true

0 comments on commit c908a05

Please sign in to comment.