586057 - support stack map generation for Java 6 bytecode

Status: RESOLVED FIXED

(Rhino Graveyard :: Compiler, enhancement)

Description

[Daniel Tang]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.125 Safari/533.4
Build Identifier: cvs

Java 6 bytecode introduced a new code attribute which embeds type information at the beginning of every super block (where a super block is defined as a block of code with a single entry point but multiple exit points). The typical case for this is to allow the Java compiler to take some of the JVM load time away, but this is only applies for precompiled programs. In any case, future JVM versions may fail to load bytecode with recent versions without stack maps.

This is part one of updating Rhino to be able to output modern bytecode. The other will be to remove the use of the jsr instruction.

Reproducible: Always

Comment 1

[Daniel Tang]

Attachment: stack map implementation

patch -p1 < stackmap.patch

Comment 2

[Daniel Tang]

I forgot to mention: This patch assumes that removing jsr bytecode is forthcoming (and it is). Any test that uses jsr will fail with this patch, but it will work when jsr removal is reviewed/committed.

(In reply to comment #1)
> Created attachment 464535 [details] [diff] [review]
> stack map implementation
> 
> patch -p1 < stackmap.patch

Comment 3

[Hannes Wallnoefer]

Thanks for the patch! Can you provide some links to explain the relevant Java 6 bytecode changes such as the stack map attribute? A quick google search didn't turn up anything for me.

Comment 4

[Daniel Tang]

Sure, they are documented in JSR 202. Try this link:

http://jcp.org/aboutJava/communityprocess/final/jsr202/index.html

It should have a comparison PDF which highlights the changes, but the stack map is the only complicated part. I found this to be valuable for implementing this:

http://asm.ow2.org/doc/developer-guide.html#controlflow

The JSR is more or less just documentation of data structures and what they mean. They give very little to no information on how to implement stack maps in a way that passes the Sun split verifier. By the way, you can test for correctness on OpenJDK by "java -XX:-FailOverToOldVerifier" and making sure that no VerifyErrors are thrown. The flag disables the old, type checking verifier.

Comment 5

[Norris Boyd]

Hannes,

I looked at this code a bit. I'm in favor of accepting the patches. Here's the email exchange with Daniel:

me:
The code itself looks well-written and tight. Thanks for all your work
on it. Did you ever investigate generating the necessary information
to generate the stack maps in the code generator rather than
rediscovering it in the ClassFileWriter? Seems like that could be less
code and more efficient. Also, how frequently does Rhino generate dead
code? Seems like that might also be fixable in the code generator
which would be better for the size of the generated code.

Daniel:
I never investigated trying to generate the stack maps in Codegen, though I thought about it for awhile after my initial implementation. I didn't give it too much more thought, since my somewhat trivial benchmarks didn't show all too much overhead when generating benchmarks. I imagine the initial stack maps would be easy to generate from the code generation phase, but you still need to dive down into the bytecode to iterate over the stack maps until equilibrium is reached.

I think the dead code generation is fairly frequent. If I recall correctly, there is a lot of indirection using goto with exceptions and try/finally code that sometimes leaves dead code if a subroutine doesn't ret, for example. The only solutions I could think of were to somehow rewrite the bytecode at the end to eliminate nops, which would be painful, or implement some sort of CFA, which may be less painful. Given the narrow scope of my internship (and duration), I decided not to pursue dead code elimination, though it could be interesting. It might be a nice side project to do once I'm back in school, if I'm not extremely busy.

Comment 6

[Norris Boyd]

Patch submitted. Thanks, Daniel!

Comment 7

[Hannes Wallnoefer]

Thanks for the context, Daniel and Norris! It's great to have Rhino generate modern bytecode. 

I found a minor problem with this patch: when Rhino is on the bootclasspath, the class version detection in org.mozilla.classfile.ClassFileWriter line #4186 throws a NullPointerException because Class.getClassLoader() returns null in that case. 

Unfortunately, prepending Rhino to the bootclasspath was/is a common workaround for an OpenJDK bug where OpenJDK shipped with an old version of Rhino on the bootclasspath and thus disallowed people to use their own version of Rhino by normal means.

Comment 8

[Norris Boyd]

Reopening based on Hannes comment. Daniel, can you provide a fix?

Comment 9

[Hannes Wallnoefer]

Currently the "jar" ant task generates JDK 1.5 class files (major version 49) while the "retrojar" task creates JDK 1.4 classes (major version 48). I guess the safe route would be to fall back to 48 if class lookup fails. If that's fine with everybody I'll commit that change later today.

Comment 10

[Daniel Tang]

Sorry for the inactivity; I was busy wrapping up my internship last week and am moving/resuming school. Is there a better solution that would allow checking for class file version at runtime? I would think that if ant jar fails on the boot classpath, retrojar will fail too. Perhaps I don't understand what you're getting at with your solution.

I forgot to mention one minor, but important detail: in order to test stack map behavior, you should compile Rhino with "ant jar -Dtarget-jvm=1.6". I didn't bother mucking with the ant file, so this was my workaround.

Comment 11

[Hannes Wallnoefer]

(In reply to comment #10)
> Is there a better solution that would allow checking
> for class file version at runtime? 

I just discovered java.lang.ClassLoader.getSystemClassLoader and .getSystemResourceAsStream(), and it seems to work. I'll try to come up with a working patch.

> I would think that if ant jar fails on the
> boot classpath, retrojar will fail too. Perhaps I don't understand what you're
> getting at with your solution.

Sorry if I wasn't clear. I just mentioned the ant tasks to see what class files they produced to decide what version to use as fallback. The bootclasspath issue was a long-standing problem with OpenJDK that caused many projects and products using Rhino to prepend their version of it to the bootclasspath, so we should handle this situation gracefully.

Comment 12

[Hannes Wallnoefer]

Attachment: fixes for possible NullPointerException in class file detection

Comment 13

[Daniel Tang]

The logic makes sense to me. I haven't actually tried it, but I assume you have. I'm recreating my setup and can test it later tonight.

Comment 14

[Daniel Tang]

Okay, I tried it and it looks good to me. Thanks for pointing out the problem.

Comment 15

[Hannes Wallnoefer]

Thanks, I committed the second patch, closing again.

Comment 16

[Hannes Wallnoefer]

Attachment: catch exceptions in classfile version detection and go with default version

Unfortunately the code from the previous patch still throws a NullPointerException when running on Google App Engine (not related to bootclasspath but to security settings, I presume). This patch catches all exceptions in classfile version detection and uses 48/0 (Java 1.4) if classfile access fails.

Comment 17

[Daniel Tang]

Interesting. I would have expected a security-related exception rather than another NPE. I don't know much about GAE though. Looks good to me, though I can't test on GAE.

Comment 18

[Hannes Wallnoefer]

I committed the second patch, no more exception on GAE.

Comment 19

[Daniel Tang]

Great. If you and Norris could look at bug 586062 next, that would be excellent. I think it's less straightforward, but I added as much documentation as I could. The patch there is needed to bring the test suite back into a working state.