public inbox for frysk@sourceware.org
 help / color / mirror / Atom feed
* inner classes with fields names similar to outer class/method  fields
@ 2007-07-02 12:32 Mark Wielaard
  2007-07-03 15:34 ` Andrew Cagney
  2007-07-05 18:43 ` Andrew Cagney
  0 siblings, 2 replies; 6+ messages in thread
From: Mark Wielaard @ 2007-07-02 12:32 UTC (permalink / raw)
  To: frysk

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);
        }
       });


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: inner classes with fields names similar to outer class/method   fields
  2007-07-02 12:32 inner classes with fields names similar to outer class/method fields Mark Wielaard
@ 2007-07-03 15:34 ` Andrew Cagney
  2007-07-03 17:27   ` Mark Wielaard
  2007-07-05 18:43 ` Andrew Cagney
  1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cagney @ 2007-07-03 15:34 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: frysk

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: inner classes with fields names similar to outer class/method  fields
  2007-07-03 15:34 ` Andrew Cagney
@ 2007-07-03 17:27   ` Mark Wielaard
  2007-07-03 18:44     ` Andrew Cagney
  0 siblings, 1 reply; 6+ messages in thread
From: Mark Wielaard @ 2007-07-03 17:27 UTC (permalink / raw)
  To: Andrew Cagney; +Cc: frysk

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: inner classes with fields names similar to outer class/method  fields
  2007-07-03 17:27   ` Mark Wielaard
@ 2007-07-03 18:44     ` Andrew Cagney
  2007-07-03 19:21       ` Mark Wielaard
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cagney @ 2007-07-03 18:44 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: frysk

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
>
>   

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: inner classes with fields names similar to outer class/method  fields
  2007-07-03 18:44     ` Andrew Cagney
@ 2007-07-03 19:21       ` Mark Wielaard
  0 siblings, 0 replies; 6+ messages in thread
From: Mark Wielaard @ 2007-07-03 19:21 UTC (permalink / raw)
  To: Andrew Cagney; +Cc: frysk

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: inner classes with fields names similar to outer class/method   fields
  2007-07-02 12:32 inner classes with fields names similar to outer class/method fields Mark Wielaard
  2007-07-03 15:34 ` Andrew Cagney
@ 2007-07-05 18:43 ` Andrew Cagney
  1 sibling, 0 replies; 6+ messages in thread
From: Andrew Cagney @ 2007-07-05 18:43 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: frysk

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);
>         }
>        });
>
>
>   

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2007-07-05 18:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-07-02 12:32 inner classes with fields names similar to outer class/method fields Mark Wielaard
2007-07-03 15:34 ` Andrew Cagney
2007-07-03 17:27   ` Mark Wielaard
2007-07-03 18:44     ` Andrew Cagney
2007-07-03 19:21       ` Mark Wielaard
2007-07-05 18:43 ` Andrew Cagney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).