Bug 310264

Summary:

Wrong warning: The assignment to variable has no effect

Product:

[Eclipse Project] JDT

Reporter:

Steffen Heil <bugs.eclipse.org>

Component:

Core

Assignee:

Srikanth Sankaran <srikanth_sankaran>

Status:

VERIFIED FIXED

QA Contact:

Severity:

minor

Priority:

CC:

Olivier_Thomann, satyam.kandula

Version:

3.6

Target Milestone:

3.7 M1

Hardware:

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
Patch under consideration	none
Full Patch	none

Description Steffen Heil

2010-04-23 07:16:29 EDT

Build Identifier: M20100211-1343

If as variable is assigned to itself, the compiler issues a warning, as in the following:

  int x = 2;
  x = x;

This warning is usually a sign of a programming problem, so for itself it is a good thing to have it, yet in the case that the variable is volatile, the warning is incorrect.

  int y = 0;
  volatile int x = 2;
  y = 3;
  x = x;

The java memory model enforces that every thread reading x and y in that order is guaranteed to see the value 3 for y. This is because assigning to x in one thread and reading x in the other establishes a happens-before-ordering.

Yet eclipse also warns, if this assignment is done with a volatile variable. I think that it should not warn for volatiles or have an option to disable warnings in that special case.

At least the warning is incorrect: The assignment has no effect to the value of the variable but it does have a huge effect for concurrency.

Note that in the example above the warning can be suppressed by using
  
  x = x + 0;

Yet there is no such hack for references. One case that hit me was when trying to sync the content of a int[]. One thread reads a member:  

  myarray[3] 

and another thread writes the member: 

  myarray[3] = 2;

Because array members themselfes cannot be volatile, I made myarray volatile and added the following to the writing thread:

  myarrray = myarray;

(Effecively enforcing a happens-before-order between this write and the member reads in the other thread.) Yet eclipse gave me the warning...

Reproducible: Always

Comment 1 Srikanth Sankaran

2010-07-12 01:24:21 EDT

> The java memory model enforces that every thread reading x and y in that order
> is guaranteed to see the value 3 for y. This is because assigning to x in one
> thread and reading x in the other establishes a happens-before-ordering.
> 
> Yet eclipse also warns, if this assignment is done with a volatile variable. I
> think that it should not warn for volatiles or have an option to disable
> warnings in that special case.

Per initial reading, this does sound correct. I'll look at suppressing
this warning for volatiles for 3.7 M1.

Comment 2 Steffen Heil

2010-07-12 05:20:35 EDT

One additional question:

In the meantime I get a little worried that the problem is bigger than just a warning: Does the compiler emmit any bytecode for such statements? Or is this optimized away? (In that case not only the warning was anoying, but also the resulting classes were broken.)

Comment 3 Srikanth Sankaran

2010-07-13 00:47:05 EDT

(In reply to comment #2)
> One additional question:
> 
> In the meantime I get a little worried that the problem is bigger than just a
> warning: Does the compiler emmit any bytecode for such statements? Or is this
> optimized away? (In that case not only the warning was anoying, but also the
> resulting classes were broken.)

All the compiler seems to do with the volatile modifier is to
faithfully expose it to downstream components and do some error
reporting as needed (e.g. illegal combination of "final" and
"volatile" etc)

I don't see the compiler doing any load avoidance in the first
place for volatile to have an effect:

(1) public class FetchEach {
	int field;
	public int thrice() {
		return field + field + field;
	}	
}

Code generated:

public int thrice();
  Code:
   Stack=2, Locals=1, Args_size=1
   0:   aload_0
   1:   getfield        #18; //Field field:I
   4:   aload_0
   5:   getfield        #18; //Field field:I
   8:   iadd
   9:   aload_0
   10:  getfield        #18; //Field field:I
   13:  iadd
   14:  ireturn

(2) public class FetchEach {
	volatile int field;
	public int thrice() {
		return field + field + field;
	}	
}


Code generated:
public int thrice();
  Code:
   Stack=2, Locals=1, Args_size=1
   0:   aload_0
   1:   getfield        #18; //Field field:I
   4:   aload_0
   5:   getfield        #18; //Field field:I
   8:   iadd
   9:   aload_0
   10:  getfield        #18; //Field field:I
   13:  iadd
   14:  ireturn

So we should be good.

(3) Returning to your original question, no this warning has no effect
on the code generated:

public class FetchEach { 
	volatile int field;
	public int thrice() {
		field = field;
		return 0;
	}	
}

produces the warning and generates the following:

public int thrice();
  Code:
   Stack=2, Locals=1, Args_size=1
   0:   aload_0
   1:   aload_0
   2:   getfield        #18; //Field field:I
   5:   putfield        #18; //Field field:I
   8:   iconst_0
   9:   ireturn

Comment 4 Srikanth Sankaran

2010-07-13 11:15:35 EDT

Created attachment 174162 [details]
Patch under consideration

Comment 5 Srikanth Sankaran

2010-07-13 11:20:33 EDT

(In reply to comment #4)
> Created an attachment (id=174162) [details]
> Patch under consideration

This patch (intentionally) leaves the warning in place for cases like:

class X { 
   volatile int x = this.x;\n" +
   int nvx = this.nvx;\n" +
}

however unlikely it is to be useful.

Comment 6 Srikanth Sankaran

2010-07-13 11:27:46 EDT

Created attachment 174166 [details]
Full Patch

Earlier patch was missing some changes.

Comment 7 Srikanth Sankaran

2010-07-14 01:09:51 EDT

Satyam, please review, TIA.

Comment 8 Satyam Kandula

2010-07-14 05:58:57 EDT

(In reply to comment #7)
> Satyam, please review, TIA.
The changes look good. +1

Comment 9 Srikanth Sankaran

2010-07-14 06:41:17 EDT

Released in HEAD for 3.7 M1.

Comment 10 Olivier Thomann

2010-08-04 15:50:09 EDT

Verified for 3.7M1.