Various problems with @Advice.OnMethodEnter/Exit when used on constructors

[kriegaex]

Situation: I am writing a generic advice which works fine for all kinds of methods, even decorating already loaded JRE classes work fine. Now I want to extend it from @Origin Method method to @Origin Executable method in order to cover constructors, too. Later I might want to extend it to initialisers, but I have not looked into that yet. Unfortunately neither the manual nor the Javadoc explain much there, so I need to experiment.

I was expecting ByteBuddy to handle cases where some (combinations of) annotations are unsupported gracefully, i.e. just ignore them and maybe log a warning, similar to how @Advice.This is just set to null for static methods or constructors when before entering them. Unfortunately my expectation was not met. But let me not get ahead of myself and show you some code first:

Target class:

package de.scrum_master.agent.bytebuddy.example;

class UnderTest {
  static {
    System.out.println("static initialiser");
  }

  public UnderTest() {
    this("default");
    System.out.println("default constructor");
  }

  public UnderTest(String name) {
    System.out.println("constructor with parameter: " + name);
  }

  public int add(int a, int b) {
    System.out.println("instance method with parameters: " + a + ", " + b);
    return a + b;
  }

  static String greet(String recipient) {
    System.out.println("static method with parameter: " + recipient);
    return "Hello " + recipient;
  }
}

Driver application advising target class:

package de.scrum_master.agent.bytebuddy.example;

import net.bytebuddy.agent.ByteBuddyAgent;
import net.bytebuddy.agent.builder.AgentBuilder;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.matcher.ElementMatchers;

import java.lang.instrument.Instrumentation;

import static net.bytebuddy.matcher.ElementMatchers.*;

public class ExampleAgent {
  public static void premain(String options, Instrumentation instrumentation) {
    new AgentBuilder.Default()
      .disableClassFormatChanges()
      .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION)
      .with(AgentBuilder.RedefinitionStrategy.Listener.StreamWriting.toSystemError())
      .with(AgentBuilder.Listener.StreamWriting.toSystemError().withTransformationsOnly())
      .with(AgentBuilder.InstallationListener.StreamWriting.toSystemError())
      .type(ElementMatchers.nameContains("UnderTest"))
      .transform((builder, td, cl, m) -> builder.visit(Advice.to(MyConstructorInterceptor.class).on(isConstructor())))
      .installOn(instrumentation);
  }

  public static void main(String[] args) {
    premain(null, ByteBuddyAgent.install());
    UnderTest.greet("world");
    new UnderTest().add(1, 2);
    new UnderTest("John Doe").add(3, 4);
  }
}

Advice implementations:

First let me comment out some problematic stuff in order to start with a running before/after advice pair:

package de.scrum_master.agent.bytebuddy.example;

import net.bytebuddy.asm.Advice;

import java.lang.reflect.Executable;

import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;

public class MyConstructorInterceptor {

  @Advice.OnMethodEnter  //(skipOn = Advice.OnDefaultValue.class)
  public static boolean before(
    @Advice.This(typing = DYNAMIC, optional = true) Object target,
    @Advice.Origin Executable method,
    @Advice.AllArguments(readOnly = false, typing = DYNAMIC) Object[] args
  ) {
    System.out.println("[C] >> " + method);
    return true;
  }

  @Advice.OnMethodExit(/*onThrowable = Throwable.class,*/ /*backupArguments = false*/)
  public static void after(
    @Advice.This(typing = DYNAMIC, optional = true) Object target,
    @Advice.Origin Executable method,
    @Advice.AllArguments(readOnly = false, typing = DYNAMIC) Object[] args,
    @Advice.Enter boolean proceedMode,
    @Advice.Return(readOnly = false, typing = DYNAMIC) Object returnValue  //,
//    @Advice.Thrown(readOnly = false, typing = DYNAMIC) Throwable throwable
  ) {
    System.out.println("[C] << " + method);
  }

}

