Fix StringReferenceValueComparator.

Test: Added a regression test in string_reference_test.cc,
run the standard ART test suite on host and Nexus 5.

Bug: 29602109
Change-Id: Idcc059a07df048a0e3ece257b16b6556f242243e

3 files changed, 111 insertions, 4 deletions

diff --git a/build/Android.gtest.mk b/build/Android.gtest.mk
index 1afbdfcb59..c09116ff5d 100644
--- a/build/Android.gtest.mk
+++ b/build/Android.gtest.mk
@@ -277,6 +277,7 @@ COMPILER_GTEST_COMMON_SRC_FILES := \
   compiler/optimizing/suspend_check_test.cc \
   compiler/utils/dedupe_set_test.cc \
   compiler/utils/intrusive_forward_list_test.cc \
+  compiler/utils/string_reference_test.cc \
   compiler/utils/swap_space_test.cc \
   compiler/utils/test_dex_file_builder_test.cc \
   compiler/utils/transform_array_ref_test.cc \
diff --git a/compiler/utils/string_reference.h b/compiler/utils/string_reference.h
index 9e1058ea4d..0ab45c86e3 100644
--- a/compiler/utils/string_reference.h
+++ b/compiler/utils/string_reference.h
@@ -20,12 +20,11 @@
 #include <stdint.h>
 
 #include "base/logging.h"
+#include "dex_file-inl.h"
 #include "utf-inl.h"
 
 namespace art {
 
-class DexFile;
-
 // A string is located by its DexFile and the string_ids_ table index into that DexFile.
 struct StringReference {
   StringReference(const DexFile* file, uint32_t index) : dex_file(file), string_index(index) { }
@@ -48,13 +47,13 @@ struct StringReferenceValueComparator {
           sr1.string_index < sr2.string_index,
           CompareModifiedUtf8ToModifiedUtf8AsUtf16CodePointValues(
               sr1.dex_file->GetStringData(sr1.dex_file->GetStringId(sr1.string_index)),
-              sr1.dex_file->GetStringData(sr2.dex_file->GetStringId(sr2.string_index))) < 0);
+              sr2.dex_file->GetStringData(sr2.dex_file->GetStringId(sr2.string_index))) < 0);
       return sr1.string_index < sr2.string_index;
     } else {
       // Cannot compare indexes, so do the string comparison.
       return CompareModifiedUtf8ToModifiedUtf8AsUtf16CodePointValues(
           sr1.dex_file->GetStringData(sr1.dex_file->GetStringId(sr1.string_index)),
-          sr1.dex_file->GetStringData(sr2.dex_file->GetStringId(sr2.string_index))) < 0;
+          sr2.dex_file->GetStringData(sr2.dex_file->GetStringId(sr2.string_index))) < 0;
     }
   }
 };
diff --git a/compiler/utils/string_reference_test.cc b/compiler/utils/string_reference_test.cc
new file mode 100644
index 0000000000..df5080e93e
--- /dev/null
+++ b/compiler/utils/string_reference_test.cc
@@ -0,0 +1,107 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "utils/string_reference.h"
+
+#include <memory>
+
+#include "gtest/gtest.h"
+#include "utils/test_dex_file_builder.h"
+
+namespace art {
+
+TEST(StringReference, ValueComparator) {
+  // This is a regression test for the StringReferenceValueComparator using the wrong
+  // dex file to get the string data from a StringId. We construct two dex files with
+  // just a single string with the same length but different value. This creates dex
+  // files that have the same layout, so the byte offset read from the StringId in one
+  // dex file, when used in the other dex file still points to valid string data, except
+  // that it's the wrong string. Without the fix the strings would then compare equal.
+  TestDexFileBuilder builder1;
+  builder1.AddString("String1");
+  std::unique_ptr<const DexFile> dex_file1 = builder1.Build("dummy location 1");
+  ASSERT_EQ(1u, dex_file1->NumStringIds());
+  ASSERT_STREQ("String1", dex_file1->GetStringData(dex_file1->GetStringId(0)));
+  StringReference sr1(dex_file1.get(), 0);
+
+  TestDexFileBuilder builder2;
+  builder2.AddString("String2");
+  std::unique_ptr<const DexFile> dex_file2 = builder2.Build("dummy location 2");
+  ASSERT_EQ(1u, dex_file2->NumStringIds());
+  ASSERT_STREQ("String2", dex_file2->GetStringData(dex_file2->GetStringId(0)));
+  StringReference sr2(dex_file2.get(), 0);
+
+  StringReferenceValueComparator cmp;
+  EXPECT_TRUE(cmp(sr1, sr2));  // "String1" < "String2" is true.
+  EXPECT_FALSE(cmp(sr2, sr1));  // "String2" < "String1" is false.
+}
+
+TEST(StringReference, ValueComparator2) {
+  const char* const kDexFile1Strings[] = {
+      "",
+      "abc",
+      "abcxyz",
+  };
+  const char* const kDexFile2Strings[] = {
+      "a",
+      "abc",
+      "abcdef",
+      "def",
+  };
+  const bool expectedCmp12[arraysize(kDexFile1Strings)][arraysize(kDexFile2Strings)] = {
+      { true, true, true, true },
+      { false, false, true, true },
+      { false, false, false, true },
+  };
+  const bool expectedCmp21[arraysize(kDexFile2Strings)][arraysize(kDexFile1Strings)] = {
+      { false, true, true },
+      { false, false, true },
+      { false, false, true },
+      { false, false, false },
+  };
+
+  TestDexFileBuilder builder1;
+  for (const char* s : kDexFile1Strings) {
+    builder1.AddString(s);
+  }
+  std::unique_ptr<const DexFile> dex_file1 = builder1.Build("dummy location 1");
+  ASSERT_EQ(arraysize(kDexFile1Strings), dex_file1->NumStringIds());
+  for (size_t index = 0; index != arraysize(kDexFile1Strings); ++index) {
+    ASSERT_STREQ(kDexFile1Strings[index], dex_file1->GetStringData(dex_file1->GetStringId(index)));
+  }
+
+  TestDexFileBuilder builder2;
+  for (const char* s : kDexFile2Strings) {
+    builder2.AddString(s);
+  }
+  std::unique_ptr<const DexFile> dex_file2 = builder2.Build("dummy location 1");
+  ASSERT_EQ(arraysize(kDexFile2Strings), dex_file2->NumStringIds());
+  for (size_t index = 0; index != arraysize(kDexFile2Strings); ++index) {
+    ASSERT_STREQ(kDexFile2Strings[index], dex_file2->GetStringData(dex_file2->GetStringId(index)));
+  }
+
+  StringReferenceValueComparator cmp;
+  for (size_t index1 = 0; index1 != arraysize(kDexFile1Strings); ++index1) {
+    for (size_t index2 = 0; index2 != arraysize(kDexFile2Strings); ++index2) {
+      StringReference sr1(dex_file1.get(), index1);
+      StringReference sr2(dex_file2.get(), index2);
+      EXPECT_EQ(expectedCmp12[index1][index2], cmp(sr1, sr2)) << index1 << " " << index2;
+      EXPECT_EQ(expectedCmp21[index2][index1], cmp(sr2, sr1)) << index1 << " " << index2;
+    }
+  }
+}
+
+}  // namespace art