Comment 1 Olivier Thomann 2011-03-07 14:35:00 EST

A must fix for M6.

Comment 2 Olivier Thomann 2011-03-07 15:05:00 EST

Reduced test case:
public abstract class X {
	private static Object[] bar() {
		return null;
	}
	protected final Object foo() {
			Object[] tab = null;
			if (tab != null) {
					Object[] v = bar();
					int length = tab.length;
					loop : for (int i = 0, max = v.length; i < max; i++) {
						Object o = v[i];
						if (o == null) {
							continue loop;
						}
						if (0 == length) {
							loop2 : for (int j = 0; j < length; j++) {
								Object o2 = null;
								for (int k = 0; k < length; k++) {
									if (o2 == tab[k]) {
										continue loop2;
									}
								}
								continue loop;
							}
							return o;
						}
					}
				}
		return null;
	}
}

This blows up with HEAD contents. It is related to the recent changes for dead code detection and code generation.
The code stream of the labels for the labeled statement are still null at code generation time causing the NPE.

Comment 3 Olivier Thomann 2011-03-07 15:28:13 EST

Created attachment 190598 [details]
Proposed fix

With this patch, the code compiles again and still reports a dead code warning.

Comment 4 Olivier Thomann 2011-03-07 15:30:16 EST

Stephan, Ayushman, please review the patch and propagate the same kind of changes to all possible locations.
I haven't checked in details other possible usages of UNREACHABLE instead of UNREACHABLE_OR_DEAD.
I also haven't run all tests yet.

Comment 5 Ayushman Jain 2011-03-08 04:30:54 EST

Created attachment 190636 [details]
proposed fix extended

Here's the same fix extended to some other places which affect code gen later on. Testing this now.

Comment 6 Ayushman Jain 2011-03-08 06:53:53 EST

Created attachment 190644 [details]
fix + tests

Same fix with added tests.
Note that the change in LocalDeclaration is to make sure we treat a local declaration in a null analysed dead code as reachable. As a consequence, local variable unused warnings also show up inside dead code now, which is good. 
Olivier/Stephan, if you don't have any objections with this fix, I'll release it.

Comment 7 Stephan Herrmann 2011-03-08 07:11:25 EST

(In reply to comment #5)
> Created attachment 190636 [details]
> proposed fix extended
> 
> Here's the same fix extended to some other places which affect code gen later
> on. Testing this now.

I had prepared a similar patch last night. But I must have been tired: 
I forgot to actually click submit..

So, while working independently we agree on the locations in FlowContext
and sub-classes. The common pattern is:

 - XFlowContext has a field initsOnY 
 - this field is initialized to FlowInfo.DEAD_END (!)
 - only during recordZFrom(..) is this field updated to something useful
 - this method only works if not dead code (need to use UNREACHABLE_OR_DEAD)

IMO in all those methods changing the outer if should suffice for the 
immediate issue. (The inner if in LoopingFlowContext only affects the
content of the flowInfo, it is unrelated to the NPE at hand).

With just these changes I ran tests.compiler and tests.model and they pass.

Also the occurrences in LocalDeclaration and MethodScope looked suspicious 
to me. I agree with changing these, too.

I missed the location in TryStatement, but that's a good catch, too.


On the semantic side I suspect FlowContext.recordSettingFinal(..) should
also be changed for correct analysis, but I don't see danger of an NPE here.

Comment 8 Stephan Herrmann 2011-03-08 07:15:18 EST

Now we had a mid-air collision of comments. ....

(In reply to comment #6)
> Created attachment 190644 [details]
> fix + tests
> 
> Same fix with added tests.
> Note that the change in LocalDeclaration is to make sure we treat a local
> declaration in a null analysed dead code as reachable. As a consequence, local
> variable unused warnings also show up inside dead code now, which is good. 
> Olivier/Stephan, if you don't have any objections with this fix, I'll release
> it.

Maybe you can briefly comment on two points where our findings differ:
- why change the inner if in LoopingFlowContext?
- how about FlowContext.recordSettingFinal?

thanks,
Stephan

Comment 9 Ayushman Jain 2011-03-08 08:59:29 EST

(In reply to comment #8)
> Maybe you can briefly comment on two points where our findings differ:
> - why change the inner if in LoopingFlowContext?
I couldn't really convince myself on changing it or leaving it as is. I was basically trying the following tweaked case (focus on the portion between the "///////.."):

public class X {
  
	private static Object[] bar() {
    	Object o = null;
    	
    	if (o != null) {
    		int a11 = 0; 
        	a11++;
	    	Object aa = null;
	    	if (aa == null) {
	    		aa.toString(); 
	    	}
    	}
        return null;
    }
    protected final Object foo() {
        Object[] tab = null;
        if (tab != null) {
                Object[] v = bar();
                int length = tab.length;
                loop : for (int i = 0, max = v.length; i < max; i++) {
                    Object o = v[i];
/////////////////////////////////////////////////
                    if (o == null) { 
                    	continue loop;    // CONTINUE 1
                    }
                    o = null;
                    if (o == null ) { 
                        continue loop;    // CONTINUE 2
                    }
////////////////////////////////////////////////////
                    if (0 == length) {
                        loop2 : for (int j = 0; j < length; j++) {
                            Object o2 = null;
                            for (int k = 0; k < length; k++) {
                                if (o2 == tab[k]) {
                                    continue loop2;
                                }
                            }
                            continue loop;
                        }
                        return o;
                    }
                }
            }
    return null;
}
}

I saw that if we just have the UNREACHABLE bit comparison, we don't merge the initsOnContinue  that we obtained from the CONTINUE 1, and just copy the flowInfo of the loop into the inits. I'm not sure if it'll have any consequence on code gen really. Frankly speaking, I wasn't able to make up my mind. We can use just the UNREACHABLE bit as well. 


> - how about FlowContext.recordSettingFinal?
I did consider this but didnt find a possibility of an NPE. Again, we can change this as well to be consistent.

Let me know what you think.

Comment 10 Stephan Herrmann 2011-03-08 09:15:37 EST

(In reply to comment #9)
> Let me know what you think.

It's probably safest to use UNREACHABLE_OR_DEAD in all mentioned locations
even if no direct impact on codegen can be shown now (as to minimize the danger 
of null-analysis causing grief).

At the same time I suggest to open fups for the issues where we still want
to investigate the exact impact on analysis in general. This would affect
at least both locations mentioned in comment 8 .
That why we can take our time to find test cases for those and still cleanly 
fix the NPE now.

Comment 11 Ayushman Jain 2011-03-08 11:35:32 EST

Created attachment 190668 [details]
same fix with suggested changes

I'll release this fix then. It passes all tests.

Comment 12 Ayushman Jain 2011-03-08 11:46:26 EST

Released in HEAD for 3.7M6

Comment 13 Ayushman Jain 2011-03-08 11:47:32 EST

Created attachment 190669 [details]
released patch

Same patch. Just fixed some comments.

Comment 14 Ayushman Jain 2011-03-08 12:14:04 EST

(In reply to comment #10)
> At the same time I suggest to open fups for the issues where we still want
> to investigate the exact impact on analysis in general.
Done. See bug 339253

Comment 15 Olivier Thomann 2011-03-10 09:15:13 EST

Verified for 3.7M6.

Comment 16 Jay Arthanareeswaran 2011-05-02 10:33:45 EDT

Verified with build I20110428-0848 for 3.7RC0