Skip to content

[GR-43934] Only build full FrameTree when OmitInlinedMethodDebugLineInfo is false. #6504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 5, 2023
Merged

[GR-43934] Only build full FrameTree when OmitInlinedMethodDebugLineInfo is false. #6504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b70c640
Only build full FrameTree when OmitInlinedMethodDebugLineInfo is false
olpaw Apr 26, 2023
f79c0a5
Do not use SourceMappings for DebugCodeInfo generation per-default
olpaw Apr 27, 2023
00eaa8e
Make -H:-OmitInlinedMethodDebugLineInfo the default
olpaw Apr 27, 2023
abdb630
Handle missing caller FileEntry in DwarfInfoSectionImpl.writeInlineSu…
olpaw Apr 27, 2023
13ff344
[GR-45807] Dwarf debuginfo generation makes only sense when LIR backe…
olpaw Apr 27, 2023
5e6ceb5
Use unboxed int
olpaw May 2, 2023
969971c
Make dependency to old DebugInfo explicit in expression
olpaw May 4, 2023
6430425
Remove -H:-OmitInlinedMethodDebugLineInfo since it's now the default
olpaw May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion substratevm/mx.substratevm/mx_substratevm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,6 @@ def _debuginfotest(native_image, path, build_only, with_isolates_only, args):
'-cp', classpath('com.oracle.svm.test'),
'-Dgraal.LogFile=graal.log',
'-g',
'-H:-OmitInlinedMethodDebugLineInfo',
'-H:+SourceLevelDebug',
'-H:DebugInfoSourceSearchPath=' + sourcepath,
'-H:DebugInfoSourceCacheRoot=' + join(path, 'sources'),
Expand Down
25 changes: 14 additions & 11 deletions ...m/src/com.oracle.objectfile/src/com/oracle/objectfile/elf/dwarf/DwarfInfoSectionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,30 +32,30 @@
import java.util.Iterator;
import java.util.List;

import jdk.vm.ci.meta.JavaConstant;
import jdk.vm.ci.meta.JavaKind;
import jdk.vm.ci.meta.PrimitiveConstant;

import org.graalvm.compiler.debug.DebugContext;

import com.oracle.objectfile.LayoutDecision;
import com.oracle.objectfile.debugentry.ArrayTypeEntry;
import com.oracle.objectfile.debugentry.ClassEntry;
import com.oracle.objectfile.debugentry.CompiledMethodEntry;
import com.oracle.objectfile.debugentry.FieldEntry;
import com.oracle.objectfile.debugentry.FileEntry;
import com.oracle.objectfile.debugentry.HeaderTypeEntry;
import com.oracle.objectfile.debugentry.InterfaceClassEntry;
import com.oracle.objectfile.debugentry.MethodEntry;
import com.oracle.objectfile.debugentry.CompiledMethodEntry;
import com.oracle.objectfile.debugentry.PrimitiveTypeEntry;
import com.oracle.objectfile.debugentry.range.Range;
import com.oracle.objectfile.debugentry.range.SubRange;
import com.oracle.objectfile.debugentry.StructureTypeEntry;
import com.oracle.objectfile.debugentry.TypeEntry;
import com.oracle.objectfile.debugentry.range.Range;
import com.oracle.objectfile.debugentry.range.SubRange;
import com.oracle.objectfile.debuginfo.DebugInfoProvider.DebugLocalInfo;
import com.oracle.objectfile.debuginfo.DebugInfoProvider.DebugLocalValueInfo;
import com.oracle.objectfile.debuginfo.DebugInfoProvider.DebugPrimitiveTypeInfo;

import jdk.vm.ci.meta.JavaConstant;
import jdk.vm.ci.meta.JavaKind;
import jdk.vm.ci.meta.PrimitiveConstant;