Console log:

[Byte Buddy] BEFORE_INSTALL net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer@3532ec19 on sun.instrument.InstrumentationImpl@68c4039c
[Byte Buddy] REDEFINE COMPLETE 0 batch(es) containing 0 types [0 failed batch(es)]
[Byte Buddy] INSTALL net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer@3532ec19 on sun.instrument.InstrumentationImpl@68c4039c
[Byte Buddy] TRANSFORM de.scrum_master.agent.bytebuddy.example.UnderTest [sun.misc.Launcher$AppClassLoader@18b4aac2, null, loaded=false]
static initialiser
static method with parameter: world
[C] >> public de.scrum_master.agent.bytebuddy.example.UnderTest()
[C] >> public de.scrum_master.agent.bytebuddy.example.UnderTest(java.lang.String)
constructor with parameter: default
[C] << public de.scrum_master.agent.bytebuddy.example.UnderTest(java.lang.String)
default constructor
[C] << public de.scrum_master.agent.bytebuddy.example.UnderTest()
instance method with parameters: 1, 2
[C] >> public de.scrum_master.agent.bytebuddy.example.UnderTest(java.lang.String)
constructor with parameter: John Doe
[C] << public de.scrum_master.agent.bytebuddy.example.UnderTest(java.lang.String)
instance method with parameters: 3, 4

So far, so good. Now let me start to modify the advice code a bit:

---

Activating this line in the exit advice

    @Advice.Thrown(readOnly = false, typing = DYNAMIC) Throwable throwable

yields

[Byte Buddy] ERROR de.scrum_master.agent.bytebuddy.example.UnderTest [sun.misc.Launcher$AppClassLoader@18b4aac2, null, loaded=false]
java.lang.IllegalStateException: Usage of interface net.bytebuddy.asm.Advice$Thrown is not allowed on java.lang.Throwable arg5
    at net.bytebuddy.asm.Advice$OffsetMapping$Factory$Illegal.make(Advice.java:1421)
    at net.bytebuddy.asm.Advice$Dispatcher$Resolved$AbstractBase.<init>(Advice.java:7033)
    at net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved.<init>(Advice.java:7308)

Is catching exceptions for constructors unsupported? The Javadoc of @Advice.Thrown does not mention anything like it.

Secondly, if it is unsupported couldn't this just be logged as a warning and throwable always set to null? Plus documentation and a comprehensible error message, of course. Now the whole advice does not work.

---

Anyway, let's activate the corresponding part of the OnMethodExit annotation too:

@Advice.OnMethodExit(onThrowable = Throwable.class/*, backupArguments = false*/)

This yields:

[Byte Buddy] ERROR de.scrum_master.agent.bytebuddy.example.UnderTest [sun.misc.Launcher$AppClassLoader@18b4aac2, null, loaded=false]
java.lang.IllegalStateException: Cannot catch exception during constructor call for public de.scrum_master.agent.bytebuddy.example.UnderTest()
    at net.bytebuddy.asm.Advice.doWrap(Advice.java:553)
    at net.bytebuddy.asm.Advice.wrap(Advice.java:508)
    at net.bytebuddy.asm.AsmVisitorWrapper$ForDeclaredMethods$Entry.wrap(AsmVisitorWrapper.java:573)

Okay, now the error message makes more sense. I seem to use an unsupported feature. But again, can ByteBuddy not handle this gracefully and just warn about it and ignore what it cannot use? I know, I could just write a very similar advice pair to the one covering methods for constructors, but I want to avoid it not just for reasons of unwanted code duplication but also for others I do not want to bother you with.

---

Okay, let me comment out the two exception handling parts in the exit advice again and try this:

@Advice.OnMethodEnter(skipOn = Advice.OnDefaultValue.class)

This yields:

