-
Notifications
You must be signed in to change notification settings - Fork 733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Crash in javac compiling functionality tests #10623
Comments
@andrewcraik fyi |
that backtrace looks very dubious - esp since it is windows - the offset is insane.... |
yup, but there is a core file to look at, available for a short time. |
Another "similar" crash, adding to the 0.23 milestone plan. MauveSingleInvocationLoadTest_special_22
|
@rpshukla Could you do a grinder to get the failure rate? |
Not really sure what's going on. I took a look at the core from the original crash this issue was opened against. Even with symbols, windbg wasn't able to print out the backtrace properly. However, looking at it manually:
Crashing instruction
Stack:
Method before calling
Note,
and is pushed on to the stack before the call to However, note also that
So
would have resulted in the thread jumping over the call to |
Heh, actually, thinking on it a bit more I believe I figured out what's going on. It's caused by a race condition because of the compiler (visual studio) privatizing a field but not consistently using the privatized version. At the time The compiler chose to use Not 100% sure what a "fix" should be here... @andrewcraik do you have any suggestions? The relevant code is https://github.com/eclipse/openj9/blob/352f71b11badb3a9fd137c84237810224c36855a/runtime/compiler/control/HookedByTheJit.cpp#L867-L869 |
The reason for stating this is the C++ code is technically correct here because: if |
Leo mentioned the issue might be happening because of a lack of
|
My first reaction was that we are missing a volatile. If it is that we are not using a single canonical definition of the extra field and others could be making changes then we need to mark extra volatile to stop the compiler privatizing and doing other things to stop reads or writes from memory IMO. |
10x grinder for MauveSingleInvocationLoadTest_special_22 passed: running more grinders: |
I am strongly against this - it's a hack which fixes exactly one place. Does this even have any real meaning in the specification? We had to do this in a few places in the interpreter to fix ZOS XLC, but the correct solution would have been to make the field volatile. It also results in the compiler doing very foolish things (even though it correctly read the field and stored it to the stack, it would go to the stack every time instead of registerizing the value, which makes no sense). |
Is it possible for |
Yes, the field would need to be volatile and also read only once into a local. |
Another similar crash.
|
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_sanity.functional_x86-32_windows_OMR_testList_0/105
There is a core file in the artifact.
May be the same problem as #10490
The text was updated successfully, but these errors were encountered: