Skip to content

Commit cc81ae9

Browse files
jpbempeljpbempel
authored
make local var hoisting disabled by default (#8158)
until we have a complete data and CFG analysis make it safe.
1 parent 3f8fbe6 commit cc81ae9

File tree

4 files changed

+80
-55
lines changed

4 files changed

+80
-55
lines changed

‎.circleci/config.continue.yml.j2‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ instrumentation_modules: &instrumentation_modules "dd-java-agent/instrumentation
3636
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"
3737
profiling_modules: &profiling_modules "dd-java-agent/agent-profiling"
3838

39-
default_system_tests_commit: &default_system_tests_commit 752456136fab0642446d124ead05aa7d8f4fa5c4
39+
default_system_tests_commit: &default_system_tests_commit c6e54d143cfdf97b2f0a815f22f53247c119f635
4040

4141
parameters:
4242
nightly:

‎dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -902,7 +902,8 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList)
902902
return;
903903
}
904904
Collection<LocalVariableNode> localVarNodes;
905-
if (definition.isLineProbe()) {
905+
boolean isLocalVarHoistingEnabled = Config.get().isDebuggerHoistLocalVarsEnabled();
906+
if (definition.isLineProbe() || !isLocalVarHoistingEnabled) {
906907
localVarNodes = methodNode.localVariables;
907908
} else {
908909
localVarNodes = unscopedLocalVars;
@@ -913,7 +914,7 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList)
913914
int idx = variableNode.index - localVarBaseOffset;
914915
if (idx >= argOffset) {
915916
// var is local not arg
916-
if (isLineProbe) {
917+
if (isLineProbe || !isLocalVarHoistingEnabled) {
917918
if (ASMHelper.isInScope(methodNode, variableNode, location)) {
918919
applicableVars.add(variableNode);
919920
}

‎dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java‎

Lines changed: 75 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import static com.datadog.debugger.util.MoshiSnapshotHelper.REDACTED_IDENT_REASON;
88
import static com.datadog.debugger.util.MoshiSnapshotHelper.REDACTED_TYPE_REASON;
99
import static com.datadog.debugger.util.MoshiSnapshotTestHelper.VALUE_ADAPTER;
10+
import static com.datadog.debugger.util.TestHelper.setFieldInConfig;
1011
import static datadog.trace.bootstrap.debugger.util.Redaction.REDACTED_VALUE;
1112
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
1213
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -238,14 +239,20 @@ public void oldClass1_1() throws Exception {
238239

239240
@Test
240241
public void oldJavacBug() throws Exception {
241-
final String CLASS_NAME = "com.datadog.debugger.classfiles.JavacBug"; // compiled with jdk 1.6
242-
TestSnapshotListener listener = installSingleProbe(CLASS_NAME, "main", null);
243-
Class<?> testClass = Class.forName(CLASS_NAME);
244-
assertNotNull(testClass);
245-
int result = Reflect.onClass(testClass).call("main", "").get();
246-
assertEquals(45, result);
247-
// with local var hoisting and initialization at the beginning of the method, issue is resolved
248-
assertEquals(1, listener.snapshots.size());
242+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
243+
try {
244+
final String CLASS_NAME = "com.datadog.debugger.classfiles.JavacBug"; // compiled with jdk 1.6
245+
TestSnapshotListener listener = installSingleProbe(CLASS_NAME, "main", null);
246+
Class<?> testClass = Class.forName(CLASS_NAME);
247+
assertNotNull(testClass);
248+
int result = Reflect.onClass(testClass).call("main", "").get();
249+
assertEquals(45, result);
250+
// with local var hoisting and initialization at the beginning of the method, issue is
251+
// resolved
252+
assertEquals(1, listener.snapshots.size());
253+
} finally {
254+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
255+
}
249256
}
250257

251258
@Test
@@ -1801,32 +1808,39 @@ public void evaluateAtExitFalse() throws IOException, URISyntaxException {
18011808
value = "datadog.trace.api.Platform#isJ9",
18021809
disabledReason = "we cannot get local variable debug info")
18031810
public void uncaughtExceptionConditionLocalVar() throws IOException, URISyntaxException {
1804-
final String CLASS_NAME = "CapturedSnapshot05";
1805-
LogProbe probe =
1806-
createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "(String)")
1807-
.when(
1808-
new ProbeCondition(DSL.when(DSL.ge(DSL.ref("after"), DSL.value(0))), "after >= 0"))
1809-
.evaluateAt(MethodLocation.EXIT)
1810-
.build();
1811-
TestSnapshotListener listener = installProbes(probe);
1812-
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
1811+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
18131812
try {
1814-
Reflect.onClass(testClass).call("main", "triggerUncaughtException").get();
1815-
Assertions.fail("should not reach this code");
1816-
} catch (ReflectException ex) {
1817-
assertEquals("oops", ex.getCause().getCause().getMessage());
1813+
1814+
final String CLASS_NAME = "CapturedSnapshot05";
1815+
LogProbe probe =
1816+
createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "(String)")
1817+
.when(
1818+
new ProbeCondition(
1819+
DSL.when(DSL.ge(DSL.ref("after"), DSL.value(0))), "after >= 0"))
1820+
.evaluateAt(MethodLocation.EXIT)
1821+
.build();
1822+
TestSnapshotListener listener = installProbes(probe);
1823+
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
1824+
try {
1825+
Reflect.onClass(testClass).call("main", "triggerUncaughtException").get();
1826+
Assertions.fail("should not reach this code");
1827+
} catch (ReflectException ex) {
1828+
assertEquals("oops", ex.getCause().getCause().getMessage());
1829+
}
1830+
Snapshot snapshot = assertOneSnapshot(listener);
1831+
assertCaptureThrowable(
1832+
snapshot.getCaptures().getReturn(),
1833+
"CapturedSnapshot05$CustomException",
1834+
"oops",
1835+
"CapturedSnapshot05.triggerUncaughtException",
1836+
8);
1837+
assertNull(snapshot.getEvaluationErrors());
1838+
// after is 0 because the exception is thrown before the assignment and local var initialized
1839+
// at the beginning of the method by instrumentation
1840+
assertCaptureLocals(snapshot.getCaptures().getReturn(), "after", "long", "0");
1841+
} finally {
1842+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
18181843
}
1819-
Snapshot snapshot = assertOneSnapshot(listener);
1820-
assertCaptureThrowable(
1821-
snapshot.getCaptures().getReturn(),
1822-
"CapturedSnapshot05$CustomException",
1823-
"oops",
1824-
"CapturedSnapshot05.triggerUncaughtException",
1825-
8);
1826-
assertNull(snapshot.getEvaluationErrors());
1827-
// after is 0 because the exception is thrown before the assignment and local var initialized
1828-
// at the beginning of the method by instrumentation
1829-
assertCaptureLocals(snapshot.getCaptures().getReturn(), "after", "long", "0");
18301844
}
18311845

18321846
@Test
@@ -1858,15 +1872,20 @@ public void uncaughtExceptionCaptureLocalVars() throws IOException, URISyntaxExc
18581872
value = "datadog.trace.api.Platform#isJ9",
18591873
disabledReason = "we cannot get local variable debug info")
18601874
public void methodProbeLocalVarsLocalScopes() throws IOException, URISyntaxException {
1861-
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
1862-
LogProbe probe = createProbeAtExit(PROBE_ID, CLASS_NAME, "localScopes", "(String)");
1863-
TestSnapshotListener listener = installProbes(probe);
1864-
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
1865-
int result = Reflect.onClass(testClass).call("main", "localScopes").get();
1866-
assertEquals(42, result);
1867-
Snapshot snapshot = assertOneSnapshot(listener);
1868-
assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size());
1869-
assertCaptureLocals(snapshot.getCaptures().getReturn(), "@return", "int", "42");
1875+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
1876+
try {
1877+
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
1878+
LogProbe probe = createProbeAtExit(PROBE_ID, CLASS_NAME, "localScopes", "(String)");
1879+
TestSnapshotListener listener = installProbes(probe);
1880+
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
1881+
int result = Reflect.onClass(testClass).call("main", "localScopes").get();
1882+
assertEquals(42, result);
1883+
Snapshot snapshot = assertOneSnapshot(listener);
1884+
assertEquals(1, snapshot.getCaptures().getReturn().getLocals().size());
1885+
assertCaptureLocals(snapshot.getCaptures().getReturn(), "@return", "int", "42");
1886+
} finally {
1887+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
1888+
}
18701889
}
18711890

18721891
@Test
@@ -1957,16 +1976,21 @@ public void overlappingLocalVarSlot() throws IOException, URISyntaxException {
19571976
value = "datadog.trace.api.Platform#isJ9",
19581977
disabledReason = "we cannot get local variable debug info")
19591978
public void duplicateLocalDifferentScope() throws IOException, URISyntaxException {
1960-
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
1961-
LogProbe probe =
1962-
createProbeAtExit(PROBE_ID, CLASS_NAME, "duplicateLocalDifferentScope", "(String)");
1963-
TestSnapshotListener listener = installProbes(probe);
1964-
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
1965-
int result = Reflect.onClass(testClass).call("main", "duplicateLocalDifferentScope").get();
1966-
assertEquals(28, result);
1967-
Snapshot snapshot = assertOneSnapshot(listener);
1968-
assertCaptureLocals(
1969-
snapshot.getCaptures().getReturn(), "ch", Character.TYPE.getTypeName(), "e");
1979+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", true);
1980+
try {
1981+
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot31";
1982+
LogProbe probe =
1983+
createProbeAtExit(PROBE_ID, CLASS_NAME, "duplicateLocalDifferentScope", "(String)");
1984+
TestSnapshotListener listener = installProbes(probe);
1985+
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
1986+
int result = Reflect.onClass(testClass).call("main", "duplicateLocalDifferentScope").get();
1987+
assertEquals(28, result);
1988+
Snapshot snapshot = assertOneSnapshot(listener);
1989+
assertCaptureLocals(
1990+
snapshot.getCaptures().getReturn(), "ch", Character.TYPE.getTypeName(), "e");
1991+
} finally {
1992+
setFieldInConfig(Config.get(), "debuggerHoistLocalVarsEnabled", false);
1993+
}
19701994
}
19711995

19721996
@Test

‎dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ public final class ConfigDefaults {
181181
static final boolean DEFAULT_DEBUGGER_VERIFY_BYTECODE = true;
182182
static final boolean DEFAULT_DEBUGGER_INSTRUMENT_THE_WORLD = false;
183183
static final int DEFAULT_DEBUGGER_CAPTURE_TIMEOUT = 100; // milliseconds
184-
static final boolean DEFAULT_DEBUGGER_HOIST_LOCALVARS_ENABLED = true;
184+
static final boolean DEFAULT_DEBUGGER_HOIST_LOCALVARS_ENABLED = false;
185185
static final boolean DEFAULT_DEBUGGER_SYMBOL_ENABLED = true;
186186
static final boolean DEFAULT_DEBUGGER_SYMBOL_FORCE_UPLOAD = false;
187187
static final int DEFAULT_DEBUGGER_SYMBOL_FLUSH_THRESHOLD = 100; // nb of classes

0 commit comments

Comments
 (0)