Skip to content

Add clear_return_stacks default for coroutines modules. #3011

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

Draft
sharvath-newrelic wants to merge 2 commits into
base: change_kotlin_ignores
Choose a base branch
from
Draft

Add clear_return_stacks default for coroutines modules. #3011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f45774
Add clearn_return_stacks default for coroutines modules.
sharvath-newrelic Jul 21, 2026
591d8ae
Extensibility enhancements
sharvath-newrelic Jul 22, 2026
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
32 changes: 30 additions & 2 deletions ...lic-agent/src/main/java/com/newrelic/agent/instrumentation/weaver/ClassWeaverService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import java.security.ProtectionDomain;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
Expand All @@ -91,6 +92,22 @@ public class ClassWeaverService implements ClassMatchVisitorFactory, ContextClas
private static final int PARTITIONS = 8;
private static ClassNode EXTENSION_TEMPLATE;

/**
* Weave packages whose method inlining should default to normalizing operand-return stacks (see
* {@code ReturnInsnProcessor.clearReturnStacks}), since weaving Kotlin-coroutine-generated bytecode
* (e.g. invokeSuspend continuations) can otherwise throw ArrayIndexOutOfBoundsException. An explicit
* -Dnewrelic.config.class_transformer.clear_return_stacks system property still overrides this default.
*/
private static final Set<String> CLEAR_RETURN_STACKS_DEFAULT_WEAVE_PACKAGES = new HashSet<>(Arrays.asList(
"com.newrelic.instrumentation.kotlin-coroutines-1.4",
"com.newrelic.instrumentation.kotlin-coroutines-1.7",
"com.newrelic.instrumentation.kotlin-coroutines-1.9",
"com.newrelic.instrumentation.kotlin-coroutines-suspends",
"com.newrelic.instrumentation.ktor-server-core-3.0.0",
"com.newrelic.instrumentation.ktor-server-netty-3.0.0",
"com.newrelic.instrumentation.labs.ktor-utils-3.0",
"com.newrelic.instrumentation.labs.ktor-client-core-3.0"));

static {
AgentBridge.extensionHolderFactory = new ExtensionHolderFactoryImpl();
try {
Expand Down Expand Up @@ -191,18 +208,29 @@ private WeavePackage createWeavePackage(InputStream inputStream, String source)
return weavePackage;
}

/**
* Whether the named weave package should default return-stack normalization on (see
* {@link #CLEAR_RETURN_STACKS_DEFAULT_WEAVE_PACKAGES}).
*/
static boolean isClearReturnStacksDefaultWeavePackage(String weavePackageName) {
return CLEAR_RETURN_STACKS_DEFAULT_WEAVE_PACKAGES.contains(weavePackageName);
}

private WeavePackageConfig createWeavePackageConfig(JarInputStream jarStream, String source,
Instrumentation instrumentation, WeavePackageType type, AgentConfig agentConfig) throws Exception {
AgentPreprocessors preprocessors = new AgentPreprocessors(agentConfig, tracedWeaveInstrumentationDetails);
AgentPostprocessors postprocessors = new AgentPostprocessors();

WeavePackageConfig result = WeavePackageConfig.builder()
WeavePackageConfig.Builder builder = WeavePackageConfig.builder()
.source(source)
.jarInputStream(jarStream)
.weavePreprocessor(preprocessors)
.weavePostprocessor(postprocessors)
.errorHandleClassNode(LogAndReturnOriginal.ERROR_HANDLER_NODE)
.extensionClassTemplate(EXTENSION_TEMPLATE)
.extensionClassTemplate(EXTENSION_TEMPLATE);

WeavePackageConfig result = builder
.clearReturnStacksDefault(isClearReturnStacksDefaultWeavePackage(builder.getName()))
.build();

preprocessors.setInstrumentationTitle(result.getName());
Expand Down
25 changes: 25 additions & 0 deletions ...agent/src/test/java/com/newrelic/agent/instrumentation/weaver/ClassWeaverServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,34 @@ public void test_registerInstrumentation_validInstrumentationJar() throws Except

Assert.assertNotNull(target.getWeavePackageManger().getWeavePackage("com.newrelic.instrumentation.jms-1.1"));
Assert.assertEquals(1, target.getWeavePackageManger().getRegisteredPackages().size());

// jms-1.1 is not a Kotlin-coroutines/Ktor module, so it should not default clear_return_stacks on
Assert.assertFalse(target.getWeavePackageManger().getWeavePackage("com.newrelic.instrumentation.jms-1.1")
.getConfig().isClearReturnStacksDefault());
}
}