[Byte Buddy] ERROR de.scrum_master.agent.bytebuddy.example.UnderTest [sun.misc.Launcher$AppClassLoader@18b4aac2, null, loaded=false]
java.lang.IllegalStateException: Cannot skip code execution from constructor: public de.scrum_master.agent.bytebuddy.example.UnderTest()
    at net.bytebuddy.asm.Advice$Dispatcher$RelocationHandler$ForValue$Bound.apply(Advice.java:6800)
    at net.bytebuddy.asm.Advice$Dispatcher$Inlining$CodeTranslationVisitor.visitEnd(Advice.java:8439)
    at net.bytebuddy.jar.asm.MethodVisitor.visitEnd(MethodVisitor.java:782)

Same request: Can I just get a warning instead and let ByteBuddy continue working? Maybe there is a configuration setting to allow for softening advice wiring exceptions to warnings and I am unaware of it. If not, maybe an optional parameter for OnMethodEnter and OnMethodExit could be introduced so as not to break backward compatibility for developers expecting exceptions but an option to make ByteBuddy behave differently.

BTW, of course Cannot skip code execution from constructor makes sense to me. I cannot just skip a constructor without throwing an exception or causing one. But as a feature I would like to be able to skip indeed and make the method return another compatible object instead. This would be nice in a mocking advice. One way to do that would be to actively assign a value to a @This parameter in the enter advice. Besides, this "exchange my object to be constructed for another one" mocking idea is one of the things I hoped to be able to implement with ByteBuddy, but there is no convenient way to do that withing the confines of my generic advice pair.

---

Now let's use backupArguments = false in the exit advice. To recap, the situation with commented out stuff is:

  @Advice.OnMethodEnter  //(skipOn = Advice.OnDefaultValue.class)

/* (...) */

  @Advice.OnMethodExit(/*onThrowable = Throwable.class, */backupArguments = false)

/* (...) */

//    @Advice.Thrown(readOnly = false, typing = DYNAMIC) Throwable throwable

Now it gets really bad:

[Byte Buddy] BEFORE_INSTALL net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer@3532ec19 on sun.instrument.InstrumentationImpl@68c4039c
[Byte Buddy] REDEFINE COMPLETE 0 batch(es) containing 0 types [0 failed batch(es)]
[Byte Buddy] INSTALL net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer@3532ec19 on sun.instrument.InstrumentationImpl@68c4039c
[Byte Buddy] TRANSFORM de.scrum_master.agent.bytebuddy.example.UnderTest [sun.misc.Launcher$AppClassLoader@18b4aac2, null, loaded=false]
Exception in thread "main" java.lang.VerifyError: Inconsistent stackmap frames at branch target 55
Exception Details:
  Location:
    de/scrum_master/agent/bytebuddy/example/UnderTest.<init>()V @55: getstatic
  Reason:
    Type 'de/scrum_master/agent/bytebuddy/example/UnderTest' (current frame, locals[0]) is not assignable to uninitializedThis (stack map, locals[0])
  Current Frame:
    bci: @52
    flags: { }
    locals: { 'de/scrum_master/agent/bytebuddy/example/UnderTest', integer }
    stack: { }
  Stackmap Frame:
    bci: @55
    flags: { flagThisUninit }
    locals: { uninitializedThis, integer }
    stack: { }
  Bytecode:
    0x0000000: b200 03bb 0007 59b7 0008 1248 b600 0a12
    0x0000010: 1203 bd00 4ab6 004e b600 51b6 000b b600
    0x0000020: 0504 a700 033c 2a12 01b7 0002 b200 0312
    0x0000030: 04b6 0005 a700 03b2 0003 bb00 0759 b700
    0x0000040: 0812 53b6 000a 1212 03bd 004a b600 4eb6
    0x0000050: 0051 b600 0bb6 0005 a700 03b1          
  Stackmap Table:
    same_locals_1_stack_item_frame(@37,Integer)
    append_frame(@38,Integer)
    same_frame_extended(@55)
    same_frame(@91)

    at de.scrum_master.agent.bytebuddy.example.ExampleAgent.main(ExampleAgent.java:33)

