Bug in Jitter

[Godmar Back]

 Alexandre,

> 
> AFAIK, the only case that may raise this kind of exception is
> NullPointerException.  Any other case would be the result of calling a
> soft_* function or a method, and this would have spilled all registers
> before the call.  Thus, I see two alternatives:
> 
> 1) #define CHECK_NULL(a,b,c) sync_registers()
>    unless it is already defined in some platform-specific
>    header-file.
> 
>    This would ensure that any registers are saved in the stack before
>    a NullPointerException may be thrown, with a very small performance
>    impact.  Only registers that are dirty and would be overwritten in
>    the near future will be unnecessarily spilled; others may be
>    written to the stack frame earlier and marked as clean, so they
>    won't be spilled any more.
> 
>    sync_registers() does not exist on the intrp, so it may be defined
>    to nothing in this case.  Furthermore, on x86, a patch that ensures
>    that sync_registers() does not remove a floating-point value from
>    the top of the FP stack must be installed.
> 

 I bet you wrote this off the top of your head or with a non-current
version in mind.  The CHECK_NULL macro is defined in machine.c, so there's
no need to define a sync_register for the interpreter.  I don't think
it's intended to be redefined by md.h either.

Also, sync_register doesn't reset the modified flags.  Therefore,
the register would unnecessarily spilled again if a function call follows.
I think what we want is prepare_function_call() (maybe minus the part 
where temps are spilled?).

It seems that a similar situation arises for BadArrayIndex
exceptions.  jit/icode.c already emits conditional spill code if that
can happen.  The code is conditional in the sense that a canCatch()
macro will only emit it if the exception in question is in fact caught
in the current method.

With all that said:  I tried it, and I believe your idea works.

I defined CHECK_NULL for the hardware version like this:

+#define CHECK_NULL(_i, _s, _n)	\
+	if (canCatch(NULLPOINTER)) {		\
+		prepare_function_call(); 	\
+	}

where I added NULLPOINTER as a second flag to BADARRAYINDEX.

I append the patches to baseClasses.c and jit/machine.[ch].
They fix the FinalTest example and pass the regression tests.

For what it's worth, I counted the instructions generated while jitting
a large javac compile:

434420: kaffe with CREATE_NULLPOINTER_CHECKS defined.
312067: kaffe with if canCatch(NULL) -> prepare_function_call
306914:	current, incorrect Kaffe

Could you elaborate some more on that patch for the x86 you're mentioned?

	- Godmar

-- patches --

Index: baseClasses.c
===================================================================
RCS file: /home/cvspublic/kaffe/kaffe/kaffevm/baseClasses.c,v
retrieving revision 1.5
diff -u -r1.5 baseClasses.c
--- baseClasses.c	1998/07/06 23:43:01	1.5
+++ baseClasses.c	1998/07/08 02:56:21
@@ -56,6 +56,7 @@
 Hjava_lang_Class* javaLangDoubleClass;
 
 Hjava_lang_Class* javaLangArrayIndexOutOfBoundsException;
+Hjava_lang_Class* javaLangNullPointerException;
 
 #define SYSTEMCLASS "java/lang/System"
 #define	SERIALCLASS "java/io/Serializable"
@@ -163,6 +164,7 @@
 
 	/* Exception handling types */
 	loadStaticClass(&javaLangArrayIndexOutOfBoundsException, "java/lang/ArrayIndexOutOfBoundsException");
+	loadStaticClass(&javaLangNullPointerException, "java/lang/NullPointerException");
 
 	/* Fixup primitive types */
 	finishTypes();
cvs server: Diffing jit
Index: jit/machine.c
===================================================================
RCS file: /home/cvspublic/kaffe/kaffe/kaffevm/jit/machine.c,v
retrieving revision 1.3
diff -u -r1.3 machine.c
--- machine.c	1998/05/28 18:37:01	1.3
+++ machine.c	1998/07/08 02:56:21
@@ -66,6 +66,7 @@
 
 /* Various exception related things */
 extern Hjava_lang_Class javaLangArrayIndexOutOfBoundsException;
+extern Hjava_lang_Class javaLangNullPointerException;
 
 jitflags willcatch;
 
@@ -77,7 +78,10 @@
       softcall_nullpointer();                                 \
       set_label(_i, _n)
 #else
-#define CHECK_NULL(_i, _s, _n)
+#define CHECK_NULL(_i, _s, _n)	\
+	if (canCatch(NULLPOINTER)) {		\
+		prepare_function_call(); 	\
+	}
 #endif
 
 /* Unit in which code block is increased when overrun */
@@ -193,6 +197,7 @@
 	len = meth->c.bcode.codelen;
 
 	willcatch.BADARRAYINDEX = false;
+	willcatch.NULLPOINTER = false;
 
 	/* Deterimine various exception conditions */
 	if (meth->exception_table != 0) {
@@ -201,10 +206,14 @@
 			etype = meth->exception_table->entry[i].catch_type;
 			if (etype == 0) {
 				willCatch(BADARRAYINDEX);
+				willCatch(NULLPOINTER);
 			}
 			else {
 				if (instanceof(&javaLangArrayIndexOutOfBoundsException, etype)) {
 					willCatch(BADARRAYINDEX);
+				}
+				if (instanceof(&javaLangNullPointerException, etype)) {
+					willCatch(NULLPOINTER);
 				}
 			}
 		}
Index: jit/machine.h
===================================================================
RCS file: /home/cvspublic/kaffe/kaffe/kaffevm/jit/machine.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 machine.h
--- machine.h	1998/03/31 19:10:54	1.1.1.1
+++ machine.h	1998/07/08 02:56:21
@@ -98,6 +98,7 @@
 
 typedef struct {
         bool BADARRAYINDEX;
+        bool NULLPOINTER;
 } jitflags;
 
 extern jitflags willcatch;