/**
* Section generator for debug_info section.
*/
Expand Down Expand Up @@ -1341,17 +1341,20 @@ private int writeInlineSubroutine(DebugContext context, Range caller, int depth,

int callLine = caller.getLine();
assert callLine >= -1 : callLine;
Integer fileIndex;
int fileIndex;
if (callLine == -1) {
log(context, " Unable to retrieve call line for inlined method %s", callee.getFullMethodName());
/* continue with line 0 and fileIndex 1 as we must insert a tree node */
callLine = 0;
fileIndex = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local var fileIndex is declared a few lines up as an Integer, a legacy of an earlier version where the index for a given source dir was stored in a per-CU (i.e. per ClassEntry) map. This no longer applies with the latest code where the file index is defined globally across all classes, hence stored as an int in the FileEntry. It would be good to take the opportunity to clean this up by declaring fileIndex as an int.

} else {
FileEntry subFileEntry = caller.getFileEntry();
assert subFileEntry != null : caller.getClassName() + "." + caller.getMethodName() + "(" + caller.getFileName() + ":" + callLine + ")";
fileIndex = caller.getFileIndex();
assert fileIndex != null;
if (subFileEntry != null) {
fileIndex = subFileEntry.getIdx();
} else {
log(context, " Unable to retrieve caller FileEntry for inlined method %s (caller method %s)", callee.getFullMethodName(), caller.getFullMethodName());
fileIndex = 1;
}
}
final int code;
code = DwarfDebugInfo.DW_ABBREV_CODE_inlined_subroutine_with_children;
Expand Down
12 changes: 11 additions & 1 deletion substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,10 @@ public void update(EconomicMap<OptionKey<?>, Object> values, Object newValue) {
@Option(help = "Use old debuginfo", deprecated = true, deprecationMessage = "Please use the -g option.")//
public static final HostedOptionKey<Boolean> UseOldDebugInfo = new HostedOptionKey<>(false, SubstrateOptions::validateUseOldDebugInfo);

public static boolean useDebugInfoGeneration() {
return useLIRBackend() && GenerateDebugInfo.getValue() > 0 && !UseOldDebugInfo.getValue();
}

private static void validateUseOldDebugInfo(HostedOptionKey<Boolean> optionKey) {
if (optionKey.getValue() && SubstrateOptions.GenerateDebugInfo.getValue() < 1) {
throw UserError.abort("The option '%s' can only be used together with '%s'.",
Expand Down Expand Up @@ -684,7 +688,13 @@ private static void validateStripDebugInfo(HostedOptionKey<Boolean> optionKey) {
}

@Option(help = "Omit generation of DebugLineInfo originating from inlined methods") //
public static final HostedOptionKey<Boolean> OmitInlinedMethodDebugLineInfo = new HostedOptionKey<>(true);
public static final HostedOptionKey<Boolean> OmitInlinedMethodDebugLineInfo = new HostedOptionKey<>(false);

@Option(help = "Specify maximum inlining depth to consider when building DebugCodeInfo") //
public static final HostedOptionKey<Integer> DebugCodeInfoMaxDepth = new HostedOptionKey<>(Integer.MAX_VALUE);

@Option(help = "Do not use SourceMappings for generating DebugCodeInfo (i.e. only use Infopoints)") //
public static final HostedOptionKey<Boolean> DebugCodeInfoUseSourceMappings = new HostedOptionKey<>(false);

@Option(help = "Emit debuginfo debug.svm.imagebuild.* sections with detailed image-build options.")//
public static final HostedOptionKey<Boolean> UseImagebuildDebugSections = new HostedOptionKey<>(true);
Expand Down
11 changes: 6 additions & 5 deletions ...m/src/com.oracle.svm.core/src/com/oracle/svm/core/UniqueShortNameProviderDefaultImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,17 @@
*/
package com.oracle.svm.core;

import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;
import jdk.vm.ci.meta.ResolvedJavaType;
import jdk.vm.ci.meta.Signature;

import java.lang.reflect.Constructor;
import java.lang.reflect.Executable;
import java.lang.reflect.Member;
import java.lang.reflect.Method;
import java.util.function.BooleanSupplier;

import com.oracle.svm.core.feature.AutomaticallyRegisteredImageSingleton;

import jdk.vm.ci.meta.ResolvedJavaType;
import jdk.vm.ci.meta.Signature;

/**
* Default implementation for unique method and field short names which concatenates the
* (unqualified) owner class name and method or field selector with an SHA1 digest of the fully
Expand Down Expand Up @@ -93,7 +94,7 @@ public String uniqueShortLoaderName(ClassLoader classLoader) {
public static class UseDefault implements BooleanSupplier {

public static boolean useDefaultProvider() {
return !(OS.LINUX.isCurrent() && SubstrateOptions.GenerateDebugInfo.getValue() > 0 && !SubstrateOptions.UseOldDebugInfo.getValue());
return !OS.LINUX.isCurrent() || !SubstrateOptions.useDebugInfoGeneration();
}

@Override
Expand Down
106 changes: 66 additions & 40 deletions ...tevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/CompilationResultFrameTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,26 +31,35 @@
import java.util.List;
import java.util.function.Consumer;

import jdk.vm.ci.code.BytecodeFrame;
import jdk.vm.ci.meta.JavaValue;
import jdk.vm.ci.meta.Local;
import jdk.vm.ci.meta.LocalVariableTable;
import jdk.vm.ci.meta.ResolvedJavaMethod;
import org.graalvm.compiler.code.CompilationResult;
import org.graalvm.compiler.code.SourceMapping;
import org.graalvm.compiler.debug.DebugContext;

import com.oracle.svm.core.util.VMError;

import jdk.vm.ci.code.BytecodeFrame;
import jdk.vm.ci.code.BytecodePosition;
import jdk.vm.ci.code.site.Call;
import jdk.vm.ci.code.site.Infopoint;
import jdk.vm.ci.code.site.InfopointReason;
import jdk.vm.ci.common.JVMCIError;
import jdk.vm.ci.meta.JavaValue;
import jdk.vm.ci.meta.Local;
import jdk.vm.ci.meta.LocalVariableTable;
import jdk.vm.ci.meta.ResolvedJavaMethod;

public final class CompilationResultFrameTree {

public abstract static class SourcePositionSupplier implements Comparable<SourcePositionSupplier> {

private final BytecodePosition bytecodePosition;
private final int callDepth;

protected SourcePositionSupplier(BytecodePosition bytecodePosition, int callDepth) {
this.bytecodePosition = bytecodePosition;
this.callDepth = callDepth;
}

public abstract int getStartOffset();

public abstract int getSize();
Expand All @@ -59,7 +68,9 @@ public int getEndOffset() {
return getStartOffset() + getSize() - 1;
}

public abstract BytecodePosition getBytecodePosition();
public BytecodePosition getBytecodePosition() {
return bytecodePosition;
}

public static StackTraceElement getStackTraceElement(BytecodePosition pos) {
return pos.getMethod().asStackTraceElement(pos.getBCI());
Expand All @@ -82,9 +93,13 @@ public final String getStackFrameStr() {
}
}

public final int getCallDepth() {
public int getCallDepth() {
return callDepth;
}

protected static int getCallDepth(BytecodePosition startPos) {
int depth = 0;
BytecodePosition pos = getBytecodePosition();
BytecodePosition pos = startPos;
while (pos != null) {
depth += 1;
pos = pos.getCaller();
Expand Down Expand Up @@ -119,14 +134,20 @@ public int compareTo(SourcePositionSupplier o) {
public static final class InfopointSourceWrapper extends SourcePositionSupplier {
public final Infopoint infopoint;

public static InfopointSourceWrapper create(Infopoint infopoint) {
public static InfopointSourceWrapper create(Infopoint infopoint, int maxDepth) {
if (infopoint.debugInfo == null || infopoint.debugInfo.getBytecodePosition() == null) {
return null;
}
return new InfopointSourceWrapper(infopoint);
BytecodePosition pos = infopoint.debugInfo.getBytecodePosition();
int depth = getCallDepth(pos);
if (depth > maxDepth) {
return null;
}
return new InfopointSourceWrapper(pos, depth, infopoint);
}

private InfopointSourceWrapper(Infopoint infopoint) {
private InfopointSourceWrapper(BytecodePosition bytecodePosition, int callDepth, Infopoint infopoint) {
super(bytecodePosition, callDepth);
this.infopoint = infopoint;
}

Expand All @@ -143,11 +164,6 @@ public int getSize() {
return 1;
}

@Override
public BytecodePosition getBytecodePosition() {
return infopoint.debugInfo.getBytecodePosition();
}

@Override
public int compareTo(SourcePositionSupplier o) {
int res = super.compareTo(o);
Expand All @@ -166,22 +182,29 @@ public int compareTo(SourcePositionSupplier o) {
public static final class SourceMappingWrapper extends SourcePositionSupplier {
public final SourceMapping sourceMapping;

public static SourceMappingWrapper create(SourceMapping sourceMapping) {
public static SourceMappingWrapper create(SourceMapping sourceMapping, int maxDepth) {
if (sourceMapping.getSourcePosition() == null) {
return null;
}
BytecodePosition pos = sourceMapping.getSourcePosition();
int depth = getCallDepth(pos);
if (depth > maxDepth) {
return null;
}
if (sourceMapping.getStartOffset() > sourceMapping.getEndOffset()) {
JVMCIError.shouldNotReachHere("Invalid SourceMapping " + getSourceMappingString(sourceMapping));
}
return new SourceMappingWrapper(sourceMapping);
return new SourceMappingWrapper(pos, depth, sourceMapping);
}

static String getSourceMappingString(SourceMapping sourceMapping) {
SourceMappingWrapper tmp = new SourceMappingWrapper(sourceMapping);
BytecodePosition pos = sourceMapping.getSourcePosition();
SourceMappingWrapper tmp = new SourceMappingWrapper(pos, getCallDepth(pos), sourceMapping);
return tmp.getPosStr() + " with " + tmp.getStackFrameStr();
}

private SourceMappingWrapper(SourceMapping sourceMapping) {
private SourceMappingWrapper(BytecodePosition bytecodePosition, int callDepth, SourceMapping sourceMapping) {
super(bytecodePosition, callDepth);
this.sourceMapping = sourceMapping;
}

Expand All @@ -208,11 +231,6 @@ public int getSize() {
return sourceMapping.getEndOffset() - sourceMapping.getStartOffset();
}

@Override
public BytecodePosition getBytecodePosition() {
return sourceMapping.getSourcePosition();
}

@Override
public int compareTo(SourcePositionSupplier o) {
int res = super.compareTo(o);
Expand Down Expand Up @@ -417,7 +435,9 @@ public String toString() {
public static final class Builder {
private Builder.RootNode root = null;
private final int targetCodeSize;
private final int maxDepth;
private final DebugContext debug;
private final boolean useSourceMappings;
private final boolean verify;

int indexLeft;
Expand Down Expand Up @@ -446,8 +466,10 @@ public String toString() {
}
}

public Builder(DebugContext debug, int targetCodeSize, boolean verify) {
public Builder(DebugContext debug, int targetCodeSize, int maxDepth, boolean useSourceMappings, boolean verify) {
this.targetCodeSize = targetCodeSize;
this.maxDepth = maxDepth;
this.useSourceMappings = useSourceMappings;
this.verify = verify;
this.debug = debug;
}
Expand All @@ -462,34 +484,38 @@ public CallNode build(CompilationResult compilationResult) {
List<SourcePositionSupplier> sourcePosData = new ArrayList<>(infopoints.size() + sourceMappings.size());
InfopointSourceWrapper infopointForRoot = null;
for (Infopoint infopoint : infopoints) {
InfopointSourceWrapper wrapper = InfopointSourceWrapper.create(infopoint);
InfopointSourceWrapper wrapper = InfopointSourceWrapper.create(infopoint, maxDepth);
if (wrapper != null) {
sourcePosData.add(wrapper);
infopointForRoot = wrapper;
} else {
debug.log(DebugContext.DETAILED_LEVEL, " Discard Infopoint without BytecodePosition %s", infopoint);
debug.log(DebugContext.DETAILED_LEVEL, " Discard Infopoint %s", infopoint);
}
}
for (SourceMapping sourceMapping : sourceMappings) {
SourceMappingWrapper wrapper = SourceMappingWrapper.create(sourceMapping);
if (wrapper != null) {
if (wrapper.getStartOffset() > targetCodeSize - 1) {
if (useSourceMappings) {
for (SourceMapping sourceMapping : sourceMappings) {
SourceMappingWrapper wrapper = SourceMappingWrapper.create(sourceMapping, maxDepth);
if (wrapper != null) {
if (wrapper.getStartOffset() > targetCodeSize - 1) {
if (debug.isLogEnabled(DebugContext.DETAILED_LEVEL)) {
debug.log(" Discard SourceMapping outside code-range %s", SourceMappingWrapper.getSourceMappingString(sourceMapping));
}
continue;
}
sourcePosData.add(wrapper);
} else {
if (debug.isLogEnabled(DebugContext.DETAILED_LEVEL)) {
debug.log(" Discard SourceMapping outside code-range %s", SourceMappingWrapper.getSourceMappingString(sourceMapping));
debug.log(" Discard SourceMapping %s", SourceMappingWrapper.getSourceMappingString(sourceMapping));
}
continue;
}
sourcePosData.add(wrapper);
} else {
if (debug.isLogEnabled(DebugContext.DETAILED_LEVEL)) {
debug.log(" Discard SourceMapping without NodeSourcePosition %s", SourceMappingWrapper.getSourceMappingString(sourceMapping));
}
}
}

sourcePosData.sort(Comparator.naturalOrder());

nullifyOverlappingSourcePositions(sourcePosData);
if (useSourceMappings) {
nullifyOverlappingSourcePositions(sourcePosData);
}

if (debug.isLogEnabled(DebugContext.DETAILED_LEVEL)) {
debug.log("Sorted input data:");
Expand Down
2 changes: 1 addition & 1 deletion ...rc/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageDebugInfoFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class NativeImageDebugInfoFeature implements InternalFeature {

@Override
public boolean isInConfiguration(IsInConfigurationAccess access) {
return SubstrateOptions.GenerateDebugInfo.getValue() > 0 && !SubstrateOptions.UseOldDebugInfo.getValue();
return SubstrateOptions.useDebugInfoGeneration();
}

@Override
Expand Down
16 changes: 14 additions & 2 deletions ...c/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageDebugInfoProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,12 +1246,24 @@ public Stream<DebugLocationInfo> locationInfoProvider() {
if (fileName().length() == 0) {
return Stream.empty();
}
final CallNode root = new Builder(debugContext, compilation.getTargetCodeSize(), true).build(compilation);
boolean omitInline = SubstrateOptions.OmitInlinedMethodDebugLineInfo.getValue();
int maxDepth = SubstrateOptions.DebugCodeInfoMaxDepth.getValue();
boolean useSourceMappings = SubstrateOptions.DebugCodeInfoUseSourceMappings.getValue();
if (omitInline) {
if (!SubstrateOptions.DebugCodeInfoMaxDepth.hasBeenSet()) {
/* TopLevelVisitor will not go deeper than level 2 */
maxDepth = 2;
}
if (!SubstrateOptions.DebugCodeInfoUseSourceMappings.hasBeenSet()) {
/* Skip expensive CompilationResultFrameTree building with SourceMappings */
useSourceMappings = false;
}
Copy link
Member

@olpaw olpaw Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If DebugCodeInfoMaxDepth or DebugCodeInfoUseSourceMappings are set explicitly by the user (-H:...) they override any adjustment of the settings performed for OmitInlinedMethodDebugLineInfo.

}
final CallNode root = new Builder(debugContext, compilation.getTargetCodeSize(), maxDepth, useSourceMappings, true).build(compilation);
if (root == null) {
return Stream.empty();
}
final List<DebugLocationInfo> locationInfos = new ArrayList<>();
final boolean omitInline = SubstrateOptions.OmitInlinedMethodDebugLineInfo.getValue();
int frameSize = getFrameSize();
final Visitor visitor = (omitInline ? new TopLevelVisitor(locationInfos, frameSize) : new MultiLevelVisitor(locationInfos, frameSize));
// arguments passed by visitor to apply are
Expand Down
2 changes: 1 addition & 1 deletion ...m.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageDebugInfoStripFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public class NativeImageDebugInfoStripFeature implements InternalFeature {

@Override
public boolean isInConfiguration(IsInConfigurationAccess access) {
return SubstrateOptions.GenerateDebugInfo.getValue() > 0 && SubstrateOptions.StripDebugInfo.getValue() && (SubstrateOptions.useLIRBackend() || SubstrateOptions.useLLVMBackend());
return SubstrateOptions.useDebugInfoGeneration() || SubstrateOptions.UseOldDebugInfo.getValue();
}

@SuppressWarnings("try")
Expand Down