I do not think the VerifyError should occur. Is this a bug or am I doing anything wrong?

---

I am sorry for this very wordy issue, but after thinking about it I decided not to split it into different ones, all with 99% the same code which you can just easily manipulate in order to recreate the problems.

I am working on Windows 10 with Oracle JDK 8 (sorry!) and have not tried to reproduce with more up to date JDK versions. My ByteBuddy version is 1.10.10.

[raphw]

The VerifyError is a bug and I think I already understood it and have a fix ready. I will look into further optimizing the stack map frame; if a constructor has no branch after the super method call, currently the unitializedThis frame is not corrected to initialized state.

The easy solution is to always require a full frame but I'd like to explore the option to detect if a frame correction was applied and to only require the full frame in case of it.

[raphw]

The bug should be fixed by https://github.com/raphw/byte-buddy/commit/3fb9bbb85e7fe3af2d631d41df71f905b90577b3 - can you validate that this patch works for you by building master yourself?

[kriegaex]

Whoa, you caught me on the wrong foot here. I don't know nearly enough about the inner workings of classes and methods on the level of stack frames. But I am confident the fix is good. Gonna check it out soon and give feedback here.

[kriegaex]

I can confirm that the VerifyError is still reproducible in 1.10.10 but gone with the latest 1.10.11-SNAPSHOT. Thanks for the swift reaction. :-)

[kriegaex]

Any thought about my requests to handle superfluous annotations or annotation parameters gracefully, continuing with the rest which is applicable? Or what about making it configurable that processing continues? By now my work has progressed and I have separated my generic wrapper advice pairs for

• methods (both static and non-static),
• constructors,
• static type initialisers.

Especially the latter are very limited in what you can do other than suppress the type initialiser, but there still is a lot of very similar (next to duplicate) code between method and constructor advice pairs.

[raphw]

I think these issues are fixed, closing the ticket for it.

[kriegaex]

Hm, actually no, at least you never mentioned anything about it. I am referring to https://github.com/raphw/byte-buddy/issues/857#issuecomment-625846289 and BB handling unexpected annotations gracefully, allowing the user to have one advice pair for different types of targets (methods, constructors, static type initialisers). If you close as "won't fix", then of course the status of this ticket is correct.

[raphw]

There's been a ticket to support the feature you're wanting for a while, the verification error is adressed.

[kriegaex]

I cannot find the corresponding ticket. Maybe we are talking about different things. Would you mind pointing me to the ticket you mean? Then I can verify if it is what I mean. Thank you.

[raphw]

Isn't this what you were looking for? https://github.com/raphw/byte-buddy/issues/375

[kriegaex]

No, that is something very specific and completely different. I am talking about this part of my initial message here:

I was expecting ByteBuddy to handle cases where some (combinations of) annotations are unsupported gracefully, i.e. just ignore them and maybe log a warning, similar to how @Advice.This is just set to null for static methods or constructors when before entering them. Unfortunately my expectation was not met.

And then in https://github.com/raphw/byte-buddy/issues/857#issuecomment-625846289 I also said:

Any thought about my requests to handle superfluous annotations or annotation parameters gracefully, continuing with the rest which is applicable? Or what about making it configurable that processing continues? By now my work has progressed and I have separated my generic wrapper advice pairs for

• methods (both static and non-static),
• constructors,
• static type initialisers.

Especially the latter are very limited in what you can do other than suppress the type initialiser, but there still is a lot of very similar (next to duplicate) code between method and constructor advice pairs.

Please feel free to ask follow-up questions or to contact me via Gitter if my explanation is unclear.

[raphw]

Of course, this makes sense. It would probably be possible to add a property like ignoreUnsupported = true to allow for such overrides. This makes sense; if I find the time, I might add it.