Exclude the bit-field bits from string hash codes. String hash codes

are always odd if the bit-field bits are included.

Modified a couple of debugger tests that relied on the ordering of
elements in descriptor arrays.  Descriptor arrays are sorted by
hash-code values, so the order changes if we change the hash code.
Review URL: http://codereview.chromium.org/6489

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@440 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/heap.cc

@@ -1872,7 +1872,7 @@ Map* Heap::SymbolMapForString(String* string) {
 
 Object* Heap::AllocateSymbol(unibrow::CharacterStream* buffer,
                              int chars,
-                             int hash) {
+                             uint32_t length_field) {
   // Ensure the chars matches the number of characters in the buffer.
   ASSERT(static_cast<unsigned>(chars) == buffer->Length());
   // Determine whether the string is ascii.
@@ -1914,7 +1914,7 @@ Object* Heap::AllocateSymbol(unibrow::CharacterStream* buffer,
 
   reinterpret_cast<HeapObject*>(result)->set_map(map);
   // The hash value contains the length of the string.
-  String::cast(result)->set_length_field(hash);
+  String::cast(result)->set_length_field(length_field);
 
   ASSERT_EQ(size, String::cast(result)->Size());
 

src/heap.h

@@ -341,7 +341,7 @@ class Heap : public AllStatic {
   // Please note this function does not perform a garbage collection.
   static Object* AllocateSymbol(unibrow::CharacterStream* buffer,
                                 int chars,
-                                int hash);
+                                uint32_t length_field);
 
   // Allocates and partially initializes a String.  There are two String
   // encodings: ASCII and two byte.  These functions allocate a string of the

src/ic-arm.cc

@@ -94,6 +94,7 @@ static void GenerateDictionaryLoad(MacroAssembler* masm,
   for (int i = 0; i < kProbes; i++) {
     // Compute the masked index: (hash + i + i * i) & mask.
     __ ldr(t1, FieldMemOperand(r2, String::kLengthOffset));
+    __ mov(t1, Operand(t1, LSR, String::kHashShift));
     if (i > 0) __ add(t1, t1, Operand(Dictionary::GetProbeOffset(i)));
     __ and_(t1, t1, Operand(r3));
 

src/ic-ia32.cc

@@ -90,6 +90,7 @@ static void GenerateDictionaryLoad(MacroAssembler* masm, Label* miss_label,
   for (int i = 0; i < kProbes; i++) {
     // Compute the masked index: (hash + i + i * i) & mask.
     __ mov(r1, FieldOperand(name, String::kLengthOffset));
+    __ shr(r1, String::kHashShift);
     if (i > 0) __ add(Operand(r1), Immediate(Dictionary::GetProbeOffset(i)));
     __ and_(r1, Operand(r2));
 

src/objects-inl.h

@@ -484,6 +484,12 @@ Object* Object::GetProperty(String* key, PropertyAttributes* attributes) {
 #define WRITE_INT_FIELD(p, offset, value) \
   (*reinterpret_cast<int*>(FIELD_ADDR(p, offset)) = value)
 
+#define READ_UINT32_FIELD(p, offset) \
+  (*reinterpret_cast<uint32_t*>(FIELD_ADDR(p, offset)))
+
+#define WRITE_UINT32_FIELD(p, offset, value) \
+  (*reinterpret_cast<uint32_t*>(FIELD_ADDR(p, offset)) = value)
+
 #define READ_SHORT_FIELD(p, offset) \
   (*reinterpret_cast<uint16_t*>(FIELD_ADDR(p, offset)))
 
@@ -1204,13 +1210,13 @@ void String::set_length(int value) {
 }
 
 
-int String::length_field() {
-  return READ_INT_FIELD(this, kLengthOffset);
+uint32_t String::length_field() {
+  return READ_UINT32_FIELD(this, kLengthOffset);
 }
 
 
-void String::set_length_field(int value) {
-  WRITE_INT_FIELD(this, kLengthOffset, value);
+void String::set_length_field(uint32_t value) {
+  WRITE_UINT32_FIELD(this, kLengthOffset, value);
 }
 
 
@@ -2070,16 +2076,16 @@ bool String::HasHashCode() {
 
 uint32_t String::Hash() {
   // Fast case: has hash code already been computed?
-  int hash = length_field();
-  if (hash & kHashComputedMask) return hash;
+  uint32_t field = length_field();
+  if (field & kHashComputedMask) return field >> kHashShift;
   // Slow case: compute hash code and set it..
   return ComputeAndSetHash();
 }
 
 
 bool String::AsArrayIndex(uint32_t* index) {
-  int hash = length_field();
-  if ((hash & kHashComputedMask) && !(hash & kIsArrayIndexMask)) return false;
+  uint32_t field = length_field();
+  if ((field & kHashComputedMask) && !(field & kIsArrayIndexMask)) return false;
   return SlowAsArrayIndex(index);
 }
 

src/objects.cc

@@ -3773,14 +3773,14 @@ uint32_t String::ComputeAndSetHash() {
 
   // Compute the hash code.
   StringInputBuffer buffer(this);
-  int hash = ComputeHashCode(&buffer, length());
+  uint32_t field = ComputeLengthAndHashField(&buffer, length());
 
   // Store the hash code in the object.
-  set_length_field(hash);
+  set_length_field(field);
 
   // Check the hash code is there.
   ASSERT(length_field() & kHashComputedMask);
-  return hash;
+  return field >> kHashShift;
 }
 
 
@@ -3825,8 +3825,8 @@ static inline uint32_t HashField(uint32_t hash, bool is_array_index) {
 }
 
 
-uint32_t String::ComputeHashCode(unibrow::CharacterStream* buffer,
-                                 int length) {
+uint32_t String::ComputeLengthAndHashField(unibrow::CharacterStream* buffer,
+                                           int length) {
   // Large string (please note large strings cannot be an array index).
   if (length > kMaxMediumStringSize) return HashField(length, false);
 
@@ -5430,7 +5430,7 @@ class StringKey : public HashTableKey {
 class Utf8SymbolKey : public HashTableKey {
  public:
   explicit Utf8SymbolKey(Vector<const char> string)
-      : string_(string), hash_(0) { }
+      : string_(string), length_field_(0) { }
 
   bool IsMatch(Object* other) {
     if (!other->IsString()) return false;
@@ -5442,19 +5442,19 @@ class Utf8SymbolKey : public HashTableKey {
   }
 
   uint32_t Hash() {
-    if (hash_ != 0) return hash_;
+    if (length_field_ != 0) return length_field_ >> String::kHashShift;
     unibrow::Utf8InputBuffer<> buffer(string_.start(),
                                       static_cast<unsigned>(string_.length()));
     chars_ = buffer.Length();
-    hash_ = String::ComputeHashCode(&buffer, chars_);
-    return hash_;
+    length_field_ = String::ComputeLengthAndHashField(&buffer, chars_);
+    return length_field_ >> String::kHashShift;
   }
 
   Object* GetObject() {
-    if (hash_ == 0) Hash();
+    if (length_field_ == 0) Hash();
     unibrow::Utf8InputBuffer<> buffer(string_.start(),
                                       static_cast<unsigned>(string_.length()));
-    return Heap::AllocateSymbol(&buffer, chars_, hash_);
+    return Heap::AllocateSymbol(&buffer, chars_, length_field_);
   }
 
   static uint32_t StringHash(Object* obj) {
@@ -5464,7 +5464,7 @@ class Utf8SymbolKey : public HashTableKey {
   bool IsStringKey() { return true; }
 
   Vector<const char> string_;
-  uint32_t hash_;
+  uint32_t length_field_;
   int chars_;  // Caches the number of characters when computing the hash code.
 };
 
@@ -5503,7 +5503,9 @@ class SymbolKey : public HashTableKey {
     }
     // Otherwise allocate a new symbol.
     StringInputBuffer buffer(string_);
-    return Heap::AllocateSymbol(&buffer, string_->length(), string_->Hash());
+    return Heap::AllocateSymbol(&buffer,
+                                string_->length(),
+                                string_->length_field());
   }
 
   static uint32_t StringHash(Object* obj) {

src/objects.h

@@ -2857,8 +2857,8 @@ class String: public HeapObject {
   // that the length field is also used to cache the hash value of
   // strings.  In order to get or set the actual length of the string
   // use the length() and set_length methods.
-  inline int length_field();
-  inline void set_length_field(int value);
+  inline uint32_t length_field();
+  inline void set_length_field(uint32_t value);
 
   // Get and set individual two byte chars in the string.
   inline void Set(int index, uint16_t value);
@@ -2930,7 +2930,9 @@ class String: public HeapObject {
   // Returns a hash value used for the property table
   inline uint32_t Hash();
 
-  static uint32_t ComputeHashCode(unibrow::CharacterStream* buffer, int length);
+  static uint32_t ComputeLengthAndHashField(unibrow::CharacterStream* buffer,
+                                            int length);
+
   static bool ComputeArrayIndex(unibrow::CharacterStream* buffer,
                                 uint32_t* index,
                                 int length);
@@ -2981,11 +2983,6 @@ class String: public HeapObject {
   // Max ascii char code.
   static const int kMaxAsciiCharCode = 127;
 
-  // Shift constants for retriving length from length/hash field.
-  static const int kShortLengthShift = 3 * kBitsPerByte;
-  static const int kMediumLengthShift = 2 * kBitsPerByte;
-  static const int kLongLengthShift = 2;
-
   // Mask constant for checking if a string has a computed hash code
   // and if it is an array index.  The least significant bit indicates
   // whether a hash code has been computed.  If the hash code has been
@@ -2993,6 +2990,15 @@ class String: public HeapObject {
   // array index.
   static const int kHashComputedMask = 1;
   static const int kIsArrayIndexMask = 1 << 1;
+  static const int kNofLengthBitFields = 2;
+
+  // Shift constants for retriving length and hash code from
+  // length/hash field.
+  static const int kHashShift = kNofLengthBitFields;
+  static const int kShortLengthShift = 3 * kBitsPerByte;
+  static const int kMediumLengthShift = 2 * kBitsPerByte;
+  static const int kLongLengthShift = kHashShift;
+
 
   // Limit for truncation in short printing.
   static const int kMaxShortPrintLength = 1024;

src/stub-cache-arm.cc

@@ -103,8 +103,8 @@ void StubCache::GenerateProbe(MacroAssembler* masm,
   __ b(eq, &miss);
 
   // Get the map of the receiver and compute the hash.
-  __ ldr(scratch, FieldMemOperand(receiver, HeapObject::kMapOffset));
-  __ ldr(ip, FieldMemOperand(name, String::kLengthOffset));
+  __ ldr(scratch, FieldMemOperand(name, String::kLengthOffset));
+  __ ldr(ip, FieldMemOperand(receiver, HeapObject::kMapOffset));
   __ add(scratch, scratch, Operand(ip));
   __ eor(scratch, scratch, Operand(flags));
   __ and_(scratch,

src/stub-cache-ia32.cc

@@ -99,8 +99,8 @@ void StubCache::GenerateProbe(MacroAssembler* masm,
   __ j(zero, &miss, not_taken);
 
   // Get the map of the receiver and compute the hash.
-  __ mov(scratch, FieldOperand(receiver, HeapObject::kMapOffset));
-  __ add(scratch, FieldOperand(name, String::kLengthOffset));
+  __ mov(scratch, FieldOperand(name, String::kLengthOffset));
+  __ add(scratch, FieldOperand(receiver, HeapObject::kMapOffset));
   __ xor_(scratch, flags);
   __ and_(scratch, (kPrimaryTableSize - 1) << kHeapObjectTagSize);
 

src/stub-cache.h

@@ -198,11 +198,16 @@ class StubCache : public AllStatic {
 
   // Computes the hashed offsets for primary and secondary caches.
   static int PrimaryOffset(String* name, Code::Flags flags, Map* map) {
+    // This works well because the heap object tag size and the hash
+    // shift are equal.  Shifting down the length field to get the
+    // hash code would effectively throw away two bits of the hash
+    // code.
+    ASSERT(kHeapObjectTagSize == kHashShift);
     // Compute the hash of the name (use entire length field).
-    uint32_t name_hash = name->length_field();
-    ASSERT(name_hash & String::kHashComputedMask);
+    ASSERT(name->HasHashCode());
+    uint32_t field = name->length_field();
     // Base the offset on a simple combination of name, flags, and map.
-    uint32_t key = (reinterpret_cast<uint32_t>(map) + name_hash) ^ flags;
+    uint32_t key = (reinterpret_cast<uint32_t>(map) + field) ^ flags;
     return key & ((kPrimaryTableSize - 1) << kHeapObjectTagSize);
   }
 

test/mjsunit/debug-evaluate-locals.js

@@ -32,29 +32,59 @@ Debug = debug.Debug
 listenerComplete = false;
 exception = false;
 
+
+function checkFrame0(name, value) {
+  assertTrue(name == 'a' || name == 'b');
+  if (name == 'a') {
+    assertEquals(1, value);
+  }
+  if (name == 'b') {
+    assertEquals(2, value);
+  }
+}
+
+
+function checkFrame1(name, value) {
+  assertTrue(name == '.arguments' || name == 'a');
+  if (name == 'a') {
+    assertEquals(3, value);
+  }
+}
+
+
+function checkFrame2(name, value) {
+  assertTrue(name == '.arguments' || name == 'a' ||
+             name == 'arguments' || name == 'b');
+  if (name == 'a') {
+    assertEquals(5, value);
+  }
+  if (name == 'b') {
+    assertEquals(0, value);
+  }
+}
+
+
 function listener(event, exec_state, event_data, data) {
   try {
     if (event == Debug.DebugEvent.Break)
     {
       // Frame 0 has normal variables a and b.
-      assertEquals('a', exec_state.frame(0).localName(0));
-      assertEquals('b', exec_state.frame(0).localName(1));
-      assertEquals(1, exec_state.frame(0).localValue(0).value());
-      assertEquals(2, exec_state.frame(0).localValue(1).value());
+      var frame0 = exec_state.frame(0);
+      checkFrame0(frame0.localName(0), frame0.localValue(0).value());
+      checkFrame0(frame0.localName(1), frame0.localValue(1).value());
 
       // Frame 1 has normal variable a (and the .arguments variable).
-      assertEquals('.arguments', exec_state.frame(1).localName(0));
-      assertEquals('a', exec_state.frame(1).localName(1));
-      assertEquals(3, exec_state.frame(1).localValue(1).value());
+      var frame1 = exec_state.frame(1);
+      checkFrame1(frame1.localName(0), frame1.localValue(0).value());
+      checkFrame1(frame1.localName(1), frame1.localValue(1).value());
 
-      // Frame 0 has normal variables a and b (and both the .arguments and
+      // Frame 2 has normal variables a and b (and both the .arguments and
       // arguments variable).
-      assertEquals('.arguments', exec_state.frame(2).localName(0));
-      assertEquals('a', exec_state.frame(2).localName(1));
-      assertEquals('arguments', exec_state.frame(2).localName(2));
-      assertEquals('b', exec_state.frame(2).localName(3));
-      assertEquals(5, exec_state.frame(2).localValue(1).value());
-      assertEquals(0, exec_state.frame(2).localValue(3).value());
+      var frame2 = exec_state.frame(2);
+      checkFrame2(frame2.localName(0), frame2.localValue(0).value());
+      checkFrame2(frame2.localName(1), frame2.localValue(1).value());
+      checkFrame2(frame2.localName(2), frame2.localValue(2).value());
+      checkFrame2(frame2.localName(3), frame2.localValue(3).value());
 
       // Evaluating a and b on frames 0, 1 and 2 produces 1, 2, 3, 4, 5 and 6.
       assertEquals(1, exec_state.frame(0).evaluate('a').value());

test/mjsunit/regress/regress-1170187.js

@@ -35,11 +35,32 @@ Debug = debug.Debug
 listenerCalled = false;
 exception = false;
 
+
+function checkName(name) {
+  assertTrue(name == 'a' || name == 'b' || name == 'c');
+}
+
+
+function checkValue(value) {
+  assertEquals(void 0, value);
+}
+
+
 function listener(event, exec_state, event_data, data) {
   try {
     if (event == Debug.DebugEvent.Break) {
-      assertEquals('c', exec_state.frame(0).localName(0));
-      assertEquals(void 0, exec_state.frame(0).localValue(0).value());
+      var local0Name = exec_state.frame(0).localName(0);
+      var local1Name = exec_state.frame(0).localName(1);
+      var local2Name = exec_state.frame(0).localName(2);
+      checkName(local0Name);
+      checkName(local1Name);
+      checkName(local2Name);
+      var local0Value = exec_state.frame(0).localValue(0).value();
+      var local1Value = exec_state.frame(0).localValue(1).value();
+      var local2Value = exec_state.frame(0).localValue(2).value();
+      checkValue(local0Value);
+      checkValue(local1Value);
+      checkValue(local2Value);
       listenerCalled = true;
     }
   } catch (e) {