Harmony
  1. Harmony
  2. HARMONY-1688

[DRLVM] Jitrino.OPT crashes on ClassTest

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: DRLVM
    • Labels:
      None
    • Environment:
      debug gcc 3.3.3 DRLVM r452709
      SLES 9 32-bit SP2; CPU 2xXeon x64 (Clovertown B, 4cores)

      Description

      The Jitrino.OPT fails with segmentation fault on org.apache.harmony.luni.tests.java.lang.ClassTest.
      To reproduce:

      > java -cp junit.jar:$classlib/modules/luni/bin/test:$classlib/deploy/build/test/support.jar junit.textui.TestRunner org.apache.harmony.luni.tests.java.lang.ClassTest

      SIGSEGV in VM code.
      Stack trace:
      1: Jitrino::TypeManager::toInternalType(Jitrino::Type*) (??:-1)
      2: Jitrino::JavaLabelPrepass::JavaLabelPrepass(Jitrino::MemoryManager&, Jitrino::TypeManager&, Jitrino::MemoryManager&, Jitrino::MethodDesc&, Jitrino::CompilationInterface&, Jitrino::Opnd**) (??:-1)
      3: Jitrino::alloc_arena(Jitrino::Arena*, unsigned int) (??:-1)
      4: Jitrino::alloc_arena(Jitrino::Arena*, unsigned int) (??:-1)
      5: ?? (??:-1)
      6: Jitrino::MemoryManager::alloc(unsigned int) (??:-1)
      7: Jitrino::Tree::computeNodeOrder(Jitrino::TreeNode*, unsigned int&, unsigned int&) (??:-1)
      8: ?? (??:-1)
      9: ?? (??:-1)
      10: Jitrino::JavaByteCodeTranslator::JavaByteCodeTranslator(Jitrino::CompilationInterface&, Jitrino::MemoryManager&, Jitrino::IRBuilder&, Jitrino::ByteCodeParser&, Jitrino::MethodDesc&, Jitrino::TypeManager&, Jitrino::JavaFlowGraphBuilder&) (??:-1)
      11: ?? (0015d890:15)
      12: Jitrino::MemoryManager::alloc(unsigned int) (??:-1)
      13: ?? (??:-1)
      14: Jitrino::JavaFlowGraphBuilder::JavaFlowGraphBuilder(Jitrino::MemoryManager&, Jitrino::IRBuilder&) (??:-1)
      15: method_get_byte_code_addr (/nfs/ins/proj/drl/coreapi/avarlamo/harmony/linux.ia32/svn-repo/drlvm/vm/vmcore/src/class_support/C_Interface.cpp:365)
      16: ?? (??:-1)
      17: ?? (??:-1)
      18: Jitrino::JavaTranslator::translateMethod(Jitrino::CompilationInterface&, Jitrino::MethodDesc&, Jitrino::IRBuilder&) (??:-1)
      <end of stack trace>

      1. H-1688.ClassTest.patch
        3 kB
        Alexey Varlamov
      2. H-1688_VM-initiating-loaders-support.patch
        7 kB
        Eugene S. Ostrovsky

        Issue Links

          Activity

          Hide
          Alexey Varlamov added a comment -

          The root cause of this crash lies in classloading: an attempt to resolve j.l.String returns null at jitrino/src/translator/java/JavaLabelPrepass.cpp, line 513.
          OTOH Jitrino itself is guilty in bad error handling here.

          Another related fact: test_getClasses_subtest0 of this test also fails on interpreter:
          java.lang.ClassCircularityError: java/lang/StringBuilder
          at org.apache.harmony.luni.tests.java.lang.ClassTest$1SecurityManagerCheck.checkPackageAccess(ClassTest.java:233)
          at java.net.URLClassLoader$SubURLClassLoader.loadClass(URLClassLoader.java:112)
          at java.lang.ClassLoader.loadClass(Unknown Source)
          at org.apache.harmony.luni.tests.java.lang.ClassTest$1SecurityManagerCheck.checkMemberAccess(ClassTest.java:221)
          at java.lang.Class.checkMemberAccess(Unknown Source) at java.lang.Class.getClasses(Unknown Source)
          at org.apache.harmony.luni.tests.java.lang.ClassTest$2.run(ClassTest.java:264)
          at java.security.AccessController.doPrivilegedImpl(Unknown Source)
          at java.security.AccessController.doPrivileged(Unknown Source)
          at org.apache.harmony.luni.tests.java.lang.ClassTest.test_getClasses_subtest0(ClassTest.java:333)
          at java.lang.reflect.VMReflection.invokeMethod(Native Method)

          Show
          Alexey Varlamov added a comment - The root cause of this crash lies in classloading: an attempt to resolve j.l.String returns null at jitrino/src/translator/java/JavaLabelPrepass.cpp, line 513. OTOH Jitrino itself is guilty in bad error handling here. Another related fact: test_getClasses_subtest0 of this test also fails on interpreter: java.lang.ClassCircularityError: java/lang/StringBuilder at org.apache.harmony.luni.tests.java.lang.ClassTest$1SecurityManagerCheck.checkPackageAccess(ClassTest.java:233) at java.net.URLClassLoader$SubURLClassLoader.loadClass(URLClassLoader.java:112) at java.lang.ClassLoader.loadClass(Unknown Source) at org.apache.harmony.luni.tests.java.lang.ClassTest$1SecurityManagerCheck.checkMemberAccess(ClassTest.java:221) at java.lang.Class.checkMemberAccess(Unknown Source) at java.lang.Class.getClasses(Unknown Source) at org.apache.harmony.luni.tests.java.lang.ClassTest$2.run(ClassTest.java:264) at java.security.AccessController.doPrivilegedImpl(Unknown Source) at java.security.AccessController.doPrivileged(Unknown Source) at org.apache.harmony.luni.tests.java.lang.ClassTest.test_getClasses_subtest0(ClassTest.java:333) at java.lang.reflect.VMReflection.invokeMethod(Native Method)
          Hide
          Alexey Varlamov added a comment -

          This testcase appears to be an abundant fountain of issues So far I found the following:
          1) The testcase effectively disables unprivileged classloading of system classes and thus implicitly checks how the VM under test handles requests to already loaded classes (in application security context). The DRLVM only caches internally a set of defined classes per classloader and relies on Java classes in obtaining initiated classes of any classloader. Anytime the system classloader is asked to loadClass(), it first tries to checkPackageAccess() and falls into recursion. That is why we get ClassCircularityError on interpreter and what provokes SIGSEGV in Jitrino.OPT.
          2) If we overcome the issue above, the testcase enforces somewhat stronger limitation: AccessController.checkPermission() should never lead to nested call for SecurityManager.checkPermission(), otherwise we have recursion again. I guess if we slightly hack the environment of this testcase (without changing testing logic itself) e.g. to use custom security policy provider, we'll be able to reproduce endless recursion on RI too. The DRLVM is more vulnerable to this due to it's pure-Java ACC impl peculiarities, it fails even with the default policy.

          Actually both issues are not justified with any specification I'm aware of, and both can be solved either via fixing the test or JRE impl (either VM and/or classes). So the question is: which solutions are more appropriate? I personally feel that the first issue should be fixed in VM (because it is quite likely to reoccur as real-world compatibility issue). And, strictly speaking, the second issue has no complete solution in JRE.

          Show
          Alexey Varlamov added a comment - This testcase appears to be an abundant fountain of issues So far I found the following: 1) The testcase effectively disables unprivileged classloading of system classes and thus implicitly checks how the VM under test handles requests to already loaded classes (in application security context). The DRLVM only caches internally a set of defined classes per classloader and relies on Java classes in obtaining initiated classes of any classloader. Anytime the system classloader is asked to loadClass(), it first tries to checkPackageAccess() and falls into recursion. That is why we get ClassCircularityError on interpreter and what provokes SIGSEGV in Jitrino.OPT. 2) If we overcome the issue above, the testcase enforces somewhat stronger limitation: AccessController.checkPermission() should never lead to nested call for SecurityManager.checkPermission(), otherwise we have recursion again. I guess if we slightly hack the environment of this testcase (without changing testing logic itself) e.g. to use custom security policy provider, we'll be able to reproduce endless recursion on RI too. The DRLVM is more vulnerable to this due to it's pure-Java ACC impl peculiarities, it fails even with the default policy. Actually both issues are not justified with any specification I'm aware of, and both can be solved either via fixing the test or JRE impl (either VM and/or classes). So the question is: which solutions are more appropriate? I personally feel that the first issue should be fixed in VM (because it is quite likely to reoccur as real-world compatibility issue). And, strictly speaking, the second issue has no complete solution in JRE.
          Hide
          Mikhail Fursov added a comment -

          Whatever solution is applied to fix VM<->Java interaction, I'm agree with you that Jitrino.OPT error handling must be fixed. I get TODO to find a solution and fix it in Jitrino.OPT.

          Show
          Mikhail Fursov added a comment - Whatever solution is applied to fix VM<->Java interaction, I'm agree with you that Jitrino.OPT error handling must be fixed. I get TODO to find a solution and fix it in Jitrino.OPT.
          Hide
          Egor Pasko added a comment -

          I do not think that it is critical for Jitrino to have a fix like you are proposing. The root cause is somewhere beyond the JIT.

          Jitrino assumes that only methods with all parameters resolved can be issued for compilation. We can put an assert for that (if it's not there yet). If some parameter is not resolved successfully, Jitrino.OPT can crash. Unfortunately, I cannot find any words in the VM Spec that could guarantee this. But I think it is the right assumption for JIT.

          What possible solutions I see (sorted by my preference)

          • it's quite natural for VM to check if parameters were resolved successfully (and throw LinkageError (or what??) if they are not), so, let VM do the job
          • JIT has unresolved params, so, they can only be nulls and can be treated as j.l.Object (because the resolution will always fail). Not so hard to do.
          • JIT may refuse to compile a method (returning JIT_FAILURE), what VM should do in that case becomes unclear to me (EM has something to do with it first). It is also quite complicated to return JIT_FAILURE correctly in current JIT impl.

          what else JIT can do for you?

          Show
          Egor Pasko added a comment - I do not think that it is critical for Jitrino to have a fix like you are proposing. The root cause is somewhere beyond the JIT. Jitrino assumes that only methods with all parameters resolved can be issued for compilation. We can put an assert for that (if it's not there yet). If some parameter is not resolved successfully, Jitrino.OPT can crash. Unfortunately, I cannot find any words in the VM Spec that could guarantee this. But I think it is the right assumption for JIT. What possible solutions I see (sorted by my preference) it's quite natural for VM to check if parameters were resolved successfully (and throw LinkageError (or what??) if they are not), so, let VM do the job JIT has unresolved params, so, they can only be nulls and can be treated as j.l.Object (because the resolution will always fail). Not so hard to do. JIT may refuse to compile a method (returning JIT_FAILURE), what VM should do in that case becomes unclear to me (EM has something to do with it first). It is also quite complicated to return JIT_FAILURE correctly in current JIT impl. what else JIT can do for you?
          Hide
          Pavel Pervov added a comment -

          There are three TODOs:
          0) JIT must not assume it has everything to be able to compile a method
          1) Current protocol between VM and JIT should be revised so VM could "throw LinkageError (or what)" from compilation of a method if resolution of parameter (or returm) type fails;
          2) JITs should resolve on execution path - not at the time of compilation.

          That is what JIT can do for all of us.

          Show
          Pavel Pervov added a comment - There are three TODOs: 0) JIT must not assume it has everything to be able to compile a method 1) Current protocol between VM and JIT should be revised so VM could "throw LinkageError (or what)" from compilation of a method if resolution of parameter (or returm) type fails; 2) JITs should resolve on execution path - not at the time of compilation. That is what JIT can do for all of us.
          Hide
          Mikhail Fursov added a comment -

          >JIT must not assume it has everything to be able to compile a method
          Yes, I vote to fix it in JIT once and forget about this problem forever. Otherwise it won't be the last time we discuss this problem.

          Show
          Mikhail Fursov added a comment - >JIT must not assume it has everything to be able to compile a method Yes, I vote to fix it in JIT once and forget about this problem forever. Otherwise it won't be the last time we discuss this problem.
          Hide
          Egor Pasko added a comment -

          > >JIT must not assume it has everything to be able to compile a method
          > Yes, I vote to fix it in JIT once and forget about this problem forever. Otherwise it won't be the last time we discuss this problem.
          sounds reasonable, but there are a lot of other reasons...
          It is not one place to fix, there are many assumptions on how VM should behave (especially verifier) the fix will increase the uglyness of the OPT.translator and I am not feeling like it helps somebody. A similar issue: turning verifier OFF will lead to a crash in JIT on many synthetic bytecodes. Fixing would take a lot of effort and is the thing nobody will benefit from, IMHO.

          > 2) JITs should resolve on execution path - not at the time of compilation.
          a) not easy to do
          b) that will lead to performance degradation.
          Do we really need it? I would vote not to touch the resolution strategy og OPT like that until there is a perfect reason not to resolve so eagerly. Currently I see no reason in inserting extra resolution helpers, performing more recompilations when references are actually resolved, etc.

          Show
          Egor Pasko added a comment - > >JIT must not assume it has everything to be able to compile a method > Yes, I vote to fix it in JIT once and forget about this problem forever. Otherwise it won't be the last time we discuss this problem. sounds reasonable, but there are a lot of other reasons... It is not one place to fix, there are many assumptions on how VM should behave (especially verifier) the fix will increase the uglyness of the OPT.translator and I am not feeling like it helps somebody. A similar issue: turning verifier OFF will lead to a crash in JIT on many synthetic bytecodes. Fixing would take a lot of effort and is the thing nobody will benefit from, IMHO. > 2) JITs should resolve on execution path - not at the time of compilation. a) not easy to do b) that will lead to performance degradation. Do we really need it? I would vote not to touch the resolution strategy og OPT like that until there is a perfect reason not to resolve so eagerly. Currently I see no reason in inserting extra resolution helpers, performing more recompilations when references are actually resolved, etc.
          Show
          Alexei Fedotov added a comment - Added to http://wiki.apache.org/harmony/Unit_Tests_Pass_on_DRLVM
          Hide
          Mikhail Fursov added a comment -

          Here is the minitest to separate Jitrino.OPT bug only.
          compile this code:

          public class Test {

          public static void main(String[] args)

          { foo(null); }

          static void foo(X x) {}
          }
          class X {
          }

          delete X.class file and run the test with -Xem:opt cmdline option.

          Show
          Mikhail Fursov added a comment - Here is the minitest to separate Jitrino.OPT bug only. compile this code: public class Test { public static void main(String[] args) { foo(null); } static void foo(X x) {} } class X { } delete X.class file and run the test with -Xem:opt cmdline option.
          Hide
          Mikhail Fursov added a comment -

          Actually this JIRA has multiple problems: VM and Jitrino ones.
          I moved Jitrino.OPT problem to the separate JIRA issue: http://issues.apache.org/jira/browse/HARMONY-1802
          So feel free to close this JIRA after VM problem is fixed.

          Show
          Mikhail Fursov added a comment - Actually this JIRA has multiple problems: VM and Jitrino ones. I moved Jitrino.OPT problem to the separate JIRA issue: http://issues.apache.org/jira/browse/HARMONY-1802 So feel free to close this JIRA after VM problem is fixed.
          Hide
          Alexey Varlamov added a comment -

          Yes, the H-1802 covers JIT side fully. Resolution strategy is a bit aside from the failure in question, I don't think we need to reconsider it in the nearest future.

          Show
          Alexey Varlamov added a comment - Yes, the H-1802 covers JIT side fully. Resolution strategy is a bit aside from the failure in question, I don't think we need to reconsider it in the nearest future.
          Hide
          Alexey Varlamov added a comment -

          Here is the fix for the test, better handling of recursion in security machinery. This eliminates issue (2) above.

          Show
          Alexey Varlamov added a comment - Here is the fix for the test, better handling of recursion in security machinery. This eliminates issue (2) above.
          Hide
          Geir Magnusson Jr added a comment -

          Then we need to turn this into a classlib JIRA, and discuss in that context.

          Shall I make it a classlib JIRA?

          Show
          Geir Magnusson Jr added a comment - Then we need to turn this into a classlib JIRA, and discuss in that context. Shall I make it a classlib JIRA?
          Hide
          Alexey Varlamov added a comment -

          Yet there is issue (1) - improper handling of "initiated" classes in VM, patch is coming soon.
          So we have both classlib and drlvm involved here. Shall I split to new JIRA?

          Show
          Alexey Varlamov added a comment - Yet there is issue (1) - improper handling of "initiated" classes in VM, patch is coming soon. So we have both classlib and drlvm involved here. Shall I split to new JIRA?
          Hide
          Eugene S. Ostrovsky added a comment -

          Fix for issue (1)
          This patch adds to VM support for recording initiating class loaders

          Show
          Eugene S. Ostrovsky added a comment - Fix for issue (1) This patch adds to VM support for recording initiating class loaders
          Hide
          Geir Magnusson Jr added a comment -

          patch hangs the testcase for me (w/o applying the fix to the test case in classlib).

          I think that we need to understand :

          1) did you expect ot hang the VM w/o me applying the classlib fix?

          2) I think that the classlib patch should be a separate JIRA. it's too confusing this way

          Show
          Geir Magnusson Jr added a comment - patch hangs the testcase for me (w/o applying the fix to the test case in classlib). I think that we need to understand : 1) did you expect ot hang the VM w/o me applying the classlib fix? 2) I think that the classlib patch should be a separate JIRA. it's too confusing this way
          Hide
          Alexey Varlamov added a comment -

          Yes, the original test is expected to hang even with the latest VM patch. I filed HARMONY-1895 to reduce confusion.
          The H-1688_VM-initiating-loaders-support.patch is good; with both patches applied test passes.

          Show
          Alexey Varlamov added a comment - Yes, the original test is expected to hang even with the latest VM patch. I filed HARMONY-1895 to reduce confusion. The H-1688_VM-initiating-loaders-support.patch is good; with both patches applied test passes.
          Hide
          Geir Magnusson Jr added a comment -

          r467374

          Only accepting the DRLVM patch. Please open a new JIRA with the classlib unit
          test patch

          Ubuntu 6 - c-unit, smoke, ~kernel

          Show
          Geir Magnusson Jr added a comment - r467374 Only accepting the DRLVM patch. Please open a new JIRA with the classlib unit test patch Ubuntu 6 - c-unit, smoke, ~kernel
          Hide
          Geir Magnusson Jr added a comment -

          closing - please take classlib test to new JIRA

          Show
          Geir Magnusson Jr added a comment - closing - please take classlib test to new JIRA

            People

            • Assignee:
              Geir Magnusson Jr
              Reporter:
              Alexey Varlamov
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development