Community
Participate
Working Groups
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
> 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.
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.)
(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
Created attachment 174162 [details] Patch under consideration
(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.
Created attachment 174166 [details] Full Patch Earlier patch was missing some changes.
Satyam, please review, TIA.
(In reply to comment #7) > Satyam, please review, TIA. The changes look good. +1
Released in HEAD for 3.7 M1.
Verified for 3.7M1.
