Code cleanup, Add comments

Author: dominikmascherbauer

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java

@@ -99,17 +99,33 @@ private void addFile(FileEntry addFileEntry) {
         }
     }
 
+    /**
+     * Add a field to the class entry and store its file entry.
+     * 
+     * @param field the {@code FieldEntry} to add
+     */
     @Override
     public void addField(FieldEntry field) {
         addFile(field.getFileEntry());
         super.addField(field);
     }
 
+    /**
+     * Add a method to the class entry and store its file entry.
+     *
+     * @param methodEntry the {@code MethodEntry} to add
+     */
     public void addMethod(MethodEntry methodEntry) {
         addFile(methodEntry.getFileEntry());
         methods.add(methodEntry);
     }
 
+    /**
+     * Add a compiled method to the class entry and store its file entry and the file entries of
+     * inlined methods.
+     *
+     * @param compiledMethodEntry the {@code CompiledMethodEntry} to add
+     */
     public void addCompiledMethod(CompiledMethodEntry compiledMethodEntry) {
         addFile(compiledMethodEntry.primary().getFileEntry());
         for (Range range : compiledMethodEntry.topDownRangeStream().toList()) {
@@ -156,6 +172,18 @@ public int getFileIdx() {
         return getFileIdx(this.getFileEntry());
     }
 
+    /**
+     * Returns the file index of a given file entry within this class entry.
+     *
+     * <p>
+     * The first time a file entry is fetched, this produces a file index that is used for further
+     * index lookups. The file index is only created once. Therefore, this method must be used only
+     * after debug info generation is finished and no more file entries can be added to this class
+     * entry.
+     * 
+     * @param file the given file entry
+     * @return the index of the file entry
+     */
     public int getFileIdx(FileEntry file) {
         if (file == null || files.isEmpty() || !files.contains(file)) {
             return 0;
@@ -173,7 +201,7 @@ public int getFileIdx(FileEntry file) {
         return indexedFiles.get(file);
     }
 
-    public DirEntry getDirEntry(FileEntry file) {
+    private DirEntry getDirEntry(FileEntry file) {
         if (file == null) {
             return null;
         }
@@ -185,6 +213,18 @@ public int getDirIdx(FileEntry file) {
         return getDirIdx(dirEntry);
     }
 
+    /**
+     * Returns the dir index of a given dir entry within this class entry.
+     *
+     * <p>
+     * The first time a dir entry is fetched, this produces a dir index that is used for further
+     * index lookups. The dir index is only created once. Therefore, this method must be used only
+     * after debug info generation is finished and no more dir entries can be added to this class
+     * entry.
+     *
+     * @param dir the given dir entry
+     * @return the index of the dir entry
+     */
     public int getDirIdx(DirEntry dir) {
         if (dir == null || dir.getPathString().isEmpty() || dirs.isEmpty() || !dirs.contains(dir)) {
             return 0;
@@ -251,20 +291,20 @@ public long hipc() {
     }
 
     /**
-     * Retrieve a stream of all files referenced from debug info for this class in line info file
+     * Retrieve a list of all files referenced from debug info for this class in line info file
      * table order, starting with the file at index 1.
      *
-     * @return a stream of all referenced files
+     * @return a list of all referenced files
      */
     public List<FileEntry> getFiles() {
         return List.copyOf(files);
     }
 
     /**
-     * Retrieve a stream of all directories referenced from debug info for this class in line info
+     * Retrieve a list of all directories referenced from debug info for this class in line info
      * directory table order, starting with the directory at index 1.
      *
-     * @return a stream of all referenced directories
+     * @return a list of all referenced directories
      */
     public List<DirEntry> getDirs() {
         return List.copyOf(dirs);

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/CompiledMethodEntry.java

@@ -47,7 +47,7 @@ public record CompiledMethodEntry(PrimaryRange primary, List<FrameSizeChangeEntr
      * Returns a stream that traverses all the callees of the method associated with this entry. The
      * stream performs a depth-first pre-order traversal of the call tree.
      *
-     * @return the iterator
+     * @return the stream of all ranges
      */
     public Stream<Range> topDownRangeStream() {
         // skip the root of the range stream which is the primary range
@@ -59,7 +59,7 @@ public Stream<Range> topDownRangeStream() {
      * returns only the leafs. The stream performs a depth-first pre-order traversal of the call
      * tree returning only ranges with no callees.
      *
-     * @return the iterator
+     * @return the stream of leaf ranges
      */
     public Stream<Range> leafRangeStream() {
         return topDownRangeStream().filter(Range::isLeaf);
@@ -70,7 +70,7 @@ public Stream<Range> leafRangeStream() {
      * returns only the call ranges. The stream performs a depth-first pre-order traversal of the
      * call tree returning only ranges with callees.
      *
-     * @return the iterator
+     * @return the stream of call ranges
      */
     public Stream<Range> callRangeStream() {
         return topDownRangeStream().filter(range -> !range.isLeaf());

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/DebugInfoBase.java

@@ -155,20 +155,20 @@ public abstract class DebugInfoBase {
      */
     private ClassEntry hubClassEntry;
 
-    /*
+    /**
      * A prefix used to label indirect types used to ensure gdb performs oop reference --> raw
-     * address translation
+     * address translation.
      */
     public static final String COMPRESSED_PREFIX = "_z_.";
-    /*
+    /**
      * A prefix used for type signature generation to generate unique type signatures for type
-     * layout type units
+     * layout type units.
      */
     public static final String LAYOUT_PREFIX = "_layout_.";
 
-    /*
+    /**
      * The name of the type for header field hub which needs special case processing to remove tag
-     * bits
+     * bits.
      */
     public static final String HUB_TYPE_NAME = "java.lang.Class";
 
@@ -230,9 +230,10 @@ public void installDebugInfo(DebugInfoProvider debugInfoProvider) {
         /* Reference alignment must be 8 bytes. */
         assert objectAlignment == 8;
 
-        stringTable = debugInfoProvider.getStringTable();
-
-        // Create the cachePath string entry which serves as base directory for source files
+        stringTable = new StringTable();
+        /* Ensure we have a null string at the start of the string table. */
+        uniqueDebugString("");
+        /* Create the cachePath string entry which serves as base directory for source files */
         cachePath = uniqueDebugString(debugInfoProvider.cachePath());
 
         compiledMethods.addAll(debugInfoProvider.compiledMethodEntries());

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/LocalEntry.java

@@ -25,10 +25,28 @@
 
 package com.oracle.objectfile.debugentry;
 
-public record LocalEntry(String name, TypeEntry type, int slot, int line) {
+import java.util.concurrent.atomic.AtomicInteger;
+
+public record LocalEntry(String name, TypeEntry type, int slot, AtomicInteger line) {
+
+    public LocalEntry(String name, TypeEntry type, int slot, int line) {
+        /*
+         * Use a AtomicInteger for the line number as it might change if we encounter the same local
+         * variable in a different frame state with a lower line number
+         */
+        this(name, type, slot, new AtomicInteger(line));
+    }
 
     @Override
     public String toString() {
-        return String.format("Local(%s type=%s slot=%d line=%d)", name, type.getTypeName(), slot, line);
+        return String.format("Local(%s type=%s slot=%d line=%d)", name, type.getTypeName(), slot, getLine());
+    }
+
+    public void setLine(int line) {
+        this.line.set(line);
+    }
+
+    public int getLine() {
+        return line.get();
     }
 }

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/MemberEntry.java

@@ -81,6 +81,12 @@ public FileEntry getFileEntry() {
         return fileEntry;
     }
 
+    /**
+     * Fetch the file index from its owner class entry with {@link ClassEntry#getFileIdx}. The file
+     * entry must only be fetched for members whose owner is a {@link ClassEntry}.
+     * 
+     * @return the file index of this members file in the owner class entry
+     */
     public int getFileIdx() {
         if (ownerType instanceof ClassEntry) {
             return ((ClassEntry) ownerType).getFileIdx(fileEntry);

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/MethodEntry.java

@@ -61,10 +61,17 @@ public MethodEntry(FileEntry fileEntry, int line, String methodName, StructureTy
         this.lastParamSlot = lastParamSlot;
 
         this.locals = new ConcurrentSkipListSet<>(Comparator.comparingInt(LocalEntry::slot).thenComparing(LocalEntry::name).thenComparing(le -> le.type().getTypeName()));
-        // sort by line and add all locals, such that the methods locals only contain the lowest
-        // line number
-        locals.stream().sorted(Comparator.comparingInt(LocalEntry::line)).forEach(this.locals::add);
+        /*
+         * Sort by line and add all locals, such that the methods locals only contain the lowest
+         * line number.
+         */
+        locals.stream().sorted(Comparator.comparingInt(LocalEntry::getLine)).forEach(this.locals::add);
 
+        /*
+         * Flags to identify compiled methods. We set inRange if there is a compilation for this
+         * method and inlined if it is encountered as a CallNode when traversing the compilation
+         * result frame tree.
+         */
         this.isInRange = false;
         this.isInlined = false;
     }
@@ -92,6 +99,24 @@ public LocalEntry getThisParam() {
         return thisParam;
     }
 
+    /**
+     * Returns the local entry for a given name slot and type entry. Decides with the slot number
+     * whether this is a parameter or local variable.
+     *
+     * <p>
+     * Local variable might not be contained in this method entry. Creates a new local entry in
+     * {@link #locals} with the given parameters.
+     * <p>
+     * For locals, we also make sure that the line information is the lowest line encountered in
+     * local variable lookups. If a new lower line number is encountered for an existing local
+     * entry, we update the line number in the local entry.
+     * 
+     * @param name the given name
+     * @param slot the given slot
+     * @param type the given {@code TypeEntry}
+     * @param line the given line
+     * @return the local entry stored in {@link #locals}
+     */
     public LocalEntry lookupLocalEntry(String name, int slot, TypeEntry type, int line) {
         if (slot < 0) {
             return null;
@@ -115,12 +140,18 @@ public LocalEntry lookupLocalEntry(String name, int slot, TypeEntry type, int li
              * contained, we might update the line number to the lowest occurring line number.
              */
             LocalEntry local = new LocalEntry(name, type, slot, line);
-            if (!locals.contains(local) || locals.removeIf(le -> le.slot() == slot && le.name().equals(name) && le.type().equals(type) && le.line() > line)) {
-                locals.add(local);
-                return local;
-            } else {
-                return locals.headSet(local, true).last();
+            if (!locals.add(local)) {
+                /*
+                 * Fetch local from locals list. This iterates over all locals to create the head
+                 * set and then takes the last value.
+                 */
+                local = locals.headSet(local, true).last();
+                // Update line number if a lower one was encountered.
+                if (local.getLine() > line) {
+                    local.setLine(line);
+                }
             }
+            return local;
         }
 
         /*

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/range/Range.java

@@ -97,6 +97,10 @@ public abstract class Range implements Comparable<Range> {
      * alternatively, marked as invalid in this range.
      */
     private final Map<LocalEntry, LocalValueEntry> localValueInfos;
+    /**
+     * Mapping of local entries to subranges they occur in.
+     */
+    private final Map<LocalEntry, List<Range>> varRangeMap = new HashMap<>();
 
     /**
      * Create a primary range representing the root of the subrange tree for a top level compiled
@@ -153,19 +157,31 @@ protected Range(PrimaryRange primary, MethodEntry methodEntry, Map<LocalEntry, L
         this.localValueInfos = localValueInfos;
     }
 
+    /**
+     * Splits an initial range at the given stack decrement point. The lower split will stay as it
+     * with its high offset reduced to the stack decrement point. The higher split starts at the
+     * stack decrement point and has updated local value entries for the parameters in the then
+     * extended stack.
+     * 
+     * @param stackDecrement the offset to split the range at
+     * @param frameSize the frame size after the split
+     * @param preExtendFrameSize the frame size before the split
+     * @return the higher split, that has been split off the original {@code Range}
+     */
     public Range split(int stackDecrement, int frameSize, int preExtendFrameSize) {
-        // this should be for an initial range extending beyond the stack decrement
+        // This should be for an initial range extending beyond the stack decrement.
         assert loOffset == 0 && loOffset < stackDecrement && stackDecrement < hiOffset : "invalid split request";
 
         Map<LocalEntry, LocalValueEntry> splitLocalValueInfos = new HashMap<>();
 
         for (var localInfo : localValueInfos.entrySet()) {
             if (localInfo.getValue() instanceof StackValueEntry stackValue) {
-                // need to redefine the value for this param using a stack slot value
-                // that allows for the stack being extended by framesize. however we
-                // also need to remove any adjustment that was made to allow for the
-                // difference between the caller SP and the pre-extend callee SP
-                // because of a stacked return address.
+                /*
+                 * Need to redefine the value for this param using a stack slot value that allows
+                 * for the stack being extended by framesize. however we also need to remove any
+                 * adjustment that was made to allow for the difference between the caller SP and
+                 * the pre-extend callee SP because of a stacked return address.
+                 */
                 int adjustment = frameSize - preExtendFrameSize;
                 splitLocalValueInfos.put(localInfo.getKey(), new StackValueEntry(stackValue.stackSlot() + adjustment));
             } else {
@@ -314,18 +330,34 @@ public int getDepth() {
         return depth;
     }
 
+    /**
+     * Returns the subranges grouped by local entries in the subranges. If the map is empty, it
+     * first tries to populate the map with its callees {@link #localValueInfos}.
+     * 
+     * @return a mapping from local entries to subranges
+     */
     public Map<LocalEntry, List<Range>> getVarRangeMap() {
-        HashMap<LocalEntry, List<Range>> varRangeMap = new HashMap<>();
-        for (Range callee : getCallees()) {
-            for (LocalEntry local : callee.localValueInfos.keySet()) {
-                varRangeMap.computeIfAbsent(local, l -> new ArrayList<>()).add(callee);
+        if (varRangeMap.isEmpty()) {
+            for (Range callee : getCallees()) {
+                for (LocalEntry local : callee.localValueInfos.keySet()) {
+                    varRangeMap.computeIfAbsent(local, l -> new ArrayList<>()).add(callee);
+                }
             }
         }
+
         return varRangeMap;
     }
 
+    /**
+     * Returns whether subranges contain a value in {@link #localValueInfos} for a given local
+     * entry. A value is valid if it exists, and it can be represented as local values in the debug
+     * info.
+     * 
+     * @param local the local entry to check for
+     * @return whether a local entry has a value in any of this range's subranges
+     */
     public boolean hasLocalValues(LocalEntry local) {
-        for (Range callee : getCallees()) {
+        for (Range callee : getVarRangeMap().getOrDefault(local, List.of())) {
             LocalValueEntry localValue = callee.lookupValue(local);
             if (localValue != null) {
                 if (localValue instanceof ConstantValueEntry constantValueEntry) {

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debuginfo/DebugInfoProvider.java

@@ -29,7 +29,6 @@
 import java.util.SortedSet;
 
 import com.oracle.objectfile.debugentry.CompiledMethodEntry;
-import com.oracle.objectfile.debugentry.StringTable;
 import com.oracle.objectfile.debugentry.TypeEntry;
 
 /**
@@ -69,8 +68,6 @@ public interface DebugInfoProvider {
      */
     int objectAlignment();
 
-    StringTable getStringTable();
-
     SortedSet<TypeEntry> typeEntries();
 
     SortedSet<CompiledMethodEntry> compiledMethodEntries();