@Test
public void test_isClearReturnStacksDefaultWeavePackage_matchesKnownKotlinCoroutinesAndKtorModules() {
for (String weavePackageName : Arrays.asList(
"com.newrelic.instrumentation.kotlin-coroutines-1.4",
"com.newrelic.instrumentation.kotlin-coroutines-1.7",
"com.newrelic.instrumentation.kotlin-coroutines-1.9",
"com.newrelic.instrumentation.kotlin-coroutines-suspends",
"com.newrelic.instrumentation.ktor-server-core-3.0.0",
"com.newrelic.instrumentation.ktor-server-netty-3.0.0",
"com.newrelic.instrumentation.labs.ktor-utils-3.0",
"com.newrelic.instrumentation.labs.ktor-client-core-3.0")) {
Assert.assertTrue(weavePackageName, ClassWeaverService.isClearReturnStacksDefaultWeavePackage(weavePackageName));
}
}

@Test
public void test_isClearReturnStacksDefaultWeavePackage_doesNotMatchOtherModules() {
Assert.assertFalse(ClassWeaverService.isClearReturnStacksDefaultWeavePackage("com.newrelic.instrumentation.jms-1.1"));
Assert.assertFalse(ClassWeaverService.isClearReturnStacksDefaultWeavePackage(null));
}

@Test
public void test_registerInstrumentation_noTitleInstrumentationJar() throws Exception {
createServiceManager(null);
Expand Down
4 changes: 3 additions & 1 deletion newrelic-weaver/src/main/java/com/newrelic/weave/ClassWeave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.newrelic.weave.utils.SynchronizedClassNode;
import com.newrelic.weave.utils.WeaveUtils;
import com.newrelic.weave.weavepackage.WeavePackage;
import com.newrelic.weave.weavepackage.WeavePackageConfig;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;
Expand Down Expand Up @@ -280,8 +281,9 @@ private MethodNode weaveMethod(final MethodNode weaveMethod, MethodNode targetMe
}

// inline original invocation
WeavePackageConfig weavePackageConfig = weavePackage != null ? weavePackage.getConfig() : null;
composite = MethodProcessors.inlineMethods(WeaveUtils.INLINER_PREFIX + weaveClassName, toInline, target.name,
composite);
composite, weavePackageConfig);
// the inliner sometimes like to sneak in some jsr instructions. Sneaky inliner!
composite = MethodProcessors.removeJSRInstructions(composite);

Expand Down
36 changes: 33 additions & 3 deletions newrelic-weaver/src/main/java/com/newrelic/weave/MethodCallInlinerAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import com.newrelic.weave.utils.ReturnInsnProcessor;
import com.newrelic.weave.utils.WeaveUtils;
import com.newrelic.weave.weavepackage.WeavePackageConfig;
import org.objectweb.asm.Label;
import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;
Expand All @@ -28,6 +29,9 @@
import java.util.ArrayList;

