Bug 15250 - Need a better mapping for the method free return opcode
Summary: Need a better mapping for the method free return opcode
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 2.0 F2   Edit
Assignee: David Audel CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2002-05-03 19:52 EDT by Peter Burka CLA
Modified: 2002-06-03 11:32 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Burka CLA 2002-05-03 19:52:02 EDT
Build M5

Consider the following .java file:

public class Hello {

	public static void main(String[] args) {
		new Hello().doit();
	}

	public void doit() {
		String foo =
			((Object)null).toString();
	}

}

Eclipse produces the following debug table for doit():
  pc: 0 line number: 9
  pc: 4 line number: 8
  pc: 5 line number: 7

For the same file, javac (1.3) produces the following line number table:
  pc: 0 line number: 8
  pc: 8 line number: 10

The javac line number table results in a superior debug experience for the 
user.

It doesn't make sense to split the assignment up into two lines in the line 
number table. This makes the program difficult to debug, since a breakpoint on 
the first line of the method will never be hit!

I also don't understand why the implicit return got mapped to the method 
declaration.
Comment 1 Olivier Thomann CLA 2002-05-06 17:44:46 EDT
The disassembler output is:
/* Stack: 1, Locals: 2 */
0	aconst_null
1	invokevirtual #25 
<Method java.lang.Object>java.lang.String toString()>
4	astore_1
5	return

Line 
number attribute:
[pc: 0, line: 9]
[pc: 4, line: 8]
[pc: 5, line: 7]

The aconst_null is 
the generated code for the ((Object)null). This is on line 9. The astore_1 is the resulting code 
for the assignment. The assignment is done on line 8. So I don't see what is wrong.

For the 
implicit return being mapped to the line that contains the close brace of the method of the method 
declaration doesn't strike me at all. It is a matter of taste and I really don't find one is better 
than the other one.

Javac produces:
	Line number attribute:
		[pc: 0, line: 8]
		[pc: 
8, line: 10]

I cannot explain the first line. The opcode at 0 is aconst_null. Its 
corresponding source is on line 9. So, why is it mapped to line 8?

Putting a breakpoint of the 
first line of the method doesn't mean that this breakpoint will be hit all the time. In this case you 
put the breakpoint on an assignment line and it will be hit only if the assignment can be done. Like 
there is an exception before the assignment is executed, you won't hit the breakpoint. But 
looking at the stack trace:
java.lang.NullPointerException
	at 
Hello.doit(Hello.java:9)
	at Hello.main(Hello.java:4)
Exception in thread "main" 

can tell you that something bad happened on line 9. Put your breakpoint on that line and you will 
hit it.

I would not change anything to the line number table attribute because it really 
correspond to what it should be. Mapping between lines of code to generated code. This is the case, 
so there is nothing "boggus" or incorrect with that.

I would say that javac is incorrect in 
this case, because it doesn't make a difference between:
String foo = 
((Object)null).toString();
and:
String foo =
 ((Object)null).toString();
But there 
is one.

I consider our behavior as an enhancement, because multiple lines assignements 
result in multiple entries in the line number table attribute which results in a better debugging 
support.

If I debug the javac .class file, you get the first line highlighted and then you get a 
NPE without seeing the line containing the null.toString() highlighted. Then you have to 
understand what happened.

I don't see what we can fix in this case.
Comment 2 Olivier Thomann CLA 2002-05-06 18:16:31 EDT
It should be read:

I would say that javac is incorrect in
this case, because it doesn't make a 
difference between:
String foo = ((Object)null).toString(); // on a single 
line
and:
String foo =
 ((Object)null).toString();
But there is one.

It seems that 
bugzilla cut my line.
Comment 3 Olivier Thomann CLA 2002-05-07 10:40:22 EDT
Ok to close?
Comment 4 Peter Burka CLA 2002-05-07 11:19:26 EDT
I'm still not entirely convinced that we're doing the right thing.

It's very confusing for the user -- I'm quite experienced with Java and 
bytecodes, and I had to disassemble some class files to figure out why my 
breakpoint wasn't being hit.

But on the other hand I understand that the line number table is, strictly 
speaking, accurate. In fact it's more accurate than Sun's table.

