Skip to content

make local var hoisting disabled by default #8158

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
jpbempel merged 3 commits into from
Jan 7, 2025
Merged

make local var hoisting disabled by default #8158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
903f09c
make local var hoisting disabled by default
jpbempel Jan 6, 2025
7c51414
Update system-tests to c6e54d143cfdf97b2f0a815f22f53247c119f635
jpbempel Jan 7, 2025
2413b07
when hoisting is disabled, behave like previously
jpbempel Jan 7, 2025
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
2 changes: 1 addition & 1 deletion .circleci/config.continue.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ instrumentation_modules: &instrumentation_modules "dd-java-agent/instrumentation
debugger_modules: &debugger_modules "dd-java-agent/agent-debugger|dd-java-agent/agent-bootstrap|dd-java-agent/agent-builder|internal-api|communication|dd-trace-core"
profiling_modules: &profiling_modules "dd-java-agent/agent-profiling"

default_system_tests_commit: &default_system_tests_commit 752456136fab0642446d124ead05aa7d8f4fa5c4
default_system_tests_commit: &default_system_tests_commit c6e54d143cfdf97b2f0a815f22f53247c119f635

parameters:
nightly:
Expand Down
5 changes: 3 additions & 2 deletions ...ugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,8 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList)
return;
}
Collection<LocalVariableNode> localVarNodes;
if (definition.isLineProbe()) {
boolean isLocalVarHoistingEnabled = Config.get().isDebuggerHoistLocalVarsEnabled();
if (definition.isLineProbe() || !isLocalVarHoistingEnabled) {
localVarNodes = methodNode.localVariables;
} else {
localVarNodes = unscopedLocalVars;
Expand All @@ -913,7 +914,7 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList)
int idx = variableNode.index - localVarBaseOffset;
if (idx >= argOffset) {
// var is local not arg
if (isLineProbe) {
if (isLineProbe || !isLocalVarHoistingEnabled) {
if (ASMHelper.isInScope(methodNode, variableNode, location)) {
applicableVars.add(variableNode);
}
Expand Down
126 changes: 75 additions & 51 deletions ...a-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static com.datadog.debugger.util.MoshiSnapshotHelper.REDACTED_IDENT_REASON;
import static com.datadog.debugger.util.MoshiSnapshotHelper.REDACTED_TYPE_REASON;
import static com.datadog.debugger.util.MoshiSnapshotTestHelper.VALUE_ADAPTER;
import static com.datadog.debugger.util.TestHelper.setFieldInConfig;
import static datadog.trace.bootstrap.debugger.util.Redaction.REDACTED_VALUE;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -238,14 +239,20 @@ public void oldClass1_1() throws Exception {

@Test
public void oldJavacBug() throws Exception {
final String CLASS_NAME = "com.datadog.debugger.classfiles.JavacBug"; // compiled with jdk 1.6
TestSnapshotListener listener = installSingleProbe(CLASS_NAME, "main", null);
Class<?> testClass = Class.forName(CLASS_NAME);
assertNotNull(testClass);
int result = Reflect.onClass(testClass).call("main", "").get();
assertEquals(45, result);
// with local var hoisting and initialization at the beginning of the method, issue is resolved
assertEquals(1, listener.snapshots.size());
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
try {
final String CLASS_NAME = "com.datadog.debugger.classfiles.JavacBug"; // compiled with jdk 1.6
TestSnapshotListener listener = installSingleProbe(CLASS_NAME, "main", null);
Class<?> testClass = Class.forName(CLASS_NAME);
assertNotNull(testClass);
int result = Reflect.onClass(testClass).call("main", "").get();
assertEquals(45, result);
// with local var hoisting and initialization at the beginning of the method, issue is
// resolved
assertEquals(1, listener.snapshots.size());
} finally {
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
}
}

@Test
Expand Down Expand Up @@ -1801,32 +1808,39 @@ public void evaluateAtExitFalse() throws IOException, URISyntaxException {
value = "datadog.trace.api.Platform#isJ9",
disabledReason = "we cannot get local variable debug info")
public void uncaughtExceptionConditionLocalVar() throws IOException, URISyntaxException {
final String CLASS_NAME = "CapturedSnapshot05";
LogProbe probe =
createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "(String)")
.when(
new ProbeCondition(DSL.when(DSL.ge(DSL.ref("after"), DSL.value(0))), "after >= 0"))
.evaluateAt(MethodLocation.EXIT)
.build();
TestSnapshotListener listener = installProbes(probe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
try {
Reflect.onClass(testClass).call("main", "triggerUncaughtException").get();
Assertions.fail("should not reach this code");
} catch (ReflectException ex) {
assertEquals("oops", ex.getCause().getCause().getMessage());

final String CLASS_NAME = "CapturedSnapshot05";
LogProbe probe =
createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "(String)")
.when(
new ProbeCondition(
DSL.when(DSL.ge(DSL.ref("after"), DSL.value(0))), "after >= 0"))
.evaluateAt(MethodLocation.EXIT)
.build();
TestSnapshotListener listener = installProbes(probe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
try {
Reflect.onClass(testClass).call("main", "triggerUncaughtException").get();
Assertions.fail("should not reach this code");
} catch (ReflectException ex) {
assertEquals("oops", ex.getCause().getCause().getMessage());
}
Snapshot snapshot = assertOneSnapshot(listener);
assertCaptureThrowable(
snapshot.getCaptures().getReturn(),
"CapturedSnapshot05$CustomException",
"oops",
"CapturedSnapshot05.triggerUncaughtException",
8);
assertNull(snapshot.getEvaluationErrors());
// after is 0 because the exception is thrown before the assignment and local var initialized
// at the beginning of the method by instrumentation
assertCaptureLocals(snapshot.getCaptures().getReturn(), "after", "long", "0");
} finally {
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
}
Snapshot snapshot = assertOneSnapshot(listener);
assertCaptureThrowable(
snapshot.getCaptures().getReturn(),
"CapturedSnapshot05$CustomException",
"oops",
"CapturedSnapshot05.triggerUncaughtException",
8);
assertNull(snapshot.getEvaluationErrors());
// after is 0 because the exception is thrown before the assignment and local var initialized
// at the beginning of the method by instrumentation
assertCaptureLocals(snapshot.getCaptures().getReturn(), "after", "long", "0");
}

@Test
Expand Down Expand Up @@ -1858,15 +1872,20 @@ public void uncaughtExceptionCaptureLocalVars() throws IOException, URISyntaxExc
value = "datadog.trace.api.Platform#isJ9",
disabledReason = "we cannot get local variable debug info")
public void methodProbeLocalVarsLocalScopes() throws IOException, URISyntaxException {
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
LogProbe probe = createProbeAtExit(PROBE_ID, CLASS_NAME, "localScopes", "(String)");
TestSnapshotListener listener = installProbes(probe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "localScopes").get();
assertEquals(42, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size());
assertCaptureLocals(snapshot.getCaptures().getReturn(), "@return", "int", "42");
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
try {
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
LogProbe probe = createProbeAtExit(PROBE_ID, CLASS_NAME, "localScopes", "(String)");
TestSnapshotListener listener = installProbes(probe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "localScopes").get();
assertEquals(42, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size());
assertCaptureLocals(snapshot.getCaptures().getReturn(), "@return", "int", "42");
} finally {
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
}
}

@Test
Expand Down Expand Up @@ -1957,16 +1976,21 @@ public void overlappingLocalVarSlot() throws IOException, URISyntaxException {
value = "datadog.trace.api.Platform#isJ9",
disabledReason = "we cannot get local variable debug info")
public void duplicateLocalDifferentScope() throws IOException, URISyntaxException {
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
LogProbe probe =
createProbeAtExit(PROBE_ID, CLASS_NAME, "duplicateLocalDifferentScope", "(String)");
TestSnapshotListener listener = installProbes(probe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "duplicateLocalDifferentScope").get();
assertEquals(28, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertCaptureLocals(
snapshot.getCaptures().getReturn(), "ch", Character.TYPE.getTypeName(), "e");
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
try {
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
LogProbe probe =
createProbeAtExit(PROBE_ID, CLASS_NAME, "duplicateLocalDifferentScope", "(String)");
TestSnapshotListener listener = installProbes(probe);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "duplicateLocalDifferentScope").get();
assertEquals(28, result);
Snapshot snapshot = assertOneSnapshot(listener);
assertCaptureLocals(
snapshot.getCaptures().getReturn(), "ch", Character.TYPE.getTypeName(), "e");
} finally {
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
}
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public final class ConfigDefaults {
static final boolean DEFAULT_DEBUGGER_VERIFY_BYTECODE = true;
static final boolean DEFAULT_DEBUGGER_INSTRUMENT_THE_WORLD = false;
static final int DEFAULT_DEBUGGER_CAPTURE_TIMEOUT = 100; // milliseconds
static final boolean DEFAULT_DEBUGGER_HOIST_LOCALVARS_ENABLED = true;
static final boolean DEFAULT_DEBUGGER_HOIST_LOCALVARS_ENABLED = false;
static final boolean DEFAULT_DEBUGGER_SYMBOL_ENABLED = true;
static final boolean DEFAULT_DEBUGGER_SYMBOL_FORCE_UPLOAD = false;
static final int DEFAULT_DEBUGGER_SYMBOL_FLUSH_THRESHOLD = 100; // nb of classes
Expand Down
Loading