Community
Participate
Working Groups
Currently Eclipse compiler translates enum switch by creating a private static non-final int[] field named like $SWITCH_TABLE$EnumName and lazily fills it in the static method with the same name. The generated bytecode is roughly equivalent to the following Java program: private static int[] $SWITCH_TABLE$EnumName; static int[] $SWITCH_TABLE$EnumName() { int[] tmp = $SWITCH_TABLE$EnumName; if (tmp != null) { return tmp; } tmp = new int[EnumName.values().length]; try { tmp[EnumName.Val1.ordinal()] = 1; } catch (NoSuchFieldError ignored) { } try { tmp[EnumName.Val2.ordinal()] = 2; } catch (NoSuchFieldError ignored) { } ... return $SWITCH_TABLE$EnumName = tmp; } This method performs unsafe publication of $SWITCH_TABLE$EnumName, according to Java Memory Model. As values of int[] array are not final (array elements cannot be final) and $SWITCH_TABLE$EnumName is not volatile, no guarantees about ordering are made by JMM. In particular it's possible that `$SWITCH_TABLE$EnumName = tmp` write becomes visible in another thread prior to `tmp[EnumName.Val1.ordinal()] = 1` write. Thus, it's possible in valid JVM implementation that in another thread `tmp != null` check would return true and the corresponding switch statement would see yet-uninitialized elements of array erroneously going into the default switch branch. I cannot demonstrate it on the hardware and JVM implementation I have, because it's only single chance to step on it due to the class lifetime, and I'm not an expert in reproducing such kind of bugs. Probably it could be demonstrated on some JVM implementation using some existing hardware and some valid combination of VM options. But even if not, it doesn't mean that there's no problem. Spec is violated and future hardware/JIT improvements/memory layout changes could make it happen. The problem could be easily fixed declaring the synthetic field $SWITCH_TABLE$EnumName as `volatile`. This would introduce a happens-before edge between field write and subsequent reads and ensure that all previous writes (including writes to the array) are visible after the field read. More evident concurrency problem was fixed in bug#99282, but the fix still violates JMM.
Tagir, Thanks for the bug report and the proposed fix. I am not an expert in this area but your proposal sounds plausible at a quick look. We will take a look during 4.12.
Bulk move to 4.14
Bulk move out of 4.13
StackOverflow question: "Is Eclipse's $SWITCH_TABLE$ thread-safe?" (https://stackoverflow.com/q/58507978) https://github.com/Marcono1234/eclipse-enum-jcstress uses jcstress to test code which is similar to the one ecj would generate. However, the field is not static since, as mentioned by Tagir, it would make testing a lot more complicated because the field might only be initialized once per JVM. The test fails indeed (might have to run it multiple times) which means the current code is not thread-safe (unless the test code / setup is faulty).
New Gerrit change created: https://git.eclipse.org/r/154023
(In reply to Eclipse Genie from comment #5) > New Gerrit change created: https://git.eclipse.org/r/154023 This is basically the proposal from Tagir and makes the generated table field volatile. Would be nice if someone who understand this business could review that.
Andrey, to me this looks good. Though I might not have enough experience with the topic. However, what I noticed is that the code it generates for enum switches inside interfaces is rather weird. If I see that correctly it makes the switch array final and then never initializes it, so it always creates the temporary switch array. Should a separate issue be created for this, or do you want to solve it here as well? Can be seen for example with: public interface InterfaceEnumSwitch { enum EnumTest { FIRST, SECOND } static int get(EnumTest e) { switch (e) { case FIRST: return 1; case SECOND: return 2; } return -1; } }
Looks like fallen between cracks. Will revisit during 4.17. Andrey, meanwhile, please see if you can respond to comment #7
(In reply to Some User from comment #7) > Andrey, to me this looks good. Though I might not have enough experience > with the topic. > > However, what I noticed is that the code it generates for enum switches > inside interfaces is rather weird. If I see that correctly it makes the > switch array final and then never initializes it, so it always creates the > temporary switch array. Should a separate issue be created for this, or do > you want to solve it here as well? Would be great if we would have a dedicated bug for this.
Andrey, Please go ahead with the fix for RC1. Comment #7 can be taken up with a different bug.
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/154023 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=fe841a02686108d1034262caba01c8bf786d72ff
I've merged the "volatile" fix and created bug 566313 for comment 7.
Verified for 4.17 RC1 using build I20200824-1900.