Perhaps this is a problem better solved by the debugger, although I don't 
really see what they can do in this case, either.
Comment 5 Olivier Thomann CLA 2002-05-07 11:39:54 EDT
I think this is a limitation related to the way the line number table attribute is supposed to be 
built. We do a strict mapping from line numbers to positions in the bytes of the code attribute. In 
this case, we provide an accurate table. We cannot start introducing bugs in the line number table 
simply to fix this problem. Our table is generic and I think this specific case cannot be handle 
properly. Sun's line number table attribute is incorrect if I refer to the JVM specs. There is not 
much we can do to fix this, because there is really nothing to fix.
Comment 6 Olivier Thomann CLA 2002-05-07 14:08:51 EDT
Your case looks weird, because you don't assign a field. If you replace your example 
with:
1:public class Hello {
2:
3:	String foo;
4:	
5:	public static void 
main(String[] args) {
6:		new Hello().doit();
7:	}
8:
9:	public void doit() 
{
10:		foo =
11:			((Object)null).toString();
12:	}
13:
14:}
You generate the 
following table:
	Line number attribute:
		[pc: 0, line: 10]
		[pc: 1, line: 11]
		[pc: 
5, line: 10]
		[pc: 8, line: 9]
And putting a breakpoint on the first line will work as 
expected.
Sun is not improving their table:
	Line number attribute:
		[pc: 0, line: 
10]
		[pc: 11, line: 12]
So I really don't think we can change our actual behavior. If we map the 
opcode 0 onto the first line (line 8 in yout test case), we will lose the fact that the second line 
((Object) null).toString(); is highlighted with our table. Sun never highlights this line. I 
consider this behavior as incorrect. I will try to see if we can add another entry in your case.
Comment 7 Peter Burka CLA 2002-05-07 14:15:18 EDT
I suppose I would encounter the same problem if I did:

myStaticMethod(
 ((Object)null).toString()
);

so I'm convinced that it's probably not a problem that can be solved by the 
compiler.

Thanks for looking at it.
Comment 8 Olivier Thomann CLA 2002-05-07 14:29:00 EDT
If you write something like:
1:public class Hello {
2:
3:	public static void 
main(String[] args) {
4:		doit();
5:	}
6:
7:	public static void doit() 
{
8:		((Object)null)
9:			.toString();
10:	}
11:
12:}

Then we 
generate:
		[pc: 0, line: 8]
		[pc: 1, line: 9]
		[pc: 5, line: 7]
And it will behave as you 
expected it. The problem we have with your first test case is that there is no bytecode before the 
aconst_null which is on the next line compare to the "String foo = ". Then this will be the first 
entry in the table.
Comment 9 Peter Burka CLA 2002-05-07 14:35:31 EDT
I don't think that I was very clear.

I meant that:

1:public static void doit() {
2:  myStaticMethod(
3:    ((Object)null).toString()
4:  );
5:}

wouldn't stop at a breakpoint on line 2, since no bytecodes are run for that 
line until after the exception has been thrown.

But if myStaticMethod weren't static it would work (because of the 
aload0) . . . Java is weird sometimes <g>.

Comment 10 Olivier Thomann CLA 2002-05-07 16:23:49 EDT
In this case, javac persists to provide a table which doesn't match the lines in 
the source. Like they have the same problem, when a receiver is on the first 
line (aload_0), I believe it is a bug and not a specific way to handle this 
case.
Are you ok if we don't change anything?
Comment 11 Peter Burka CLA 2002-05-07 16:30:28 EDT
Yes. I'd still like to find a way to address this, but I think that the .class 
file debug info just isn't powerful enough. What you really want is expression 
info, not just line info.
Comment 12 Olivier Thomann CLA 2002-05-07 17:57:36 EDT
We should not put a entry for the implicit return to the method declaration line. The reason for 
that is that putting a breakpoint on the method declaration line is confusing, because the 
breakpoint is hit when the method is exited and not when the method is entered. This is not natural 
and could be improved by putting this entry on the line that contains the closing brace of the 
method.
Comment 13 Olivier Thomann CLA 2002-05-16 16:05:23 EDT
I think the best scenario is to exit the method when there is a free return instead of jumping to the 
line of the declaration of the method. Then the free return has the same behavior than a 
return.
Does this address your debugging issue?
Is this ok for you?
Comment 14 Olivier Thomann CLA 2002-05-21 11:46:18 EDT
Removing the entry for the free return is not a solution. The best solution seems to map this 
bytecode to the body end of the method declaration.
Comment 15 David Audel CLA 2002-06-03 11:32:17 EDT
Verified.