OSDN Git Service

Fix FieldGap priority queue ordering bug.
authorRichard Uhler <ruhler@google.com>
Tue, 14 Jul 2015 00:00:35 +0000 (17:00 -0700)
committerRichard Uhler <ruhler@google.com>
Wed, 15 Jul 2015 15:36:56 +0000 (08:36 -0700)
The priority queue for keeping track of gaps when packing fields in a
class object had the order reversed, giving priority to smaller gaps
instead of priority to larger gaps. This led to cases where fields
were not placed in gaps when they could be.

Bug: 22460222
Change-Id: I062e772e030c034adc227d75deed31c3322e203e

runtime/class_linker.cc
runtime/mirror/abstract_method.h
runtime/oat.h
test/140-field-packing/expected.txt [new file with mode: 0644]
test/140-field-packing/info.txt [new file with mode: 0644]
test/140-field-packing/src/GapOrder.java [new file with mode: 0644]
test/140-field-packing/src/GapOrderBase.java [new file with mode: 0644]
test/140-field-packing/src/Main.java [new file with mode: 0644]

index 0694227..fbb2796 100644 (file)
@@ -194,7 +194,9 @@ struct FieldGapsComparator {
   bool operator() (const FieldGap& lhs, const FieldGap& rhs)
       NO_THREAD_SAFETY_ANALYSIS {
     // Sort by gap size, largest first. Secondary sort by starting offset.
-    return lhs.size > rhs.size || (lhs.size == rhs.size && lhs.start_offset < rhs.start_offset);
+    // Note that the priority queue returns the largest element, so operator()
+    // should return true if lhs is less than rhs.
+    return lhs.size < rhs.size || (lhs.size == rhs.size && lhs.start_offset > rhs.start_offset);
   }
 };
 typedef std::priority_queue<FieldGap, std::vector<FieldGap>, FieldGapsComparator> FieldGaps;
index 99d697a..6240b3b 100644 (file)
@@ -60,9 +60,8 @@ class MANAGED AbstractMethod : public AccessibleObject {
 
   HeapReference<mirror::Class> declaring_class_;
   HeapReference<mirror::Class> declaring_class_of_overridden_method_;
-  uint32_t padding_;
-  uint64_t art_method_;
   uint32_t access_flags_;
+  uint64_t art_method_;
   uint32_t dex_method_index_;
 
   friend struct art::AbstractMethodOffsets;  // for verifying offset information
index 3451d0f..ee2f3f6 100644 (file)
@@ -32,7 +32,7 @@ class InstructionSetFeatures;
 class PACKED(4) OatHeader {
  public:
   static constexpr uint8_t kOatMagic[] = { 'o', 'a', 't', '\n' };
-  static constexpr uint8_t kOatVersion[] = { '0', '6', '6', '\0' };
+  static constexpr uint8_t kOatVersion[] = { '0', '6', '7', '\0' };
 
   static constexpr const char* kImageLocationKey = "image-location";
   static constexpr const char* kDex2OatCmdLineKey = "dex2oat-cmdline";
diff --git a/test/140-field-packing/expected.txt b/test/140-field-packing/expected.txt
new file mode 100644 (file)
index 0000000..2b0a2ce
--- /dev/null
@@ -0,0 +1,2 @@
+running test...
+test completed.
diff --git a/test/140-field-packing/info.txt b/test/140-field-packing/info.txt
new file mode 100644 (file)
index 0000000..a28bd04
--- /dev/null
@@ -0,0 +1 @@
+Test field packing for classes with various arrangements of fields.
diff --git a/test/140-field-packing/src/GapOrder.java b/test/140-field-packing/src/GapOrder.java
new file mode 100644 (file)
index 0000000..09d09b8
--- /dev/null
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2014 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.
+ */
+
+// Regression test for 22460222, the sub class.
+// The field gaps order was wrong. If there were two gaps of different sizes,
+// and the larger one was needed, it wouldn't be found.
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+
+class GapOrder extends GapOrderBase {
+  // The base class is 9 bytes. The entire class should be packed as:
+  //
+  //    00: oooo oooo
+  //    08: b-ss rrrr
+  //    16: rrrr iiii
+  //    24: dddd dddd
+  //
+  // The problem was, the packer wasn't finding the gap where iiii should go,
+  // because the gap where ss goes was given priority. Instead it packed as:
+  //    00: oooo oooo
+  //    08: b--- rrrr
+  //    16: rrrr ----
+  //    24: dddd dddd
+  //    32: iiii ss
+  public Object r1;
+  public Object r2;
+  public double d;
+  public int i;
+  public short s;
+
+  static private void CheckField(String fieldName, int expected) {
+    Field field = null;
+    try {
+      field = GapOrder.class.getField(fieldName);
+    } catch (ReflectiveOperationException e) {
+      System.out.println(fieldName + " not found in GapOrder.");
+      return;
+    }
+
+    int actual = -1;
+    try {
+      Method getOffset = Field.class.getMethod("getOffset");
+      actual = (Integer)getOffset.invoke(field);
+    } catch (ReflectiveOperationException e) {
+      System.out.println("Unable to get field offset for " + fieldName + ":" + e);
+      return;
+    }
+
+    if (actual != expected) {
+      System.out.println(
+          String.format("GapOrder.%s has offset %d, but expected %d",
+            fieldName, actual, expected));
+    }
+  }
+
+  static public void Check() {
+    CheckField("r1", 12);
+    CheckField("r2", 16);
+    CheckField("d", 24);
+    CheckField("i", 20);
+    CheckField("s", 10);
+  }
+}
+
diff --git a/test/140-field-packing/src/GapOrderBase.java b/test/140-field-packing/src/GapOrderBase.java
new file mode 100644 (file)
index 0000000..4a0b378
--- /dev/null
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2014 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.
+ */
+
+// Regression test for 22460222, the base class.
+// The field gaps order was wrong. If there were two gaps of different sizes,
+// and the larger one was needed, it wouldn't be found.
+
+// This class has a size of 9 bytes: 8 for object plus 1 for the field 'b'.
+class GapOrderBase {
+  public byte b;
+}
diff --git a/test/140-field-packing/src/Main.java b/test/140-field-packing/src/Main.java
new file mode 100644 (file)
index 0000000..2810b32
--- /dev/null
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2014 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.
+ */
+
+public class Main {
+  public static void main(String[] args) {
+    System.out.println("running test...");
+    GapOrder.Check();
+    System.out.println("test completed.");
+  }
+}