critical Java regression

critical Java regression

[Per Bothner]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

When I do (cd gnu/math; make) given the attached sources,
I get the following:

CLASSPATH=.:../.. gcj -C  RealNum.java
gcj -g -I../.. -I. -c FixedRealFormat.java -o gnu-math.o
gnu/math/RealNum.java: In class `gnu.math.FixedRealFormat':
gnu/math/RealNum.java: In method 
`gnu.math.FixedRealFormat.format(gnu.math.RatNum,java.lang.StringBuffer)':
gnu/math/RealNum.java:0: reading class gnu.math.RealNum for the second 
time from ./RealNum.class
compilation terminated.

This case is simplified from Kawa.  It used to work last time
I tried this.  I consider this a release-critical regression.

The problem happens when trying to resolve RealNum.toScaledInt.
It first looks for a class 'RealNum.toScaledInt' and then looks
for 'RealNum'.  However, the latter does not check if the class
is already loaded.  Unfortunatey, I don't know this code well
enough to know should be happening.
-- 
	--Per Bothner
per@bothner.com   http://www.bothner.com/per/

[-- Attachment #2: math-test.tgz --]
[-- Type: application/x-gzip, Size: 584 bytes --]

Re: critical Java regression

[Per Bothner]

I'll file a PR if this is a new problem, and it can't
be fixed trivially.
-- 
	--Per Bothner
per@bothner.com   http://www.bothner.com/per/

Re: critical Java regression

[Mark Wielaard]

Hi,

On Mon, 2002-04-22 at 22:55, Per Bothner wrote:
> When I do (cd gnu/math; make) given the attached sources,
> I get the following:
> 
> CLASSPATH=.:../.. gcj -C  RealNum.java
> gcj -g -I../.. -I. -c FixedRealFormat.java -o gnu-math.o
> gnu/math/RealNum.java: In class `gnu.math.FixedRealFormat':
> gnu/math/RealNum.java: In method 
> `gnu.math.FixedRealFormat.format(gnu.math.RatNum,java.lang.StringBuffer)':
> gnu/math/RealNum.java:0: reading class gnu.math.RealNum for the second 
> time from ./RealNum.class
> compilation terminated.
> 
> This case is simplified from Kawa.  It used to work last time
> I tried this.  I consider this a release-critical regression.
> 
> The problem happens when trying to resolve RealNum.toScaledInt.
> It first looks for a class 'RealNum.toScaledInt' and then looks
> for 'RealNum'.  However, the latter does not check if the class
> is already loaded.  Unfortunatey, I don't know this code well
> enough to know should be happening.

Ugh, nasty. The reason it fails to see that the class is already loaded
is because the first time it loads ../../gnu/math/RealNum.class, but the
second time it loads RealNum.class from the current directory.

The first time it tries to read RealNum is when it resolves the RatNum
which has as superclass RealNum. The second time it tries to resolve
RealNum.toScaledInt, which makes it look for the class RealNum which
thinks to finds in the no-name package.

A quick workaround seems to be to just ignore this and only issue a
warning when coming across a second class file that defines the same
class.

Index: jcf-parse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/java/jcf-parse.c,v
retrieving revision 1.104.2.7
diff -u -r1.104.2.7 jcf-parse.c
--- jcf-parse.c	22 Apr 2002 20:36:47 -0000	1.104.2.7
+++ jcf-parse.c	22 Apr 2002 23:25:00 -0000
@@ -680,9 +680,10 @@
   if (CLASS_PARSED_P (current_class))
     {
       /* FIXME - where was first time */
-      fatal_error ("reading class %s for the second time from %s",
+      warning ("reading class %s for the second time from %s",
 		   IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (current_class))),
 		   jcf->filename);
+      return;
     }
   CLASS_PARSED_P (current_class) = 1;
 

Ugly, but it was the only thing I could come up with quickly.

Cheers,

Mark

Re: critical Java regression

[Per Bothner]

Mark Wielaard wrote:
> Ugh, nasty. The reason it fails to see that the class is already loaded
> is because the first time it loads ../../gnu/math/RealNum.class, but the
> second time it loads RealNum.class from the current directory.
> 
> The first time it tries to read RealNum is when it resolves the RatNum
> which has as superclass RealNum. The second time it tries to resolve
> RealNum.toScaledInt, which makes it look for the class RealNum which
> thinks to finds in the no-name package.
> 
> A quick workaround seems to be to just ignore this and only issue a
> warning when coming across a second class file that defines the same
> class.

I think there may be two separate more fundamental:  RealNum.toScaledInt
cannot possibly a class name, because of context (before a '(').
According to the Names chapter of the JLS, RealNum.toScaledInt is
unambiguously a MethodName.  Now RealNum is an AmbiguousName, but if
we're trying to find a class RealNum.toScaledInt we're probably going
about it the wrong way.

Thus there is a bug in how qualify_ambiguous_name calls
resolve_and_layout:  We should never call resolve_and_layout on
RealNum.toScaledInt in this case.

Now suppose we see 'RealNum.Foo.Bar('.  In this case RealNum.Foo
is an AmbiguousName.  However, the JLS is quite clear that to
classify an AmbiguousName RealNum.Foo you *first* have to
classify RealNum.  So again, looking for a class RealNum.Foo
without having *first* resolved RealNum is wrong.
-- 
	--Per Bothner
per@bothner.com   http://www.bothner.com/per/

Re: critical Java regression

[Tom Tromey]

>>>>> "Per" == Per Bothner <per@bothner.com> writes:

Per> I think there may be two separate more fundamental:
Per> RealNum.toScaledInt cannot possibly a class name, because of
Per> context (before a '(').  According to the Names chapter of the
Per> JLS, RealNum.toScaledInt is unambiguously a MethodName.  Now
Per> RealNum is an AmbiguousName, but if we're trying to find a class
Per> RealNum.toScaledInt we're probably going about it the wrong way.

I looked at this section of the JLS today.  I also dug through the
code a bit.  I think in the long term it would be really helpful if
the code in gcj matched what we see in the JLS, if that is possible.
Something for the "rewrite this" list I guess.

Tom