inner classes with fields names similar to outer class/method fields

inner classes with fields names similar to outer class/method fields

[Mark Wielaard]

Hi,

The following bug cost me a while to track down. The fix is easy. But if
you are not expecting this then you might be puzzled about it for a long
time like I was.

testSyscallRunning(frysk.proc.TestSyscallRunning)java.lang.NullPointerException
   at frysk.proc.TestSyscallRunning$2.execute(TestRunner)

The problem was that under some versions of the fc6 compiler this method
was miscompiled (!), under f7 the compiler gets it right. The anonymous
inner class that extends TaskEvent tries to refer to the final task
field of the method it is in. But TaskEvent has a field called task
itself. The TaskEvent.task field is never initialized and so stays null.
By explicitly renaming the final field in the outer method and using
that name in the inner class the reference is always correct (under
either the fc6 or f7 compiler).

2007-06-02  Mark Wielaard  <mwielaard@redhat.com>

        * TestSyscallRunning.java (testSyscallRunning): 
        Rename final task field to proc_task.

There should be a warning in the compiler to complain if such a
confusing name clash is detected.

Cheers,

Mark

diff -u -r1.12 TestSyscallRunning.java
--- frysk/proc/TestSyscallRunning.java  17 Apr 2007 19:45:09 -0000      1.12
+++ frysk/proc/TestSyscallRunning.java  2 Jul 2007 12:16:10 -0000
@@ -103,10 +103,10 @@
     // Get the port that will be listened on.
     int port = Integer.decode(in.readLine()).intValue();
 
-    final Task task = proc.getMainTask();
+    final Task proc_task = proc.getMainTask();
 
-    final SyscallObserver syso = new SyscallObserver("accept", task, false);
-    task.requestAddSyscallObserver(syso);
+    final SyscallObserver syso = new SyscallObserver("accept", proc_task, false
);
+    proc_task.requestAddSyscallObserver(syso);
 
     // Make sure the observer is properly installed.
     while (! syso.isAdded())
@@ -122,15 +122,15 @@
 
     // Now unblock and then attach another observer.
     // Do all this on the eventloop so properly serialize calls.
-    final SyscallObserver syso2 = new SyscallObserver("accept", task, true);
+    final SyscallObserver syso2 = new SyscallObserver("accept", proc_task, true
);
     Manager.eventLoop.add(new TaskEvent()
       {
        public void execute ()
        {
          // Continue running (inside syscall),
          // while attaching another syscall observer
-         task.requestUnblock(syso);
-         task.requestAddSyscallObserver(syso2);
+         proc_task.requestUnblock(syso);
+         proc_task.requestAddSyscallObserver(syso2);
        }
       });

Re: inner classes with fields names similar to outer class/method fields

[Andrew Cagney]

Mark Wielaard wrote:
> Hi,
>
> The following bug cost me a while to track down. The fix is easy. But if
> you are not expecting this then you might be puzzled about it for a long
> time like I was.
>
> testSyscallRunning(frysk.proc.TestSyscallRunning)java.lang.NullPointerException
>    at frysk.proc.TestSyscallRunning$2.execute(TestRunner)
>
> The problem was that under some versions of the fc6 compiler this method
> was miscompiled (!), under f7 the compiler gets it right. The anonymous
> inner class that extends TaskEvent tries to refer to the final task
> field of the method it is in. But TaskEvent has a field called task
> itself. The TaskEvent.task field is never initialized and so stays null.
> By explicitly renaming the final field in the outer method and using
> that name in the inner class the reference is always correct (under
> either the fc6 or f7 compiler).
>   
Mark,

Can you be more specific?  Frysk's build system already has a check for 
what sounds like a very similar bug vis:

class foo {
{
   func1() {
     class Bar {}
  }
  func2() {
    class Bar {}
  }
}


Andrew

Re: inner classes with fields names similar to outer class/method fields

[Mark Wielaard]

Hi Andrew,

On Tue, 2007-07-03 at 11:34 -0400, Andrew Cagney wrote:
> Mark Wielaard wrote:
> > The anonymous
> > inner class that extends TaskEvent tries to refer to the final task
> > field of the method it is in. But TaskEvent has a field called task
> > itself. The TaskEvent.task field is never initialized and so stays null.
> > 
> Can you be more specific?

Look at the attached patch of the original message, in particular:

> -    final Task task = proc.getMainTask();
> +    final Task proc_task = proc.getMainTask();
> [...]
>      Manager.eventLoop.add(new TaskEvent()
>        {
>         public void execute ()
>         {
>           // Continue running (inside syscall),
>           // while attaching another syscall observer
> -         task.requestUnblock(syso);
> -         task.requestAddSyscallObserver(syso2);
> +         proc_task.requestUnblock(syso);
> +         proc_task.requestAddSyscallObserver(syso2);
>         }
>        });

Now look at TaskEvent:

> public abstract class TaskEvent
>     implements Event
> {
>     [...]
>     protected Task task;
> [...]

And ask yourself which 'task' is being referred to in the execute()
method of the anonymous class given to the add() method. Is it the final
field in the outer class method called task, or is it the inherited
protected field called task from the super class TaskEvent? The "right"
answer is the protected field task from the super class, but since that
is in a completely different file while the final field task in the
enclosing method looks like the only task in scope some confusion
ensues. Renaming (one of) the fields is the best solution seeing some
compilers also seem to get this one wrong.

Cheers,

Mark

Re: inner classes with fields names similar to outer class/method fields

[Andrew Cagney]

Mark Wielaard wrote:
>
> And ask yourself which 'task' is being referred to in the execute()
> method of the anonymous class given to the add() method. Is it the final
> field in the outer class method called task, or is it the inherited
> protected field called task from the super class TaskEvent? The "right"
> answer is the protected field task from the super class, but since that
> is in a completely different file while the final field task in the
> enclosing method looks like the only task in scope some confusion
> ensues. Renaming (one of) the fields is the best solution seeing some
> compilers also seem to get this one wrong.
>   

Ok, this is a very different problem; I have my doubts that jv-scan 
could be used to detect this.  Can you create a frysk bug to track it 
and a test case for reference?

Andrew

> Cheers,
>
> Mark
>
>   

Re: inner classes with fields names similar to outer class/method fields

[Mark Wielaard]

On Tue, 2007-07-03 at 14:44 -0400, Andrew Cagney wrote:
> Ok, this is a very different problem; I have my doubts that jv-scan 
> could be used to detect this.

Note that jv-scan isn't supported (or bundled in recent Fedoras)
anymore.

>   Can you create a frysk bug to track it 
> and a test case for reference?

There you go:
http://sourceware.org/bugzilla/show_bug.cgi?id=4735
You can create a testcase from the patch and explanation I posted.
Code like that is confusing and should be avoided altogether.

Cheers,

Mark

Re: inner classes with fields names similar to outer class/method fields

[Andrew Cagney]

There are two postscripts to this thread:
- in looking at rewriting TaskEvent to eliminate the global variable 
issue I found that the below shouldn't even be using TaskEvent; instead 
it should use frysk.event.Event
- this code is susceptible to bug 4282 :-(

Mark Wielaard wrote:
>      Manager.eventLoop.add(new TaskEvent()
>        {
>         public void execute ()
>         {
>           // Continue running (inside syscall),
>           // while attaching another syscall observer
> -         task.requestUnblock(syso);
> -         task.requestAddSyscallObserver(syso2);
> +         proc_task.requestUnblock(syso);
> +         proc_task.requestAddSyscallObserver(syso2);
>         }
>        });
>
>
>