[v8-dev] Fix bug in object literals with large array indexes as strings. (issue6410028)

Wed, 02 Feb 2011 05:43:07 -0800 

Reviewers: Mads Ager,

Description:
Fix bug in object literals with large array indexes as strings.

Please review this at http://codereview.chromium.org/6410028/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge/build-ia32

Affected files:
  M src/ast.cc
  M src/hashmap.h
  M src/parser.cc
  M test/mjsunit/compiler/literals.js


Index: src/ast.cc
diff --git a/src/ast.cc b/src/ast.cc
index 80927a88fd06f064d9ee52dbc5f544d67e1fdd70..ccfa2b4ecf50a49464d4708573f9958997679e0c 100644 
--- a/src/ast.cc
+++ b/src/ast.cc
@@ -215,17 +215,28 @@ bool IsEqualString(void* first, void* second) {
   return (*h1)->Equals(*h2);
 }

-bool IsEqualSmi(void* first, void* second) {
-  ASSERT((*reinterpret_cast<Smi**>(first))->IsSmi());
-  ASSERT((*reinterpret_cast<Smi**>(second))->IsSmi());
-  Handle<Smi> h1(reinterpret_cast<Smi**>(first));
-  Handle<Smi> h2(reinterpret_cast<Smi**>(second));
-  return (*h1)->value() == (*h2)->value();
+
+bool IsEqualNumber(void* first, void* second) {
+  ASSERT((*reinterpret_cast<Object**>(first))->IsNumber());
+  ASSERT((*reinterpret_cast<Object**>(second))->IsNumber());
+
+  Handle<Object> h1(reinterpret_cast<Object**>(first));
+  Handle<Object> h2(reinterpret_cast<Object**>(second));
+  if (h1->IsSmi()) {
+    return h2->IsSmi() && *h1 == *h2;
+  }
+  if (h2->IsSmi()) return false;
+  Handle<HeapNumber> n1 = Handle<HeapNumber>::cast(h1);
+  Handle<HeapNumber> n2 = Handle<HeapNumber>::cast(h2);
+  ASSERT(isfinite(n1->value()));
+  ASSERT(isfinite(n2->value()));
+  return n1->value() == n2->value();
 }

+
 void ObjectLiteral::CalculateEmitStore() {
   HashMap properties(&IsEqualString);
-  HashMap elements(&IsEqualSmi);
+  HashMap elements(&IsEqualNumber);
   for (int i = this->properties()->length() - 1; i >= 0; i--) {
     ObjectLiteral::Property* property = this->properties()->at(i);
     Literal* literal = property->key();
@@ -238,23 +249,19 @@ void ObjectLiteral::CalculateEmitStore() {
     uint32_t hash;
     HashMap* table;
     void* key;
-    uint32_t index;
-    Smi* smi_key_location;
     if (handle->IsSymbol()) {
       Handle<String> name(String::cast(*handle));
-      if (name->AsArrayIndex(&index)) {
-        smi_key_location = Smi::FromInt(index);
-        key = &smi_key_location;
-        hash = index;
+      if (name->AsArrayIndex(&hash)) {
+        Handle<Object> key_handle = Factory::NewNumberFromUint(hash);
+        key = key_handle.location();
         table = &elements;
       } else {
         key = name.location();
         hash = name->Hash();
         table = &properties;
       }
-    } else if (handle->ToArrayIndex(&index)) {
+    } else if (handle->ToArrayIndex(&hash)) {
       key = handle.location();
-      hash = index;
       table = &elements;
     } else {
       ASSERT(handle->IsNumber());
Index: src/hashmap.h
diff --git a/src/hashmap.h b/src/hashmap.h
index 3b947beb5388673f9d2e7c6cfe6b226db807e351..27989889c3e665a6806a61557d6f6eb13f4b011e 100644 
--- a/src/hashmap.h
+++ b/src/hashmap.h
@@ -49,7 +49,8 @@ class HashMap {
   typedef bool (*MatchFun) (void* key1, void* key2);

   // Dummy constructor.  This constructor doesn't set up the hash
-  // map properly so don't use it unless you have good reason.
+  // map properly so don't use it unless you have good reason (e.g.,
+  // you know that the HashMap will never be used).
   HashMap();

   // initial_capacity is the size of the initial hash map;
Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index c0976988ea3c077800842816c03981ee5cf7b248..ccb3f64e10dcb0ae9269b6b0d60f7bdc171dd078 100644 
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3035,7 +3035,7 @@ Handle<Object> Parser::GetBoilerplateValue(Expression* expression) { 

 // Defined in ast.cc
 bool IsEqualString(void* first, void* second);
-bool IsEqualSmi(void* first, void* second);
+bool IsEqualNumber(void* first, void* second);


 // Validation per 11.1.5 Object Initialiser
@@ -3043,7 +3043,7 @@ class ObjectLiteralPropertyChecker {
  public:
   ObjectLiteralPropertyChecker(Parser* parser, bool strict) :
     props(&IsEqualString),
-    elems(&IsEqualSmi),
+    elems(&IsEqualNumber),
     parser_(parser),
     strict_(strict) {
   }
@@ -3092,13 +3092,12 @@ void ObjectLiteralPropertyChecker::CheckProperty(
   uint32_t hash;
   HashMap* map;
   void* key;
-  Smi* smi_key_location;

   if (handle->IsSymbol()) {
     Handle<String> name(String::cast(*handle));
     if (name->AsArrayIndex(&hash)) {
-      smi_key_location = Smi::FromInt(hash);
-      key = &smi_key_location;
+      Handle<Object> key_handle = Factory::NewNumberFromUint(hash);
+      key = key_handle.location();
       map = &elems;
     } else {
       key = handle.location();
Index: test/mjsunit/compiler/literals.js
diff --git a/test/mjsunit/compiler/literals.js b/test/mjsunit/compiler/literals.js index d846cf5b781dfd2db8cda4e694edf68cdbbfbd5c..e910bb3c6a1e71a897aba36f806e855866b7585d 100644 
--- a/test/mjsunit/compiler/literals.js
+++ b/test/mjsunit/compiler/literals.js
@@ -88,3 +88,6 @@ assertEquals(17, eval('[1,2,3,4]; 17'));
 assertEquals(19, eval('var a=1, b=2; [a,b,3,4]; 19'));
 assertEquals(23, eval('var a=1, b=2; c=23; [a,b,3,4]; c'));

+// Test that literals work for non-smi indices.
+// Ensure hash-map collision if using value as hash.
+var o = {"2345678901" : 42, "2345678901" : 30};


--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to