Skip to content

Commit

Permalink
fix symbols; fix canonicalization of int64, -0
Browse files Browse the repository at this point in the history
  • Loading branch information
@MikeHolman
MikeHolman committed Mar 15, 2018
1 parent 24d0906 commit 743368f
Show file tree
Hide file tree
Showing 7 changed files with 91 additions and 39 deletions.
11 changes: 7 additions & 4 deletions lib/Runtime/Language/JavascriptConversion.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1522,15 +1522,21 @@ namespace Js
return NumberUtilities::TryToInt64(length);
}

// Lossy conversion means values are StrictEqual equivalent,
// but we cannot reconstruct the original value after canonicalization
// (e.g. -0 or an Int64Number object)
template <bool acceptLossyConversion>
Var JavascriptConversion::TryCanonicalizeAsSimpleVar(Var value)
{
TypeId typeId = JavascriptOperators::GetTypeId(value);
switch (typeId)
{
case TypeIds_Integer:
case TypeIds_Number:
case TypeIds_Int64Number:
case TypeIds_UInt64Number:
{
Var taggedInt = TryCanonicalizeAsTaggedInt<true>(value);
Var taggedInt = TryCanonicalizeAsTaggedInt<true, acceptLossyConversion>(value, typeId);
if (taggedInt)
{
return taggedInt;
Expand All @@ -1544,9 +1550,6 @@ namespace Js
}
case TypeIds_String:
case TypeIds_Symbol:
// TODO: try to canonicalize Int64/Uint64 to TaggedInt
case TypeIds_Int64Number:
case TypeIds_UInt64Number:
return nullptr;

default:
Expand Down
8 changes: 7 additions & 1 deletion lib/Runtime/Language/JavascriptConversion.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,17 @@ namespace Js {
static double LongToDouble(__int64 aValue);
static double ULongToDouble(unsigned __int64 aValue);

template <bool allowNegOne>
template <bool allowNegOne, bool acceptLossyConversion>
static Var TryCanonicalizeAsTaggedInt(Var value);
template <bool allowNegOne, bool acceptLossyConversion>
static Var TryCanonicalizeAsTaggedInt(Var value, TypeId typeId);
template <bool acceptLossyConversion>
static Var TryCanonicalizeAsSimpleVar(Var value);

private:
template <typename T, bool allowNegOne>
static Var TryCanonicalizeIntHelper(T val);

static BOOL ToInt32Finite(double value, int32* result);
template<bool zero>
static bool SameValueCommon(Var aValue, Var bValue);
Expand Down
69 changes: 53 additions & 16 deletions lib/Runtime/Language/JavascriptConversion.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,40 +203,77 @@ namespace Js {
return SameValueCommon<true>(aValue, bValue);
}

template <bool allowNegOne>
inline Var JavascriptConversion::TryCanonicalizeAsTaggedInt(Var value)
template <typename T, bool allowNegOne>
static Var JavascriptConversion::TryCanonicalizeIntHelper(T val)
{
if (TaggedInt::Is(value))
if (TaggedInt::IsOverflow(val))
{
return (allowNegOne || value != TaggedInt::ToVarUnchecked(-1))
? value
: nullptr;
return nullptr;
}

if (!JavascriptNumber::Is(value))
if (!allowNegOne && val == -1)
{
return nullptr;
}

double doubleVal = JavascriptNumber::GetValue(value);
int32 intVal = 0;
CompileAssert(sizeof(TaggedInt::k_nMinValue) <= sizeof(int));
CompileAssert(sizeof(TaggedInt::k_nMaxValue) <= sizeof(int));

return TaggedInt::ToVarUnchecked((int)val);
}

if (!JavascriptNumber::TryGetInt32Value<true>(doubleVal, &intVal))
template <bool allowNegOne, bool allowLossyConversion>
inline Var JavascriptConversion::TryCanonicalizeAsTaggedInt(Var value, TypeId typeId)
{
switch (typeId)
{
return nullptr;
}
case TypeIds_Integer:
return (allowNegOne || value != TaggedInt::ToVarUnchecked(-1))
? value
: nullptr;

if (TaggedInt::IsOverflow(intVal))
case TypeIds_Number:
{
return nullptr;
double doubleVal = JavascriptNumber::GetValue(value);
int32 intVal = 0;

if (!JavascriptNumber::TryGetInt32Value<allowLossyConversion>(doubleVal, &intVal))
{
return nullptr;
}
return TryCanonicalizeIntHelper<int32, allowNegOne>(intVal);
}
case TypeIds_Int64Number:
{
if (!allowLossyConversion)
{
return nullptr;
}
int64 int64Val = JavascriptInt64Number::UnsafeFromVar(value)->GetValue();

return TryCanonicalizeIntHelper<int64, allowNegOne>(int64Val);

if (!allowNegOne && intVal == -1)
}
case TypeIds_UInt64Number:
{
if (!allowLossyConversion)
{
return nullptr;
}
uint64 uint64Val = JavascriptUInt64Number::UnsafeFromVar(value)->GetValue();

return TryCanonicalizeIntHelper<uint64, allowNegOne>(uint64Val);
}
default:
return nullptr;
}
}

return TaggedInt::ToVarUnchecked(intVal);
template <bool allowNegOne, bool allowLossyConversion>
inline Var JavascriptConversion::TryCanonicalizeAsTaggedInt(Var value)
{
TypeId typeId = JavascriptOperators::GetTypeId(value);
return TryCanonicalizeAsTaggedInt<allowNegOne, allowLossyConversion>(value, typeId);
}

} // namespace Js
12 changes: 6 additions & 6 deletions lib/Runtime/Library/JavascriptMap.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,13 +378,13 @@ JavascriptMap::DeleteFromSimpleVarMap(Var value)
{
Assert(this->kind == MapKind::SimpleVarMap);

Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(value);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<true /* allowLossyConversion */>(value);
if (!simpleVar)
{
return false;
}

return this->DeleteFromVarMap<false>(simpleVar);
return this->DeleteFromVarMap<false /* isComplex */>(simpleVar);
}

bool
Expand Down Expand Up @@ -427,7 +427,7 @@ JavascriptMap::Get(Var key, Var* value)
return true;
}
// If the key isn't in the map, check if the canonical value is
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(key);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<true /* allowLossyConversion */>(key);
// If the simple var is the same as the original key, we know it isn't in the map
if (!simpleVar || simpleVar == key)
{
Expand Down Expand Up @@ -476,7 +476,7 @@ JavascriptMap::Has(Var key)
return true;
}
// If the key isn't in the map, check if the canonical value is
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(key);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<true /* allowLossyConversion */>(key);
// If the simple var is the same as the original key, we know it isn't in the map
if (!simpleVar || simpleVar == key)
{
Expand Down Expand Up @@ -517,7 +517,7 @@ void
JavascriptMap::SetOnEmptyMap(Var key, Var value)
{
Assert(this->kind == MapKind::EmptyMap);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(key);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<false /* allowLossyConversion */>(key);
if (simpleVar)
{
SimpleVarDataMap* newSimpleMap = RecyclerNew(this->GetRecycler(), SimpleVarDataMap, this->GetRecycler());
Expand Down Expand Up @@ -548,7 +548,7 @@ JavascriptMap::TrySetOnSimpleVarMap(Var key, Var value)
{
Assert(this->kind == MapKind::SimpleVarMap);

Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(key);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<false /* allowLossyConversion */>(key);
if (!simpleVar)
{
return false;
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JavascriptMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace Js
// An EmptyMap is a map containing no elements
EmptyMap,
// A SimpleVarMap is a map containing only Vars which are comparable by pointer, and don't require
// pointer comparison
// value comparison
//
// Addition of a Var that is not comparable by pointer value causes the set to be promoted to a ComplexVarSet
SimpleVarMap,
Expand Down
20 changes: 10 additions & 10 deletions lib/Runtime/Library/JavascriptSet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ JavascriptSet::AddToEmptySet(Var value)
{
Assert(this->kind == SetKind::EmptySet);
// We cannot store an int with value -1 inside of a bit vector
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false>(value);
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false /* allowNegOne */, false /* allowLossyConversion */>(value);
if (taggedInt)
{
int32 intVal = TaggedInt::ToInt32(taggedInt);
Expand All @@ -343,7 +343,7 @@ JavascriptSet::AddToEmptySet(Var value)
return;
}

Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(value);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<false /* allowLossyConversion */>(value);
if (simpleVar)
{
SimpleVarDataSet* newSimpleSet = RecyclerNew(this->GetRecycler(), SimpleVarDataSet, this->GetRecycler());
Expand All @@ -369,7 +369,7 @@ bool
JavascriptSet::TryAddToIntSet(Var value)
{
Assert(this->kind == SetKind::IntSet);
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false>(value);
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false /* allowNegOne */, false /* allowLossyConversion */>(value);
if (!taggedInt)
{
return false;
Expand All @@ -387,7 +387,7 @@ bool
JavascriptSet::TryAddToSimpleVarSet(Var value)
{
Assert(this->kind == SetKind::SimpleVarSet);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(value);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<false /* allowLossyConversion */>(value);
if (!simpleVar)
{
return false;
Expand Down Expand Up @@ -432,7 +432,7 @@ JavascriptSet::Add(Var value)
return;
}

Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(value);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<false /* allowLossyConversion */>(value);
if (simpleVar)
{
this->PromoteToSimpleVarSet();
Expand Down Expand Up @@ -490,7 +490,7 @@ bool
JavascriptSet::IsInIntSet(Var value)
{
Assert(this->kind == SetKind::IntSet);
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false>(value);
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false /* allowNegOne */, true /* allowLossyConversion */>(value);
if (!taggedInt)
{
return false;
Expand Down Expand Up @@ -520,7 +520,7 @@ bool
JavascriptSet::DeleteFromSimpleVarSet(Var value)
{
Assert(this->kind == SetKind::SimpleVarSet);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(value);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<true /* allowLossyConversion */>(value);
if (!simpleVar)
{
return false;
Expand Down Expand Up @@ -553,7 +553,7 @@ bool JavascriptSet::Delete(Var value)
return this->DeleteFromSimpleVarSet(value);

case SetKind::ComplexVarSet:
return this->DeleteFromVarSet<true>(value);
return this->DeleteFromVarSet<true /* isComplex */>(value);

default:
Assume(UNREACHED);
Expand All @@ -571,7 +571,7 @@ bool JavascriptSet::Has(Var value)

case SetKind::IntSet:
{
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false>(value);
Var taggedInt = JavascriptConversion::TryCanonicalizeAsTaggedInt<false /* allowNegOne */, true /* allowLossyConversion */ >(value);
if (!taggedInt)
{
return false;
Expand All @@ -588,7 +588,7 @@ bool JavascriptSet::Has(Var value)
return true;
}
// If the value isn't in the set, check if the canonical value is
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar(value);
Var simpleVar = JavascriptConversion::TryCanonicalizeAsSimpleVar<true /* allowLossyConversion */>(value);
// If the simple value is the same as the original value, we know it isn't in the set
if (!simpleVar || simpleVar == value)
{
Expand Down
8 changes: 7 additions & 1 deletion lib/Runtime/Library/SameValueComparer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,16 @@ namespace Js

case TypeIds_String:
{
JavascriptString* v = JavascriptString::FromVar(i);
JavascriptString* v = JavascriptString::UnsafeFromVar(i);
return JsUtil::CharacterBuffer<WCHAR>::StaticGetHashCode(v->GetString(), v->GetLength());
}

case TypeIds_Symbol:
{
JavascriptSymbol* sym = JavascriptSymbol::UnsafeFromVar(i);
return sym->GetValue()->GetHashCode();
}

default:
return RecyclerPointerComparer<Var>::GetHashCode(i);
}
Expand Down

0 comments on commit 743368f

Please sign in to comment.