# HG changeset patch # User dnsimon # Date 1448934007 36000 # Node ID 1cfcb971cb17ea32552fc18c7fbc692ed5dfb16a # Parent d914dfe7abf55324480dacc99570d5eef4a99045 8143730: [JVMCI] infopoint recording is too restrictive Reviewed-by: twisti diff -r d914dfe7abf5 -r 1cfcb971cb17 hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.code/src/jdk/vm/ci/code/CompilationResult.java --- a/hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.code/src/jdk/vm/ci/code/CompilationResult.java Mon Nov 30 15:21:25 2015 -0800 +++ b/hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.code/src/jdk/vm/ci/code/CompilationResult.java Mon Nov 30 15:40:07 2015 -1000 @@ -870,31 +870,11 @@ * Records a custom infopoint in the code section. * * Compiler implementations can use this method to record non-standard infopoints, which are not - * handled by the dedicated methods like {@link #recordCall}. + * handled by dedicated methods like {@link #recordCall}. * * @param infopoint the infopoint to record, usually a derived class from {@link Infopoint} */ public void addInfopoint(Infopoint infopoint) { - // The infopoints list must always be sorted - if (!infopoints.isEmpty()) { - Infopoint previousInfopoint = infopoints.get(infopoints.size() - 1); - if (previousInfopoint.pcOffset > infopoint.pcOffset) { - // This re-sorting should be very rare - Collections.sort(infopoints); - previousInfopoint = infopoints.get(infopoints.size() - 1); - } - if (previousInfopoint.pcOffset == infopoint.pcOffset) { - if (infopoint.reason.canBeOmitted()) { - return; - } - if (previousInfopoint.reason.canBeOmitted()) { - Infopoint removed = infopoints.remove(infopoints.size() - 1); - assert removed == previousInfopoint; - } else { - throw new RuntimeException("Infopoints that can not be omited should have distinct PCs"); - } - } - } infopoints.add(infopoint); } diff -r d914dfe7abf5 -r 1cfcb971cb17 hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.code/src/jdk/vm/ci/code/InfopointReason.java --- a/hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.code/src/jdk/vm/ci/code/InfopointReason.java Mon Nov 30 15:21:25 2015 -0800 +++ b/hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.code/src/jdk/vm/ci/code/InfopointReason.java Mon Nov 30 15:40:07 2015 -1000 @@ -26,22 +26,12 @@ * A reason for infopoint insertion. */ public enum InfopointReason { - UNKNOWN(false), - SAFEPOINT(false), - CALL(false), - IMPLICIT_EXCEPTION(false), - METHOD_START(true), - METHOD_END(true), - LINE_NUMBER(true), - METASPACE_ACCESS(true); - private InfopointReason(boolean canBeOmitted) { - this.canBeOmitted = canBeOmitted; - } - - private final boolean canBeOmitted; - - public boolean canBeOmitted() { - return canBeOmitted; - } + SAFEPOINT, + CALL, + IMPLICIT_EXCEPTION, + METASPACE_ACCESS, + METHOD_START, + METHOD_END, + BYTECODE_POSITION; } diff -r d914dfe7abf5 -r 1cfcb971cb17 hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotCompiledCode.java --- a/hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotCompiledCode.java Mon Nov 30 15:21:25 2015 -0800 +++ b/hotspot/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotCompiledCode.java Mon Nov 30 15:40:07 2015 -1000 @@ -24,9 +24,12 @@ import java.nio.ByteBuffer; import java.nio.ByteOrder; +import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; +import java.util.EnumMap; import java.util.List; +import java.util.Map; import java.util.stream.Stream; import java.util.stream.Stream.Builder; @@ -41,6 +44,8 @@ import jdk.vm.ci.code.CompilationResult.Mark; import jdk.vm.ci.code.CompilationResult.Site; import jdk.vm.ci.code.DataSection; +import jdk.vm.ci.code.InfopointReason; +import jdk.vm.ci.common.JVMCIError; import jdk.vm.ci.meta.Assumptions.Assumption; import jdk.vm.ci.meta.ResolvedJavaMethod; @@ -155,14 +160,75 @@ static class SiteComparator implements Comparator { + /** + * Defines an order for sorting {@link Infopoint}s based on their + * {@linkplain Infopoint#reason reasons}. This is used to choose which infopoint to preserve + * when multiple infopoints collide on the same PC offset. A negative order value implies a + * non-optional infopoint (i.e., must be preserved). Non-optional infopoints must not + * collide. + */ + static final Map HOTSPOT_INFOPOINT_SORT_ORDER = new EnumMap<>(InfopointReason.class); + static { + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.SAFEPOINT, -4); + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.CALL, -3); + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.IMPLICIT_EXCEPTION, -2); + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.METASPACE_ACCESS, 1); + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.METHOD_START, 2); + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.METHOD_END, 3); + HOTSPOT_INFOPOINT_SORT_ORDER.put(InfopointReason.BYTECODE_POSITION, 4); + } + + static int ord(Infopoint info) { + return HOTSPOT_INFOPOINT_SORT_ORDER.get(info.reason); + } + + static int checkCollision(Infopoint i1, Infopoint i2) { + int o1 = ord(i1); + int o2 = ord(i2); + if (o1 < 0 && o2 < 0) { + throw new JVMCIError("Non-optional infopoints cannot collide: %s and %s", i1, i2); + } + return o1 - o2; + } + + /** + * Records whether any two {@link Infopoint}s had the same {@link Infopoint#pcOffset}. + */ + boolean sawCollidingInfopoints; + public int compare(Site s1, Site s2) { - if (s1.pcOffset == s2.pcOffset && (s1 instanceof Mark ^ s2 instanceof Mark)) { - return s1 instanceof Mark ? -1 : 1; + if (s1.pcOffset == s2.pcOffset) { + // Marks must come first since patching a call site + // may need to know the mark denoting the call type + // (see uses of CodeInstaller::_next_call_type). + boolean s1IsMark = s1 instanceof Mark; + boolean s2IsMark = s2 instanceof Mark; + if (s1IsMark != s2IsMark) { + return s1IsMark ? -1 : 1; + } + + // Infopoints must group together so put them after + // other Site types. + boolean s1IsInfopoint = s1 instanceof Infopoint; + boolean s2IsInfopoint = s2 instanceof Infopoint; + if (s1IsInfopoint != s2IsInfopoint) { + return s1IsInfopoint ? 1 : -1; + } + + if (s1IsInfopoint) { + sawCollidingInfopoints = true; + return checkCollision((Infopoint) s1, (Infopoint) s2); + } } return s1.pcOffset - s2.pcOffset; } } + /** + * HotSpot expects sites to be presented in ascending order of PC (see + * {@code DebugInformationRecorder::add_new_pc_offset}). In addition, it expects + * {@link Infopoint} PCs to be unique. + */ private static Site[] getSortedSites(CompilationResult target) { List[] lists = new List[]{target.getInfopoints(), target.getDataPatches(), target.getMarks()}; int count = 0; @@ -176,7 +242,27 @@ result[pos++] = (Site) elem; } } - Arrays.sort(result, new SiteComparator()); + SiteComparator c = new SiteComparator(); + Arrays.sort(result, c); + if (c.sawCollidingInfopoints) { + Infopoint lastInfopoint = null; + List copy = new ArrayList<>(count); + for (int i = 0; i < count; i++) { + if (result[i] instanceof Infopoint) { + Infopoint info = (Infopoint) result[i]; + if (lastInfopoint == null || lastInfopoint.pcOffset != info.pcOffset) { + lastInfopoint = info; + copy.add(info); + } else { + // Omit this colliding infopoint + assert lastInfopoint.reason.compareTo(info.reason) <= 0; + } + } else { + copy.add(result[i]); + } + } + result = copy.toArray(new Site[copy.size()]); + } return result; } diff -r d914dfe7abf5 -r 1cfcb971cb17 hotspot/src/share/vm/jvmci/jvmciCodeInstaller.cpp --- a/hotspot/src/share/vm/jvmci/jvmciCodeInstaller.cpp Mon Nov 30 15:21:25 2015 -0800 +++ b/hotspot/src/share/vm/jvmci/jvmciCodeInstaller.cpp Mon Nov 30 15:40:07 2015 -1000 @@ -727,10 +727,9 @@ if (InfopointReason::SAFEPOINT() == reason || InfopointReason::CALL() == reason || InfopointReason::IMPLICIT_EXCEPTION() == reason) { TRACE_jvmci_4("safepoint at %i", pc_offset); site_Safepoint(buffer, pc_offset, site, CHECK_OK); - } else if (InfopointReason::METHOD_START() == reason || InfopointReason::METHOD_END() == reason || InfopointReason::LINE_NUMBER() == reason) { + } else { + TRACE_jvmci_4("infopoint at %i", pc_offset); site_Infopoint(buffer, pc_offset, site, CHECK_OK); - } else { - JVMCI_ERROR_OK("unknown infopoint reason at %i", pc_offset); } } else if (site->is_a(CompilationResult_DataPatch::klass())) { TRACE_jvmci_4("datapatch at %i", pc_offset); @@ -868,25 +867,33 @@ return objects; } -void CodeInstaller::record_scope(jint pc_offset, Handle debug_info, TRAPS) { +void CodeInstaller::record_scope(jint pc_offset, Handle debug_info, ScopeMode scope_mode, TRAPS) { Handle position = DebugInfo::bytecodePosition(debug_info); if (position.is_null()) { // Stubs do not record scope info, just oop maps return; } - GrowableArray* objectMapping = record_virtual_objects(debug_info, CHECK); - record_scope(pc_offset, position, objectMapping, CHECK); + GrowableArray* objectMapping; + if (scope_mode == CodeInstaller::FullFrame) { + objectMapping = record_virtual_objects(debug_info, CHECK); + } else { + objectMapping = NULL; + } + record_scope(pc_offset, position, scope_mode, objectMapping, CHECK); } -void CodeInstaller::record_scope(jint pc_offset, Handle position, GrowableArray* objects, TRAPS) { +void CodeInstaller::record_scope(jint pc_offset, Handle position, ScopeMode scope_mode, GrowableArray* objects, TRAPS) { Handle frame; - if (position->is_a(BytecodeFrame::klass())) { + if (scope_mode == CodeInstaller::FullFrame) { + if (!position->is_a(BytecodeFrame::klass())) { + JVMCI_ERROR("Full frame expected for debug info at %i", pc_offset); + } frame = position; } Handle caller_frame = BytecodePosition::caller(position); if (caller_frame.not_null()) { - record_scope(pc_offset, caller_frame, objects, CHECK); + record_scope(pc_offset, caller_frame, scope_mode, objects, CHECK); } Handle hotspot_method = BytecodePosition::method(position); @@ -990,7 +997,7 @@ // jint next_pc_offset = Assembler::locate_next_instruction(instruction) - _instructions->start(); OopMap *map = create_oop_map(debug_info, CHECK); _debug_recorder->add_safepoint(pc_offset, map); - record_scope(pc_offset, debug_info, CHECK); + record_scope(pc_offset, debug_info, CodeInstaller::FullFrame, CHECK); _debug_recorder->end_safepoint(pc_offset); } @@ -1000,8 +1007,12 @@ JVMCI_ERROR("debug info expected at infopoint at %i", pc_offset); } + // We'd like to check that pc_offset is greater than the + // last pc recorded with _debug_recorder (raising an exception if not) + // but DebugInformationRecorder doesn't have sufficient public API. + _debug_recorder->add_non_safepoint(pc_offset); - record_scope(pc_offset, debug_info, CHECK); + record_scope(pc_offset, debug_info, CodeInstaller::BytecodePosition, CHECK); _debug_recorder->end_non_safepoint(pc_offset); } @@ -1028,7 +1039,7 @@ if (debug_info.not_null()) { OopMap *map = create_oop_map(debug_info, CHECK); _debug_recorder->add_safepoint(next_pc_offset, map); - record_scope(next_pc_offset, debug_info, CHECK); + record_scope(next_pc_offset, debug_info, CodeInstaller::FullFrame, CHECK); } if (foreign_call.not_null()) { diff -r d914dfe7abf5 -r 1cfcb971cb17 hotspot/src/share/vm/jvmci/jvmciCodeInstaller.hpp --- a/hotspot/src/share/vm/jvmci/jvmciCodeInstaller.hpp Mon Nov 30 15:21:25 2015 -0800 +++ b/hotspot/src/share/vm/jvmci/jvmciCodeInstaller.hpp Mon Nov 30 15:40:07 2015 -1000 @@ -219,8 +219,18 @@ OopMap* create_oop_map(Handle debug_info, TRAPS); - void record_scope(jint pc_offset, Handle debug_info, TRAPS); - void record_scope(jint pc_offset, Handle code_pos, GrowableArray* objects, TRAPS); + /** + * Specifies the level of detail to record for a scope. + */ + enum ScopeMode { + // Only record a method and BCI + BytecodePosition, + // Record a method, bci and JVM frame state + FullFrame + }; + + void record_scope(jint pc_offset, Handle debug_info, ScopeMode scope_mode, TRAPS); + void record_scope(jint pc_offset, Handle position, ScopeMode scope_mode, GrowableArray* objects, TRAPS); void record_object_value(ObjectValue* sv, Handle value, GrowableArray* objects, TRAPS); GrowableArray* record_virtual_objects(Handle debug_info, TRAPS); diff -r d914dfe7abf5 -r 1cfcb971cb17 hotspot/src/share/vm/jvmci/jvmciJavaClasses.hpp --- a/hotspot/src/share/vm/jvmci/jvmciJavaClasses.hpp Mon Nov 30 15:21:25 2015 -0800 +++ b/hotspot/src/share/vm/jvmci/jvmciJavaClasses.hpp Mon Nov 30 15:40:07 2015 -1000 @@ -148,14 +148,9 @@ int_field(CompilationResult_DataSectionReference, offset) \ end_class \ start_class(InfopointReason) \ - static_oop_field(InfopointReason, UNKNOWN, "Ljdk/vm/ci/code/InfopointReason;") \ static_oop_field(InfopointReason, SAFEPOINT, "Ljdk/vm/ci/code/InfopointReason;") \ static_oop_field(InfopointReason, CALL, "Ljdk/vm/ci/code/InfopointReason;") \ static_oop_field(InfopointReason, IMPLICIT_EXCEPTION, "Ljdk/vm/ci/code/InfopointReason;") \ - static_oop_field(InfopointReason, METHOD_START, "Ljdk/vm/ci/code/InfopointReason;") \ - static_oop_field(InfopointReason, METHOD_END, "Ljdk/vm/ci/code/InfopointReason;") \ - static_oop_field(InfopointReason, LINE_NUMBER, "Ljdk/vm/ci/code/InfopointReason;") \ - static_oop_field(InfopointReason, METASPACE_ACCESS, "Ljdk/vm/ci/code/InfopointReason;") \ end_class \ start_class(CompilationResult_Infopoint) \ oop_field(CompilationResult_Infopoint, debugInfo, "Ljdk/vm/ci/code/DebugInfo;") \