public abstract class MethodCallInlinerAdapter extends LocalVariablesSorter {

private static final String CLEAR_RETURN_STACKS_PROPERTY = "newrelic.config.class_transformer.clear_return_stacks";

/**
* try/catch blocks which originated from the inlined method.
*/
Expand All @@ -40,6 +44,14 @@ public abstract class MethodCallInlinerAdapter extends LocalVariablesSorter {
*/
private final AnalyzerAdapter analyzerAdapter;

/*
* Config of the weave package whose methods are being inlined, or null if none is available (e.g. some test
* call paths). Threaded through so that per-package defaults (e.g. whether return-instruction stack
* normalization should be applied by default, see ReturnInsnProcessor.clearReturnStacks) can be read here
* without requiring a new constructor parameter for each new default this class needs to consult.
*/
private final WeavePackageConfig weavePackageConfig;

/*
* If a method to be inlined is called several times from the same caller method, we don't want to allocate new
* local variables for each invocation. Instead, we want to share the local slots between all call sites. To do this
Expand Down Expand Up @@ -75,14 +87,25 @@ public InlinedMethod(final MethodNode method, final Remapper remapper) {
// they must be computed from scratch in the ClassWriter.
public MethodCallInlinerAdapter(String owner, int access, String name, String desc, MethodVisitor next,
boolean inlineFrames) {
this(WeaveUtils.ASM_API_LEVEL, owner, access, name, desc, next, inlineFrames);
this(owner, access, name, desc, next, inlineFrames, null);
}

public MethodCallInlinerAdapter(String owner, int access, String name, String desc, MethodVisitor next,
boolean inlineFrames, WeavePackageConfig weavePackageConfig) {
this(WeaveUtils.ASM_API_LEVEL, owner, access, name, desc, next, inlineFrames, weavePackageConfig);
}

protected MethodCallInlinerAdapter(int api, String owner, int access, String name, String desc, MethodVisitor next,
boolean inlineFrames) {
this(api, owner, access, name, desc, next, inlineFrames, null);
}

protected MethodCallInlinerAdapter(int api, String owner, int access, String name, String desc, MethodVisitor next,
boolean inlineFrames, WeavePackageConfig weavePackageConfig) {
super(api, access, desc, getNext(owner, access, name, desc, next, inlineFrames));
this.analyzerAdapter = inlineFrames ? (AnalyzerAdapter) mv : null;
this.mv = new InlinedTryCatchBlockSorter(WeaveUtils.ASM_API_LEVEL, this.mv, access, name, desc, null, null);
this.weavePackageConfig = weavePackageConfig;
}

private static MethodVisitor getNext(String owner, int access, String name, String desc, MethodVisitor next,
Expand Down Expand Up @@ -135,8 +158,15 @@ private InlinedMethod getInliner(String owner, String name, String desc) {
MethodNode methodNodeCopy = WeaveUtils.copy(method.method);
//Feature flag for method nodes that require additional return insn processing.
//Introduced because some Kotlin code throws ArrayIndexOutOfBoundsException when weaved.
//To enable this feature, set -Dnewrelic.config.class_transformer.clear_return_stacks=true
if (Boolean.getBoolean("newrelic.config.class_transformer.clear_return_stacks")) {
//Defaults on for weave packages whose config sets clearReturnStacksDefault (e.g. Kotlin-coroutines/
//Ktor modules); an explicit -Dnewrelic.config.class_transformer.clear_return_stacks system property
//always overrides that default in either direction.
boolean clearReturnStacksDefault = weavePackageConfig != null
&& weavePackageConfig.isClearReturnStacksDefault();
String clearReturnStacksProp = System.getProperty(CLEAR_RETURN_STACKS_PROPERTY);
boolean clearReturnStacks = clearReturnStacksProp != null
? Boolean.parseBoolean(clearReturnStacksProp) : clearReturnStacksDefault;
if (clearReturnStacks) {
MethodNode result = WeaveUtils.newMethodNode(methodNodeCopy);
methodNodeCopy.accept(new ClearReturnAdapter(owner, methodNodeCopy, result));
methodNodeCopy = result;
Expand Down
43 changes: 41 additions & 2 deletions newrelic-weaver/src/main/java/com/newrelic/weave/MethodProcessors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.newrelic.api.agent.weaver.Weaver;
import com.newrelic.weave.utils.WeaveUtils;
import com.newrelic.weave.weavepackage.AnnotationProxyTemplate;
import com.newrelic.weave.weavepackage.WeavePackageConfig;
import org.objectweb.asm.ClassVisitor;
import org.objectweb.asm.Label;
import org.objectweb.asm.MethodVisitor;
Expand Down Expand Up @@ -56,11 +57,29 @@ private MethodProcessors() {
*/
public static MethodNode inlineMethods(final String inlineOwnerClassName, final Iterable<MethodNode> inlineMethods,
final String subjectOwnerClassName, final MethodNode subjectMethod) {
return inlineMethods(inlineOwnerClassName, inlineMethods, subjectOwnerClassName, subjectMethod, null);
}

/**
* Inline all of the specified method calls in the subject method.
*
* @param inlineOwnerClassName owner class of the methods to inline
* @param inlineMethods methods to inline
* @param subjectOwnerClassName owner class of the subject method that will have calls inlined in
* @param subjectMethod subject method to inline calls into
* @param weavePackageConfig config of the weave package the subject method belongs to, used to look up
* per-package defaults (e.g. return-stack normalization) that apply absent an explicit system property
* override; may be null if unavailable
* @return new subject method with all specified methods inlined
*/
public static MethodNode inlineMethods(final String inlineOwnerClassName, final Iterable<MethodNode> inlineMethods,
final String subjectOwnerClassName, final MethodNode subjectMethod,
final WeavePackageConfig weavePackageConfig) {

MethodNode result = WeaveUtils.newMethodNode(subjectMethod);

subjectMethod.accept(getInlineMethodsVisitor(subjectOwnerClassName, subjectMethod.access, subjectMethod.name,
subjectMethod.desc, result, inlineOwnerClassName, inlineMethods));
subjectMethod.desc, result, inlineOwnerClassName, inlineMethods, weavePackageConfig));

return result;
}
Expand All @@ -79,8 +98,28 @@ public static MethodNode inlineMethods(final String inlineOwnerClassName, final
*/
public static MethodVisitor getInlineMethodsVisitor(String owner, int access, String name, String desc,
MethodVisitor delegate, final String inlineOwnerClassName, final Iterable<MethodNode> inlineMethods) {
return getInlineMethodsVisitor(owner, access, name, desc, delegate, inlineOwnerClassName, inlineMethods, null);
}

/**
* Get a visitor that inlines all of the specified method calls into a subject method.
*
* @param owner subject method owner
* @param access subject method access
* @param name subject method name
* @param desc subject method desc
* @param delegate delegate to collect results
* @param inlineOwnerClassName owner of methods to inline
* @param inlineMethods methods to inline
* @param weavePackageConfig config of the weave package the subject method belongs to, used to look up
* per-package defaults that apply absent an explicit system property override; may be null if unavailable
* @return visitor that inlines all of the specified method calls into a subject method
*/
public static MethodVisitor getInlineMethodsVisitor(String owner, int access, String name, String desc,
MethodVisitor delegate, final String inlineOwnerClassName, final Iterable<MethodNode> inlineMethods,
final WeavePackageConfig weavePackageConfig) {

return new MethodCallInlinerAdapter(owner, access, name, desc, delegate, false) {
return new MethodCallInlinerAdapter(owner, access, name, desc, delegate, false, weavePackageConfig) {

@Override
protected InlinedMethod mustInline(String owner, String name, String desc) {
Expand Down
46 changes: 42 additions & 4 deletions newrelic-weaver/src/main/java/com/newrelic/weave/weavepackage/WeavePackageConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public static class Builder {
private boolean enabled = true;
private long priority = 0L;
private boolean custom = false;
private boolean clearReturnStacksDefault = false;
private WeaveViolationFilter weaveViolationFilter = null;
private ClassNode errorHandleClassNode = ErrorTrapHandler.NO_ERROR_TRAP_HANDLER;
private WeavePreprocessor preprocessor = WeavePreprocessor.NO_PREPROCESSOR;
Expand All @@ -59,6 +60,17 @@ public Builder name(String name) {
return this;
}

/**
* The name of the weave package as set so far, or null if not yet set. Useful for making decisions (e.g.
* about other builder parameters) based on the package name before calling {@link #build()}, since
* {@link WeavePackageConfig} itself is immutable once built.
*
* @return the package name as set so far, or null
*/
public String getName() {
return name;
}

/**
* Set the instance of {@link Instrumentation} to be used for weaving the bootstrap.
*
Expand Down Expand Up @@ -202,6 +214,19 @@ public Builder custom(boolean custom) {
return this;
}

/**
* Set whether return-instruction stack normalization (see {@code ReturnInsnProcessor.clearReturnStacks})
* should be applied by default when weaving methods from this package, absent an explicit
* {@code -Dnewrelic.config.class_transformer.clear_return_stacks} system property override.
*
* @param clearReturnStacksDefault whether to default return-stack normalization on for this package
* @return builder with updated state
*/
public Builder clearReturnStacksDefault(boolean clearReturnStacksDefault) {
this.clearReturnStacksDefault = clearReturnStacksDefault;
return this;
}

/**
* Set the {@link ClassNode} conforming to the {@link ErrorTrapHandler} specification to be used as the error
* trap.
Expand Down Expand Up @@ -331,8 +356,9 @@ public WeavePackageConfig build() {
if (null == name) {
throw new RuntimeException("WeavePackageConfig must have an Implementation-Name");
}
return new WeavePackageConfig(name, alias, vendorId, version, enabled, priority, source, custom, instrumentation,
errorHandleClassNode, preprocessor, postprocessor, extensionClassTemplate, weaveViolationFilter);
return new WeavePackageConfig(name, alias, vendorId, version, enabled, priority, source, custom,
clearReturnStacksDefault, instrumentation, errorHandleClassNode, preprocessor, postprocessor,
extensionClassTemplate, weaveViolationFilter);
}
}

Expand All @@ -358,6 +384,7 @@ public static Builder builder() {
private final String source;
private final boolean enabled;
private final boolean custom;
private final boolean clearReturnStacksDefault;
private final ClassNode errorHandleClassNode;
private final WeavePreprocessor preprocessor;
private final WeavePostprocessor postprocessor;
Expand All @@ -366,8 +393,9 @@ public static Builder builder() {
private final WeaveViolationFilter weaveViolationFilter;

private WeavePackageConfig(String name, String alias, String vendorId, float version, boolean enabled,
long priority, String source, boolean custom, Instrumentation instrumentation, ClassNode errorTrapClassNode,
WeavePreprocessor preprocessor, WeavePostprocessor postprocessor, ClassNode extensionClassTemplate,
long priority, String source, boolean custom, boolean clearReturnStacksDefault,
Instrumentation instrumentation, ClassNode errorTrapClassNode, WeavePreprocessor preprocessor,
WeavePostprocessor postprocessor, ClassNode extensionClassTemplate,
WeaveViolationFilter weaveViolationFilter) {
this.name = name;
this.alias = alias;
Expand All @@ -377,6 +405,7 @@ private WeavePackageConfig(String name, String alias, String vendorId, float ver
this.priority = priority;
this.source = source;
this.custom = custom;
this.clearReturnStacksDefault = clearReturnStacksDefault;

this.instrumentation = instrumentation;
this.errorHandleClassNode = errorTrapClassNode;
Expand Down Expand Up @@ -444,6 +473,15 @@ public boolean isCustom() {
return custom;
}

/**
* Whether return-instruction stack normalization should be applied by default when weaving methods from this
* package, absent an explicit {@code -Dnewrelic.config.class_transformer.clear_return_stacks} system property
* override.
*/
public boolean isClearReturnStacksDefault() {
return clearReturnStacksDefault;
}

/**
* The {@link WeaveViolationFilter} instance, if violations to filter are configured in the module manifest; null otherwise
*/
Expand Down
Loading
Loading