Skip to content
This repository was archived by the owner on Jun 23, 2020. It is now read-only.

Update API version in visitors to ASM5 #18

Closed
wants to merge 1 commit into from

Conversation

retronym
Copy link
Contributor

@retronym retronym commented May 10, 2016

We are using "org.pantsbuild" % "jarjar" % "1.6.0" in the SBT
build for Scala.

We see an error when we call JarJar with bytecode that contains
the MethodParameters attribute.

This seems to be due to the fact that you updated to ASM5, but
did not update all visitors in this project to the ASM5 api
version.

The ASM5's ClassReader visits these parameters, ultimately
with EmptyClassVisitor, which runs into:

    public void visitParameter(String name, int access) {
        if (api < Opcodes.ASM5) {
            throw new RuntimeException();
        }
        if (mv != null) {
            mv.visitParameter(name, access);
        }
    }

See:

https://docs.oracle.com/javase/tutorial/reflect/member/methodparameterreflection.html

for discussion of the MethodParameters classfile attribute. It is
not enabled by default, one needs to enable it with:

javac -help 2>&1 | grep parameters
  -parameters                Generate metadata for reflection on method parameters

No test is included here as I'm not familiar with pants to know how to
add this.

We are using "org.pantsbuild" % "jarjar" % "1.6.0" in the SBT
build for Scala.

We see an error when we call JarJar with bytecode that contains
the `MethodParameters` attribute.

This seems to be due to the fact that you updated to ASM5, but
did not update all visitors in this project to the ASM5 api
version.

The ASM5's `ClassReader` visits these parameters, ultimately
with `EmptyClassVisitor`, which runs into:

```
    public void visitParameter(String name, int access) {
        if (api < Opcodes.ASM5) {
            throw new RuntimeException();
        }
        if (mv != null) {
            mv.visitParameter(name, access);
        }
    }
```

See:

  https://docs.oracle.com/javase/tutorial/reflect/member/methodparameterreflection.html

for discussion of the `MethodParameters` classfile attribute. It is
not enabled by default, one needs to enable it with:

```
javac -help 2>&1 | grep parameters
  -parameters                Generate metadata for reflection on method parameters
```

No test is included here as I'm not familiar with pants to know how to
add this.
@retronym
Copy link
Contributor Author

The stacktrace that we observed in the debugger:

java.lang.RuntimeException
    at org.objectweb.asm.MethodVisitor.visitParameter(Unknown Source)
    at org.objectweb.asm.MethodVisitor.visitParameter(Unknown Source)
    at org.objectweb.asm.ClassReader.b(Unknown Source)
    at org.objectweb.asm.ClassReader.accept(Unknown Source)
    at org.objectweb.asm.ClassReader.accept(Unknown Source)
    at org.pantsbuild.jarjar.KeepProcessor.process(KeepProcessor.java:70)
    at org.pantsbuild.jarjar.util.JarProcessorChain.process(JarProcessorChain.java:38)
    at org.pantsbuild.jarjar.MainProcessor.process(MainProcessor.java:115)

Without the debugger, we just saw:

Error reading scala/tools/nsc/interpreter/jline/FileBackedHistory$.class: null
Error reading scala/tools/nsc/interpreter/jline/FileBackedHistory.class: null

Due to the way that the exception handling in JarJar employs the try { _ } catch (Exception e) err.println(e.getMessage) error handling antipattern.

retronym added a commit to retronym/scala that referenced this pull request May 10, 2016
  - Intercept incorrect "binary conflict" warning issued by SBT.
    Fixes scala/scala-dev#100
  - Add a simple way to disable use of JarJar during developer
    builds, as a workaround for scala/scala-dev#146. I've submitted
    a PR upstream to fix the root problem:
    pantsbuild/jarjar#18
  - Disable info level logging for dependency resolve/download.
retronym added a commit to retronym/scala that referenced this pull request May 10, 2016
  - Intercept incorrect "binary conflict" warning issued by SBT.
    Fixes scala/scala-dev#100
  - Add a simple way to disable use of JarJar during developer
    builds, as a workaround for scala/scala-dev#146. I've submitted
    a PR upstream to fix the root problem with parameter names:
    pantsbuild/jarjar#18
  - Disable info level logging for dependency resolve/download.
@stuhood
Copy link
Member

stuhood commented May 10, 2016

Thanks for the PR @retronym !

If you're able to extend one of the test classes in src/test/java/org/pantsbuild/jarjar/integration/ to cover this path, it would be much appreciated. If not, that's fine too. You can run those tests with:
./pants test src/test/java/org/pantsbuild/jarjar/integration
...and you can run all of the tests with :: (a recursive target glob):
./pants test ::

@retronym
Copy link
Contributor Author

@stuhood I tried to test drive the change in #19

I'm not totally happy with the resulting test, but have exhausted my budget of time, so have submitted as is.

@stuhood
Copy link
Member

stuhood commented May 15, 2016

Thanks! Went ahead and merged #19

@stuhood stuhood closed this May 15, 2016
adriaanm pushed a commit to adriaanm/scala that referenced this pull request May 16, 2016
  - Intercept incorrect "binary conflict" warning issued by SBT.
    Fixes scala/scala-dev#100
  - Add a simple way to disable use of JarJar during developer
    builds, as a workaround for scala/scala-dev#146. I've submitted
    a PR upstream to fix the root problem with parameter names:
    pantsbuild/jarjar#18
  - Disable info level logging for dependency resolve/download.
adriaanm pushed a commit to adriaanm/scala that referenced this pull request May 18, 2016
  - Intercept incorrect "binary conflict" warning issued by SBT.
    Fixes scala/scala-dev#100
  - Add a simple way to disable use of JarJar during developer
    builds, as a workaround for scala/scala-dev#146. I've submitted
    a PR upstream to fix the root problem with parameter names:
    pantsbuild/jarjar#18
  - Disable info level logging for dependency resolve/download.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants