• Type: Bug
• Component: hotspot
• Sub-Component: compiler
• Affected Version: 9

• Priority: P2
• Status: Closed
• Resolution: Fixed

• Submitted: 2015-08-07
• Updated: 2017-08-04
• Resolved: 2015-09-15

JDK 9
9Fixed

I'm not sure that pointing to some function as 'undefined behavior' (instead adding some parameter checks) is good way to resolve this problem. And there are many different function for work with Class, Method, Constant Pool in ONE interface. And there is inconsistency naming functions when both are present *Class and *Klass (getKlassImplementor, getClassInitializer). I understand that these problems are not specifically related to this bug. But my intention was that you are improving the documentation, pay attention to these shortcomings. And since there are only 12 public methods in jvmci-9 (almost all of them have some comments) this bug can be considered as closed. 15-09-2015 [~tpivovarova], is this bug resolved? 08-09-2015 I've pushed a changeset that adds javadoc for everything in CompileToVM and adds a more robustness to it: http://hg.openjdk.java.net/graal/graal-jvmci-8/rev/e3d9e0f9b50c 12-08-2015 Even when JVMCI goes from being an experimental feature to a product one, there will be methods in CompilerToVM that are inherently unsafe. You simply cannot test that a metaspace pointer passed across this interface points to a valid metaspace object. For constant pool indexes, there are assertions and range checks in other JVMCI classes that use CompilerToVM. It would be costly and redundant to repeat these checks in CompilerToVM. With respect to your comment about a JVMCI compiler writer needing access to CompilerToVM, this is unfortunately true for some methods. However, we hope to address this before too long and make CompilerToVM package private. 12-08-2015 > I updated the javadoc for CompilerToVM.getExceptionTableStart (http://hg.openjdk.java.net/graal/graal-jvmci-8/file/8abc6030cf1c/jvmci/jdk.internal.jvmci.hotspot/src/jdk/internal/jvmci/hotspot/CompilerToVM.java#l65) to state that the behavior is undefined when the argument is 0. I'd prefer not to have undefined behaviour... smth like 'returns 0 in case of getExceptionTableLength returns 0' is more appropriate. > I'm not sure what the value of testing undefined behaviour is. Furthermore, such a test for this particular method can only be run on a release VM - is that acceptable? in case of UB, we won't test it. > Are you wanting to test that none of the methods in CompilerToVM can crash the VM? That's impossible in general since this is an inherently unsafe class (much like sun.misc.Unsafe) that can crash the VM if used incorrectly. That's why it is inaccessible to untrusted code (as described in the "Security Architecture" section at https://wiki.se.oracle.com/display/JPG/JVMCI+Design+Document). since jvmci project allows us to write JIT in java, I'd say it should work as normal java developers expect it to work, meaning not to crash JVM, but return a special value or throw an exception. but I do understand your point and we aren't going to write such tests right now, but I'd like JVMCI not to crash JVM even in incorrectly usages. it should be done before we change jvmci from a experimental feature to a product one. > That said, if there are tests for sun.misc.Unsafe you could point me at, I should be able come up with javadoc that allows you to write a similar set of limited tests for CompilerToVM. there are some tests for s.m.Unsafe in hotspot testsuite -- http://hg.openjdk.java.net/jdk9/dev/hotspot/file/tip/test/runtime/Unsafe I have to emphasize this bug wasn't about javadoc which can help us write tests, it's about missed good javadoc for a public class as well. even w/ jvmci design, one who writes JVMCI compiler have access to this class, so it has to be documented. however writing a good whole documentation won't be P2. thus I guess it makes sense to create a P2 subtask to write javadoc which allows us to write tests and change priority of this bug to P3-4 or create a separate bug for whole documentation. 11-08-2015 I updated the javadoc for CompilerToVM.getExceptionTableStart (http://hg.openjdk.java.net/graal/graal-jvmci-8/file/8abc6030cf1c/jvmci/jdk.internal.jvmci.hotspot/src/jdk/internal/jvmci/hotspot/CompilerToVM.java#l65) to state that the behavior is undefined when the argument is 0. I'm not sure what the value of testing undefined behaviour is. Furthermore, such a test for this particular method can only be run on a release VM - is that acceptable? Are you wanting to test that none of the methods in CompilerToVM can crash the VM? That's impossible in general since this is an inherently unsafe class (much like sun.misc.Unsafe) that can crash the VM if used incorrectly. That's why it is inaccessible to untrusted code (as described in the "Security Architecture" section at https://wiki.se.oracle.com/display/JPG/JVMCI+Design+Document). That said, if there are tests for sun.misc.Unsafe you could point me at, I should be able come up with javadoc that allows you to write a similar set of limited tests for CompilerToVM. 11-08-2015 > "In realease VM it passed with some value." > I'm not sure what you mean by "passed" because the value would be just a random address and certainly not a pointer to a ExceptionTableElement C++ object. that is the problem, I'd expect to return 0 in case of none exception table. and it should be stated in the javadoc. if you had a javadoc, you'd see the problem, since it'd say something like: "in case there is no exception table, the method returns random long value" > Anyway, as I've tried to state, CompilerToVM is an implementation class, not an API. You need to instead test the API classes (also mentioned above). For example, you could start by fleshing out the tests in jdk.internal.jvmci.runtime.test. If you search for the fields named "untestedApiMethods" in this package, you'll get an indication of what still needs testing. due to limited resources, we decided to start w/ the lowest layer, that allows us to found critical issues earlier, have direct tests which points to a narrow problem place. it's also part of requirements for jvmci integration to have all new native entities be covered by tests, and having tests which exercise these methods is the easiest way to get them. 11-08-2015 "In realease VM it passed with some value." I'm not sure what you mean by "passed" because the value would be just a random address and certainly not a pointer to a ExceptionTableElement C++ object. Anyway, as I've tried to state, CompilerToVM is an implementation class, not an API. You need to instead test the API classes (also mentioned above). For example, you could start by fleshing out the tests in jdk.internal.jvmci.runtime.test. If you search for the fields named "untestedApiMethods" in this package, you'll get an indication of what still needs testing. 11-08-2015 "How are you going to test for the correct behavior with an argument of 0? In a debug VM, a C++ assertion will be thrown and I have no idea what happens in a product VM." if you meant a correct method without an exception table # Internal Error (hotspot/src/share/vm/jvmci/jvmciCompilerToVM.cpp:228), pid=26801, tid=0x00007fc9783f6700 # assert(method->exception_table_length() != 0) failed: should be handled in Java code # JRE version: Java(TM) SE Runtime Environment (9.0) (build 1.9.0-internal-fastdebug-tpivovar_2015_08_03_15_38-b00) # Java VM: Java HotSpot(TM) 64-Bit Server VM (1.9.0-internal-fastdebug-tpivovar_2015_08_03_15_38-b00 mixed mode, aot linux-amd64 compressed oops) And if it "should be handled in Java code" why don't you do it in CompilerToVMImpl? In realease VM it passed with some value. If you meant case when metaspaceMethod == 0 then debug VM crashed. # Internal Error (hotspot/src/share/vm/runtime/handles.hpp:177), pid=25717, tid=0x00007ff00034c700 # assert(_value != __null) failed: resolving NULL _value # JRE version: Java(TM) SE Runtime Environment (9.0) (build 1.9.0-internal-fastdebug-tpivovar_2015_08_03_15_38-b00) # Java VM: Java HotSpot(TM) 64-Bit Server VM (1.9.0-internal-fastdebug-tpivovar_2015_08_03_15_38-b00 mixed mode, aot linux-amd64 compressed oops) In release VM this case crashed with SIGSEGV # SIGSEGV (0xb) at pc=0x00007f1d9a2afb34, pid=21681, tid=0x00007f1d1bfff700 # JRE version: Java(TM) SE Runtime Environment (9.0) (build 1.9.0-internal-tpivovar_2015_08_11_13_21-b00) # Java VM: Java HotSpot(TM) 64-Bit Server VM (1.9.0-internal-tpivovar_2015_08_11_13_21-b00 mixed mode, aot linux-amd64 compressed oops) 11-08-2015 Ok, let me try a different angle. To show me what kind of javadoc you're looking for, maybe you can tell me what kind of test you would like to write for CompilerToVM.notifyCompilationStatistics. I suspect that (at best) you can only write tests for invalid inputs. Even then, pure Java tests will be a problem. Look at the implementation of CompilerToVM.getExceptionTableStart (http://hg.openjdk.java.net/graal/graal-jvmci-8/file/8abc6030cf1c/src/share/vm/jvmci/jvmciCompilerToVM.cpp#l166). How are you going to test for the correct behavior with an argument of 0? In a debug VM, a C++ assertion will be thrown and I have no idea what happens in a product VM. Either way, this simply cannot be tested from Java. The point is that CompilerToVM is not API code but is used to provide a HotSpot specific implementation of API code. The API code for interaction with the JVM (and that needs testing) is in: jdk.internal.jvmci.meta jdk.internal.jvmci.code jdk.internal.jvmci.service jdk.internal.jvmci.runtime 11-08-2015 I understand that an interface should ideally provide a fully detailed contract independent of any implementation. However, CompilerToVM is only an interface for the purpose of be able to interpose on it for the purpose of replay or remote compilation. Otherwise, it really is just a way to call VM functionality and so it's tightly coupled to the HotSpot VM. As such, in most cases the semantics of the methods in CompilerToVM is exactly the semantics of the underlying VM method and so developing tests for these methods will have to require understanding the VM methods. If all the VM methods had API quality documentation, then we could (and would) simply copy that for the javadoc but unfortunately most of them do not. BTW the native implementation for this interface is in jvmciCompilerToVM.cpp. 11-08-2015 I understand your point of view, but from sqe point of view, CompilerToVM is one of the most important parts of jvmci, since it performs an actual interaction w/ jvm. so to write good tests, we have to have good described contracts for this interface. 11-08-2015 [~dnsimon], from the interface java-doc, I'd expect to provide the full detailed contract, including information that a method will do in case of invalid data. so c++ comments won't be enough. regarding blocking test development, we do test a feature, not its current implementation, so referring to code doesn't help, since it doesn't tell us that an user should expect from the feature, it just said that it does. 10-08-2015 Can you please comment more on how this is blocking test development for JEP 243? Most of these methods are simply entry points for calling methods on HotSpot metaspace objects. In those case, do you want javadoc to specify which metaspace methods are being called? For example, CompilerToVM.resolveConstantInPool simply calls ConstantPool::resolve_constant_at and the C++ comment for the latter method is "// Resolve late bound constants." 10-08-2015 raised priority since it encumbers test development for JEP 243 07-08-2015